Skip to content

Remove access to process.env from hot code paths #5657

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
glasser merged 7 commits into from
Sep 30, 2021
Merged

Remove access to process.env from hot code paths #5657

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0d7113c
Remove access to process.env from hot code paths
moberegger Aug 26, 2021
8e1d2cc
Add nodeEnv option to allow injecting node environment value
moberegger Aug 30, 2021
61be692
Accommodate tests with nodeEnv
moberegger Sep 1, 2021
34acf3f
Use empty string to signify unset node environment
moberegger Sep 30, 2021
15f7b30
Add additional note about runtime changes to NODE_ENV
moberegger Sep 30, 2021
e8b1f0d
Disallow explicit `undefined` for `nodeEnv` (missing is OK)
glasser Sep 30, 2021
ec9ea61
Docs and changelog
glasser Sep 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion packages/apollo-datasource-rest/src/RESTDataSource.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ export interface CacheOptions {
export type Body = BodyInit | object;
export { Request };

const NODE_ENV = process.env.NODE_ENV;

export abstract class RESTDataSource<TContext = any> extends DataSource {
httpCache!: HTTPCache;
context!: TContext;
Expand Down Expand Up @@ -282,7 +284,7 @@ export abstract class RESTDataSource<TContext = any> extends DataSource {
request: Request,
fn: () => Promise<TResult>,
): Promise<TResult> {
if (process.env.NODE_ENV === 'development') {
if (NODE_ENV === 'development') {
Comment on lines -285 to +287

@moberegger moberegger Aug 26, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not clear in the diff, but this is for the trace method. A production environment would still pay the cost for this check, even though it would eventually evaluate to false and skip the tracing.

@glasser glasser Aug 27, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is fine. RESTDataSource hasn't been actively maintained in a little while (the current Apollo Server team has not yet ramped up on it; it's basically an entirely unrelated project that is very loosely coupled to Apollo Server). I don't know if anyone depends on the ability to set process.env.NODE_ENV at runtime (eg, potentially after this file is imported) and have it take effect. But if it breaks people we can revert it.

// We're not using console.time because that isn't supported on Cloudflare
const startTime = Date.now();
try {
Expand Down
22 changes: 9 additions & 13 deletions packages/apollo-server-core/src/ApolloServer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,10 @@ export class ApolloServerBase<
// The constructor should be universal across all environments. All environment specific behavior should be set by adding or overriding methods
constructor(config: Config<ContextFunctionParams>) {
if (!config) throw new Error('ApolloServer requires options.');
this.config = config;
this.config = {
...config,
nodeEnv: config.nodeEnv ?? process.env.NODE_ENV,
};
Comment thread
moberegger marked this conversation as resolved.
Comment thread
moberegger marked this conversation as resolved.
const {
context,
resolvers,
Expand All @@ -170,7 +173,7 @@ export class ApolloServerBase<
mockEntireSchema,
experimental_approximateDocumentStoreMiB,
...requestOptions
} = config;
} = this.config;
Comment thread
moberegger marked this conversation as resolved.

// Setup logging facilities
if (config.logger) {
Expand Down Expand Up @@ -206,16 +209,7 @@ export class ApolloServerBase<
this.experimental_approximateDocumentStoreMiB =
experimental_approximateDocumentStoreMiB;

// Allow tests to override process.env.NODE_ENV. As a bonus, this means
// we're only reading the env var once in the constructor, which is faster
// than reading it over and over as each read is a syscall. Note that an
// explicit `__testing_nodeEnv__: undefined` overrides a set environment
// variable!
const nodeEnv =
'__testing_nodeEnv__' in config
? config.__testing_nodeEnv__
: process.env.NODE_ENV;
const isDev = nodeEnv !== 'production';
const isDev = this.config.nodeEnv !== 'production';

// We handle signals if it was explicitly requested, or if we're in Node,
// not in a test, not in a serverless framework, and it wasn't explicitly
Expand All @@ -224,7 +218,9 @@ export class ApolloServerBase<
this.stopOnTerminationSignals =
typeof stopOnTerminationSignals === 'boolean'
? stopOnTerminationSignals
: isNodeLike && nodeEnv !== 'test' && !this.serverlessFramework();
: isNodeLike &&
this.config.nodeEnv !== 'test' &&
!this.serverlessFramework();

// if this is local dev, introspection should turned on
// in production, we can manually turn introspection on by passing {
Expand Down
1 change: 1 addition & 0 deletions packages/apollo-server-core/src/__tests__/runHttpQuery.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ describe('runHttpQuery', () => {
query: '{ testString }',
},
options: {
debug: false,

@moberegger moberegger Sep 1, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was required to avoid returning a stack trace, which the test didn't expect. Without running through ApolloServer first, the nodeEnv stuff never runs, so it doesn't get setup correctly for this test. This avoids that.

schema,
schemaHash: generateSchemaHash(schema),
},
Expand Down
2 changes: 1 addition & 1 deletion packages/apollo-server-core/src/graphqlOptions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ export interface GraphQLServerOptions<
plugins?: ApolloServerPlugin[];
documentStore?: InMemoryLRUCache<DocumentNode>;
parseOptions?: ParseOptions;
__testing_nodeEnv__?: string | undefined;
nodeEnv?: string;
}

export type DataSources<TContext> = {
Expand Down
15 changes: 7 additions & 8 deletions packages/apollo-server-core/src/runHttpQuery.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,11 +114,13 @@ export function throwHttpGraphQLError<E extends Error>(
);
}

const NODE_ENV = process.env.NODE_ENV ?? '';

export async function runHttpQuery(
handlerArguments: Array<any>,
request: HttpQueryRequest,
Comment thread
moberegger marked this conversation as resolved.
): Promise<HttpQueryResponse> {
function debugFromNodeEnv(nodeEnv: string | undefined) {
function debugFromNodeEnv(nodeEnv: string = NODE_ENV) {
return nodeEnv !== 'production' && nodeEnv !== 'test';
}

Expand All @@ -129,18 +131,15 @@ export async function runHttpQuery(
// The options can be generated asynchronously, so we don't have access to
// the normal options provided by the user, such as: formatError,
// debug. Therefore, we need to do some unnatural things, such
// as use NODE_ENV to determine the debug settings
// as use NODE_ENV to determine the debug settings. Please note that this
// will not be sensitive to any runtime changes made to NODE_ENV.
return throwHttpGraphQLError(500, [e as Error], {
debug: debugFromNodeEnv(process.env.NODE_ENV),
debug: debugFromNodeEnv(),
});
}

if (options.debug === undefined) {
const nodeEnv =
'__testing_nodeEnv__' in options
? options.__testing_nodeEnv__
: process.env.NODE_ENV;
options.debug = debugFromNodeEnv(nodeEnv);
options.debug = debugFromNodeEnv(options.nodeEnv);
Comment thread
moberegger marked this conversation as resolved.
}
Comment on lines 136 to 143

@moberegger moberegger Aug 26, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one also looks like it would happen in production environments, where debug would likely be undefined.

@glasser glasser Aug 27, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I would like to be sensitive to runtime NODE_ENV changes. We actually already capture NODE_ENV once in the ApolloServer constructor and pass that through to various ways. Unfortunately for historical reasons much of the core processing in this package happens technically outside of the ApolloServer class even though it is only usable when called from ApolloServer, so this file doesn't have access to that.

That said, I basically implemented this already but just as a "testing" hack. What I'd suggest is taking the __testing_nodeEnv__ fields of Config and GraphQLServerOptions and renaming them to nodeEnv. Then in the place in packages/apollo-server-core/src/ApolloServer.ts where we do const nodeEnv = we should set that on this.requestOptions.nodeEnv instead. That should lead to options.nodeEnv on this line in runHttpQuery being equal to the env var's value (or a manual override) already, so no need to read the env var in this file.

Is this something you can try?

@moberegger moberegger Sep 1, 2021 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've spent a few days trying to do this, and I'm not sure what to do.

The logic differs on the existence of nodeEnv, not necessarily its value. More specifically, providing nodeEnv: undefined is not the same thing as omitting it altogether.

What I would expect to happen is that if you don't provide nodeEnv, ApolloServer would default to the NODE_ENV environment variable. In other words, if nodeEnv is undefined, then default to NODE_ENV.

This doesn't work with the current implementation, at least as per the tests. The current implementation will simply check if you attempt to set it, even if it's value is undefined; as long as it's there, it's considered an override.

So what is happening is that the tests fail, because when nodeEnv is not provided, it defaults to NODE_ENV, and when NODE_ENV is test, debug is false, and you don't get stacktraces back in errors.

The old implementation would avoid defaulting to test because __testing_nodeEnv__ was simply there, even though it was undefined. The test cases look like they are attempting to validate behaviour when no node environment is set.

I'm not sure how to accommodate this.

@moberegger moberegger Sep 1, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That being said, I don't think it's possible to get this to not refer NODE_ENV in some way anyways, because resolveGraphqlOptions is what is supposed to give you the nodeEnv option, but that can error out and put you in a state where you need to know the node environment, but have no ability to get nodeEnv, and thus have to default to NODE_ENV anyways.

This would also introduce a quirk that even if you provided nodeEnv, it will be ignored depending on how the thing errors out, which sounds like a foot gun to me.

@glasser glasser Sep 29, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When __testing_nodeEnv__ was only being used in a few tests, making undefined distinct from "unset" seemed like a reasonable choice. It sounds like this is more of a burden when it's an actual part of the API.

How about we just change that? So we could make nodeEnv: undefined and "nodeEnv unset" be the same thing (use the actual environment variable if it's set), and we can use nodeEnv: '' to mean "seriously, don't be sensitive to the actual process environment, act as if the node env is unspecified". (Technically in the Unix model, there is a distinction between unset environment variables and environment variables that are set to the empty string, but it's not great to treat them differently and our code that reads this doesn't, so this should be fine.)

Does that resolve all the concerns you have here?

@moberegger moberegger Sep 30, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it!


// TODO: Errors thrown while resolving the context in
Expand Down
9 changes: 1 addition & 8 deletions packages/apollo-server-core/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,12 +99,5 @@ export interface Config<ContextFunctionParams = any> extends BaseConfig {
experimental_approximateDocumentStoreMiB?: number;
stopOnTerminationSignals?: boolean;
apollo?: ApolloConfigInput;
// Apollo Server only uses process.env.NODE_ENV to determine defaults for
// other behavior which have other mechanisms of setting explicitly. Sometimes
// our tests want to test the exact logic of how NODE_ENV affects defaults;
// they can set this parameter, but there's no reason to do so other than for
// tests. Note that an explicit `__testing_nodeEnv__: undefined` means "act as
// if the environment variable is not set", whereas the absence of
// `__testing_nodeEnv__` means to honor the environment variable.
Comment on lines -102 to -108

@moberegger moberegger Aug 30, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't think that this comment was necessary anymore, since I suppose you can consider nodeEnv a first class option now.

__testing_nodeEnv__?: string | undefined;
nodeEnv?: string;
}
10 changes: 5 additions & 5 deletions packages/apollo-server-express/src/__tests__/ApolloServer.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ describe('apollo-server-express', () => {
const { httpServer } = await createServer({
typeDefs,
resolvers,
__testing_nodeEnv__: undefined, // default landing page
nodeEnv: '', // default landing page
});

await request(httpServer)
Expand Down Expand Up @@ -265,7 +265,7 @@ describe('apollo-server-express', () => {
throw new AuthenticationError('valid result');
},
// Stack trace not included for NODE_ENV=test
__testing_nodeEnv__: undefined,
nodeEnv: '',
});

const apolloFetch = createApolloFetch({ uri });
Expand Down Expand Up @@ -296,7 +296,7 @@ describe('apollo-server-express', () => {
},
},
// Stack trace not included for NODE_ENV=test
__testing_nodeEnv__: undefined,
nodeEnv: '',
});

const apolloFetch = createApolloFetch({ uri });
Expand Down Expand Up @@ -326,7 +326,7 @@ describe('apollo-server-express', () => {
},
},
},
__testing_nodeEnv__: 'production',
nodeEnv: 'production',
});

const apolloFetch = createApolloFetch({ uri });
Expand Down Expand Up @@ -355,7 +355,7 @@ describe('apollo-server-express', () => {
},
},
},
__testing_nodeEnv__: 'production',
nodeEnv: 'production',
});

const apolloFetch = createApolloFetch({ uri });
Expand Down
10 changes: 5 additions & 5 deletions packages/apollo-server-fastify/src/__tests__/ApolloServer.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ describe('apollo-server-fastify', () => {
const { httpServer } = await createServer({
typeDefs,
resolvers,
__testing_nodeEnv__: undefined, // default landing page
nodeEnv: '', // default landing page
});

await request(httpServer)
Expand Down Expand Up @@ -283,7 +283,7 @@ describe('apollo-server-fastify', () => {
throw new AuthenticationError('valid result');
},
// Stack trace not included for NODE_ENV=test
__testing_nodeEnv__: undefined,
nodeEnv: '',
});

const apolloFetch = createApolloFetch({ uri });
Expand Down Expand Up @@ -314,7 +314,7 @@ describe('apollo-server-fastify', () => {
},
},
// Stack trace not included for NODE_ENV=test
__testing_nodeEnv__: undefined,
nodeEnv: '',
});

const apolloFetch = createApolloFetch({ uri });
Expand Down Expand Up @@ -344,7 +344,7 @@ describe('apollo-server-fastify', () => {
},
},
},
__testing_nodeEnv__: 'production',
nodeEnv: 'production',
});

const apolloFetch = createApolloFetch({ uri });
Expand Down Expand Up @@ -373,7 +373,7 @@ describe('apollo-server-fastify', () => {
},
},
},
__testing_nodeEnv__: 'production',
nodeEnv: 'production',
});

const apolloFetch = createApolloFetch({ uri });
Expand Down
10 changes: 5 additions & 5 deletions packages/apollo-server-hapi/src/__tests__/ApolloServer.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ describe('non-integration tests', () => {
const { httpServer } = await createServer({
typeDefs,
resolvers,
__testing_nodeEnv__: undefined, // default landing page
nodeEnv: '', // default landing page
});

await request(httpServer)
Expand Down Expand Up @@ -288,7 +288,7 @@ describe('non-integration tests', () => {
throw new AuthenticationError('valid result');
},
// Stack trace not included for NODE_ENV=test
__testing_nodeEnv__: undefined,
nodeEnv: '',
});

const apolloFetch = createApolloFetch({ uri });
Expand Down Expand Up @@ -319,7 +319,7 @@ describe('non-integration tests', () => {
},
},
// Stack trace not included for NODE_ENV=test
__testing_nodeEnv__: undefined,
nodeEnv: '',
});

const apolloFetch = createApolloFetch({ uri });
Expand Down Expand Up @@ -349,7 +349,7 @@ describe('non-integration tests', () => {
},
},
},
__testing_nodeEnv__: 'production',
nodeEnv: 'production',
});

const apolloFetch = createApolloFetch({ uri });
Expand Down Expand Up @@ -378,7 +378,7 @@ describe('non-integration tests', () => {
},
},
},
__testing_nodeEnv__: 'production',
nodeEnv: 'production',
});

const apolloFetch = createApolloFetch({ uri });
Expand Down
18 changes: 9 additions & 9 deletions packages/apollo-server-integration-testsuite/src/ApolloServer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ export function testApolloServer<AS extends ApolloServerBase>(
const { url: uri } = await createApolloServer({
schema,
stopOnTerminationSignals: false,
__testing_nodeEnv__: undefined,
nodeEnv: '',
});

const apolloFetch = createApolloFetch({ uri });
Expand All @@ -262,7 +262,7 @@ export function testApolloServer<AS extends ApolloServerBase>(
const { url: uri } = await createApolloServer({
schema,
stopOnTerminationSignals: false,
__testing_nodeEnv__: 'production',
nodeEnv: 'production',
});

const apolloFetch = createApolloFetch({ uri });
Expand All @@ -281,7 +281,7 @@ export function testApolloServer<AS extends ApolloServerBase>(
schema,
introspection: true,
stopOnTerminationSignals: false,
__testing_nodeEnv__: 'production',
nodeEnv: 'production',
});

const apolloFetch = createApolloFetch({ uri });
Expand Down Expand Up @@ -979,7 +979,7 @@ export function testApolloServer<AS extends ApolloServerBase>(
],
debug: true,
stopOnTerminationSignals: false,
__testing_nodeEnv__: undefined,
nodeEnv: '',
...constructorOptions,
});

Expand Down Expand Up @@ -1595,7 +1595,7 @@ export function testApolloServer<AS extends ApolloServerBase>(
typeDefs,
resolvers,
stopOnTerminationSignals: false,
__testing_nodeEnv__: undefined,
nodeEnv: '',
context: () => {
throw new AuthenticationError('valid result');
},
Expand Down Expand Up @@ -1653,7 +1653,7 @@ export function testApolloServer<AS extends ApolloServerBase>(
},
},
stopOnTerminationSignals: false,
__testing_nodeEnv__: 'production',
nodeEnv: 'production',
});

const apolloFetch = createApolloFetch({ uri });
Expand Down Expand Up @@ -1683,7 +1683,7 @@ export function testApolloServer<AS extends ApolloServerBase>(
},
},
stopOnTerminationSignals: false,
__testing_nodeEnv__: 'production',
nodeEnv: 'production',
});

const apolloFetch = createApolloFetch({ uri });
Expand Down Expand Up @@ -1714,7 +1714,7 @@ export function testApolloServer<AS extends ApolloServerBase>(
},
},
stopOnTerminationSignals: false,
__testing_nodeEnv__: 'development',
nodeEnv: 'development',
});

const apolloFetch = createApolloFetch({ uri });
Expand Down Expand Up @@ -2922,7 +2922,7 @@ export function testApolloServer<AS extends ApolloServerBase>(
})),
],
// dev mode, so we get the local landing page
__testing_nodeEnv__: undefined,
nodeEnv: '',
};
}

Expand Down
10 changes: 5 additions & 5 deletions packages/apollo-server-koa/src/__tests__/ApolloServer.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ describe('apollo-server-koa', () => {
const { httpServer } = await createServer({
typeDefs,
resolvers,
__testing_nodeEnv__: undefined, // default landing page
nodeEnv: '', // default landing page
});

await request(httpServer)
Expand Down Expand Up @@ -254,7 +254,7 @@ describe('apollo-server-koa', () => {
throw new AuthenticationError('valid result');
},
// Stack trace not included for NODE_ENV=test
__testing_nodeEnv__: undefined,
nodeEnv: '',
});

const apolloFetch = createApolloFetch({ uri });
Expand Down Expand Up @@ -285,7 +285,7 @@ describe('apollo-server-koa', () => {
},
},
// Stack trace not included for NODE_ENV=test
__testing_nodeEnv__: undefined,
nodeEnv: '',
});

const apolloFetch = createApolloFetch({ uri });
Expand Down Expand Up @@ -315,7 +315,7 @@ describe('apollo-server-koa', () => {
},
},
},
__testing_nodeEnv__: 'production',
nodeEnv: 'production',
});

const apolloFetch = createApolloFetch({ uri });
Expand Down Expand Up @@ -344,7 +344,7 @@ describe('apollo-server-koa', () => {
},
},
},
__testing_nodeEnv__: 'production',
nodeEnv: 'production',
});

const apolloFetch = createApolloFetch({ uri });
Expand Down
Loading
⚡