Skip to content
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.

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

CDN cache-control headers #1138

Merged
merged 29 commits into from
Jun 21, 2018
Merged

CDN cache-control headers #1138

merged 29 commits into from
Jun 21, 2018

Conversation

evans
Copy link
Contributor

@evans evans commented Jun 5, 2018

In order to se the cache-control headers inside of GraphQL execution, we need to return Response Headers. Currently runHttpQuery manufactures them from scratch. It could instead accept in a responseInit object and append it. However, none of the integrations currently use this functionality and it could be added in later, so to keep things simple, this PR takes the most direct route.

@ghost ghost added the ⛲️ feature New addition or enhancement to existing solutions label Jun 5, 2018
Object.keys(responseInit.headers).forEach(key =>
response.header(key, responseInit.headers[key]),
);
response.header(
Copy link
Member

Choose a reason for hiding this comment

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

this wasn't needed before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The express variant contained this, so I thought that the hapi and koa variants should be at feature parity. The reason it's not inside of apollo-server-core is because of the cloudflare integration. We can move the calculation there and then decide how to do the Buffer length in a V8 environment later

ctx.set(key, responseInit.headers[key]),
);
ctx.set(
'Content-Length',
Copy link
Member

Choose a reason for hiding this comment

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

ditto

}

return JSON.stringify(responses);
return {
gqlResponse: JSON.stringify(responses),
Copy link
Member

Choose a reason for hiding this comment

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

I think this might be better as graphqlResponse — do we really use "gql" much in actual APIs outside of graphql-tag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Love it

@@ -288,6 +293,13 @@ export async function runHttpQuery(

const responses = await Promise.all(requests);

const responseInit: ResponseInit = {
status: 200,
Copy link
Member

Choose a reason for hiding this comment

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

Seems like this isn't getting consistently used in all variants (even express lacks it).

@@ -20,6 +20,11 @@ export interface HttpQueryRequest {
request: Pick<Request, 'url' | 'method' | 'headers'>;
}

export interface HttpQueryResponse {
gqlResponse: string;
responseInit: ResponseInit;
Copy link
Member

Choose a reason for hiding this comment

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

The typing here is a little funky, since ResponseInit.headers is a HeadersInit which allows various options (Headers object, array of arrays, dictionary object) but the consumers assume that it's the latter. I'm almost surprised this typechecks, though maybe TS lets you [] index anything? Not sure what to do about this — maybe just roll your own type, or at least comment on it you can assume that the headers field is a record?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, we'll roll our own for now

@evans evans force-pushed the server-2.0/response-construction branch from 18c1b61 to cf75381 Compare June 11, 2018 22:28
@evans evans changed the title Return response object in runHttpQuery Return response object in runHttpQuery + CDN cache-control headers Jun 12, 2018
@ghost ghost added the ⛲️ feature New addition or enhancement to existing solutions label Jun 12, 2018
@evans evans changed the title Return response object in runHttpQuery + CDN cache-control headers CDN cache-control headers Jun 12, 2018
@ghost ghost added the ⛲️ feature New addition or enhancement to existing solutions label Jun 12, 2018
@evans evans force-pushed the server-2.0/response-construction branch from 47a21d3 to 772d8a6 Compare June 12, 2018 01:57
@evans evans force-pushed the server-2.0/response-construction branch from fc85b04 to 6890156 Compare June 14, 2018 23:01
@evans evans added this to the Release 2.0 milestone Jun 15, 2018
export interface PersistedQueryCache {
set(key: string, data: string): Promise<any>;
get(key: string): Promise<string | null>;
}

export function calcualteCacheControlHeaders(
Copy link
Member

Choose a reason for hiding this comment

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

"calculate"

let maxAge = Number.MAX_VALUE;
let publicOrPrivate = 'public';

for (let i = 0; i < responses.length; i++) {
Copy link
Member

Choose a reason for hiding this comment

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

So the key thing in the engineproxy calculation of cacheControl is you need to put a hint on every top-level data piece in order for it to count. See the rootDataKey loop in parseCacheControl.

@@ -363,6 +363,11 @@ describe('apollo-server-express', () => {
});
describe('file uploads', () => {
it('enabled uploads', async () => {
//XXX This is currently a failing test for node 10
Copy link
Member

Choose a reason for hiding this comment

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

Would you like to pair on tracking this down? Node 10 is released so it seems like we should make this work.

let maxAge = Number.MAX_VALUE;
let publicOrPrivate = 'public';

//Because of the early exit, we are unable to use forEach. While a reduce
Copy link
Member

Choose a reason for hiding this comment

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

I am hesitant to mention any formatting issues that prettier doesn't deal with automatically, but I've been meaning to mention that your style of not putting a space after // seems to be idiosyncratic in our codebases.

}

const rootHints = new Set<string>();
for (let y = 0; y < cacheControl.hints.length; y++) {
Copy link
Member

Choose a reason for hiding this comment

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

grab cacheControl.hints[y] immediately like you did in the outer loop? (Also a little odd that the loop indices are i/y.)


const rootHints = new Set<string>();
for (let y = 0; y < cacheControl.hints.length; y++) {
if (cacheControl.hints[y].scope === 'PRIVATE') {
Copy link
Member

Choose a reason for hiding this comment

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

In Engineproxy we do this comparison case-insensitively.

maxAge = cacheControl.hints[y].maxAge;
}

rootHints.add(cacheControl.hints[y].path[0]);
Copy link
Member

Choose a reason for hiding this comment

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

This is not quite right (or at least, not quite what we did before). This would mean that if you got a hint for "foo.bar" but nothing else, we would count this as being a hint on "foo". But if there's also a "foo.baz" and that was the only hint, then there are no hints on "foo.baz", so it wouldn't be right to count this as covering everything.

(Arguably we could change the "all roots must have hints" logic to be something more like "every leaf must be the descendent of a node with a hint" although that wouldn't be quite right either, as even interior nodes have resolvers whose return values can change.)

//we always want cacheControl to either set the CDN headers or for the
//engine proxy
cacheControl:
optionsObject.cacheControl !== null &&
Copy link
Member

Choose a reason for hiding this comment

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

hmm, why not just optionsObject.cacheControl === undefined ? true : optionsObject.cacheControl? Looks like this will override explicitly cacheControl: false which seems wrong.

if (!optionsObject.cacheControl) {
responseInit.headers = {
...responseInit.headers,
...calculateCacheControlHeaders(responses),
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we do this even when you explicitly ask for cacheControl? Perhaps there is a second option to cacheControl, eg cacheControl.setHTTPHeaders, which default to true.

...calculateCacheControlHeaders(responses),
};

//remove cacheControl headers. This could be done in production only,
Copy link
Member

Choose a reason for hiding this comment

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

It's extensions, not headers (so fix the comment).

However this all seems to be weirdly intricate. We create the CacheControlExtension object and ask it to put things in the JSON extension format, but then we turn it into headers and delete the extension. Why not just put header calculation (or perhaps, "max age and privacy tuple" calculation) into CacheControlExtension itself, and (via an option to the CacheControlExtension constructor that can be overridden) make format into a no-op?

evans added 6 commits June 18, 2018 16:25
Adds cache-control toggles for http header calculation and stripping out
the cache control extensions from the respose.

Brings the default calculation of headers in line with the proxy.
With ENGINE_API_KEY set as an environment variable, Apollo Server creates a reporting agent that sends execution traces to the Engine UI. In addition by default, Apollo Server supports [persisted queries](./features/apq.html).

<!-- FIXME add something about CDN headers-->
Apollo Server 2 is able to completely replace the Engine Proxy. To enable metrics reporting, add `ENGINE_API_KEY` as an environment variable. Apollo Server will then create a reporting agent that sends execution traces to the Engine UI. In addition by default, Apollo Server supports [persisted queries](./features/apq.html) without needing the proxy's cache. Apollo Server also provides cache-control headers for consumption by a [CDN](./features/cdn.html). Integration with a CDN provides a replacement for the full response caching in Engine Proxy.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that last sentence really is true. After all that was already a feature of the Proxy.

@@ -24,9 +22,9 @@ server.listen().then(({ url }) => {
});
```

## Starting Engine Proxy
## Starting Engine Proxy as a Sidecar
Copy link
Member

Choose a reason for hiding this comment

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

I think we moved away from the "sidecar" terminology because it never made sense to anybody

@@ -320,7 +364,9 @@ export async function runHttpQuery(
fieldResolver: optionsObject.fieldResolver,
debug: optionsObject.debug,
tracing: optionsObject.tracing,
cacheControl: optionsObject.cacheControl,
cacheControl: cacheControl
? { defaultMaxAge: cacheControl.defaultMaxAge }
Copy link
Member

Choose a reason for hiding this comment

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

Is the implication that this is the only parameter that the underlying package supports? maybe use _.omit or whatever to blacklist instead of whitelist?

@evans evans merged commit 65d7b10 into version-2 Jun 21, 2018
@evans evans deleted the server-2.0/response-construction branch June 21, 2018 20:29
@evans evans mentioned this pull request Jun 21, 2018
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
⛲️ feature New addition or enhancement to existing solutions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants