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.

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

Jump to bottom

fix: updated documentstore to use schema hash #7433

Merged
merged 4 commits into from
Apr 3, 2023
Merged

fix: updated documentstore to use schema hash #7433

merged 4 commits into from
Apr 3, 2023

Conversation

KGAdamCook
Copy link
Contributor

@KGAdamCook KGAdamCook commented Mar 9, 2023 •
edited by trevor-scheer

If you have multiple servers running with the same schema, you may wish to share the documentStore cache among them using something such as Redis, rather than inMemory.

Using the schema hash this should allow for sharing of the data whilst still creating a new prefix for whenever the schema changes.

Fixes #7471

@netlify
Copy link

netlify bot commented Mar 9, 2023 •
edited

👷 Deploy request for apollo-server-docs pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit df47598

@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 9, 2023 •
edited

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit df47598:

Sandbox Source
Apollo Server Typescript Configuration
Apollo Server Configuration

@trevor-scheer
Copy link
Member

@KGAdamCook in principle I think this change is reasonable.

I'd be remiss not to steer you away from what you're interested in doing though. The documentStore stores parsed AST in memory, which allows Apollo Server to skip parsing and validation of the document altogether. By moving the AST out of memory, you're going to incur the deserialization (JSON parsing?) cost every time you fetch from your redis document store.

@KGAdamCook
Copy link
Contributor Author

Thanks for your reply @trevor-scheer , so there is no actual benefit to having the documentStore be anything other than inMemory?

@trevor-scheer
Copy link
Member

Maybe a benefit for a new instance with a query it hasn't seen before (it would be JSON parsing the AST from your distributed redis cache instead of parsing/validating). After that query is seen/cached in memory though, the distributed cache is a performance hit every time since it has to deserialize on every request. Does that make sense?

@KGAdamCook
Copy link
Contributor Author

Yeah that does make sense. In our particular use case we are hosting Apollo Server on AWS Lambda, so we see a large amount of new servers each day.

It almost seems like to me it would benefit greatly from having the cache default to memory if it exists, otherwise also check if it exists in Redis. Then only pulling from memory on subsequent queries.

trevor-scheer
trevor-scheer approved these changes Mar 22, 2023
View reviewed changes
Copy link
Member

@trevor-scheer trevor-scheer left a comment

Choose a reason for hiding this comment

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

No objections, just a couple tweaks in wording.

packages/server/src/ApolloServer.ts Show resolved Hide resolved
.changeset/wet-berries-report.md Outdated Show resolved Hide resolved
@trevor-scheer
Copy link
Member

@KGAdamCook sorry there's usually a comment from our bot to request the CLA. Can you please sign it?
https://contribute.apollographql.com/ (via the last failing status check)

Otherwise ready to go 👍

@trevor-scheer trevor-scheer mentioned this pull request Mar 27, 2023
Closed
@trevor-scheer
Copy link
Member

@KGAdamCook bump! I'd like to get this into the next release if possible.

@KGAdamCook
Copy link
Contributor Author

Apologies @trevor-scheer , I have signed the CLA and resolved the comments you raised.
Was there anything else? :)

@trevor-scheer
Copy link
Member

Our CLA bot was having a fun time around when this PR was opened or pushed to last. Just noting that the status still says "waiting" but I've confirmed in the database that you've signed it. Thanks!

@trevor-scheer trevor-scheer merged commit e0db95b into apollographql:main Apr 3, 2023
2 checks passed
@github-actions github-actions bot mentioned this pull request Apr 3, 2023
Merged
trevor-scheer pushed a commit that referenced this pull request Apr 3, 2023
@github-actions
Version Packages (#7473)
05b2da4 
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and the packages will
be published to npm automatically. If you're not ready to do a release
yet, that's fine, whenever you add more changesets to main, this PR will
be updated.


# Releases
## @apollo/server@4.6.0

### Minor Changes

- [#7465](#7465)
[`1e808146a`](1e80814)
Thanks [@trevor-scheer](https://github.com/trevor-scheer)! - Introduce
new opt-in configuration option to mitigate v4 status code regression

Apollo Server v4 accidentally started responding to requests with an
invalid `variables` object with a 200 status code, where v3 previously
responded with a 400. In order to not break current behavior
(potentially breaking users who have creatively worked around this
issue) and offer a mitigation, we've added the following configuration
option which we recommend for all users.

    ```ts
    new ApolloServer({
      // ...
      status400ForVariableCoercionErrors: true,
    });
    ```

Specifically, this regression affects cases where _input variable
coercion_ fails. Variables of an incorrect type (i.e. `String` instead
of `Int`) or unexpectedly `null` are examples that fail variable
coercion. Additionally, missing or incorrect fields on input objects as
well as custom scalars that throw during validation will also fail
variable coercion. For more specifics on variable coercion, see the
"Input Coercion" sections in the [GraphQL
spec](https://spec.graphql.org/June2018/#sec-Scalars).

This will become the default behavior in Apollo Server v5 and the
configuration option will be ignored / no longer needed.

### Patch Changes

- [#7454](#7454)
[`f6e3ae021`](f6e3ae0)
Thanks [@trevor-scheer](https://github.com/trevor-scheer)! - Start
building packages with TS 5.x, which should have no effect for users

- [#7433](#7433)
[`e0db95b96`](e0db95b)
Thanks [@KGAdamCook](https://github.com/KGAdamCook)! - Previously, when
users provided their own `documentStore`, Apollo Server used a random
prefix per schema in order to guarantee there was no shared state from
one schema to the next. Now Apollo Server uses a hash of the schema,
which enables the provided document store to be shared if you choose to
do so.

## @apollo/server-integration-testsuite@4.6.0

### Patch Changes

- [#7454](#7454)
[`f6e3ae021`](f6e3ae0)
Thanks [@trevor-scheer](https://github.com/trevor-scheer)! - Start
building packages with TS 5.x, which should have no effect for users

- Updated dependencies
\[[`1e808146a`](1e80814),
[`f6e3ae021`](f6e3ae0),
[`e0db95b96`](e0db95b)]:
    -   @apollo/server@4.6.0

## @apollo/server-plugin-response-cache@4.1.2

### Patch Changes

- [#7454](#7454)
[`f6e3ae021`](f6e3ae0)
Thanks [@trevor-scheer](https://github.com/trevor-scheer)! - Start
building packages with TS 5.x, which should have no effect for users

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@KGAdamCook KGAdamCook deleted the fix-document-store-prefix branch April 4, 2023 09:31
trevor-scheer pushed a commit that referenced this pull request Apr 17, 2023
@KGAdamCook @trevor-scheer
fix: updated documentstore to use schema hash (#7433)
d8f2fc6 
Previously, when users provided their own `documentStore`, Apollo Server
used a random prefix per schema in order to guarantee there was no
shared state from one schema to the next. Now Apollo Server uses a hash
of the schema, which enables the provided document store to be shared if
you choose to do so.
trevor-scheer pushed a commit that referenced this pull request Apr 17, 2023
@github-actions @trevor-scheer
Version Packages (#7473)
f72dd10 
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and the packages will
be published to npm automatically. If you're not ready to do a release
yet, that's fine, whenever you add more changesets to main, this PR will
be updated.


# Releases
## @apollo/server@4.6.0

### Minor Changes

- [#7465](#7465)
[`1e808146a`](1e80814)
Thanks [@trevor-scheer](https://github.com/trevor-scheer)! - Introduce
new opt-in configuration option to mitigate v4 status code regression

Apollo Server v4 accidentally started responding to requests with an
invalid `variables` object with a 200 status code, where v3 previously
responded with a 400. In order to not break current behavior
(potentially breaking users who have creatively worked around this
issue) and offer a mitigation, we've added the following configuration
option which we recommend for all users.

    ```ts
    new ApolloServer({
      // ...
      status400ForVariableCoercionErrors: true,
    });
    ```

Specifically, this regression affects cases where _input variable
coercion_ fails. Variables of an incorrect type (i.e. `String` instead
of `Int`) or unexpectedly `null` are examples that fail variable
coercion. Additionally, missing or incorrect fields on input objects as
well as custom scalars that throw during validation will also fail
variable coercion. For more specifics on variable coercion, see the
"Input Coercion" sections in the [GraphQL
spec](https://spec.graphql.org/June2018/#sec-Scalars).

This will become the default behavior in Apollo Server v5 and the
configuration option will be ignored / no longer needed.

### Patch Changes

- [#7454](#7454)
[`f6e3ae021`](f6e3ae0)
Thanks [@trevor-scheer](https://github.com/trevor-scheer)! - Start
building packages with TS 5.x, which should have no effect for users

- [#7433](#7433)
[`e0db95b96`](e0db95b)
Thanks [@KGAdamCook](https://github.com/KGAdamCook)! - Previously, when
users provided their own `documentStore`, Apollo Server used a random
prefix per schema in order to guarantee there was no shared state from
one schema to the next. Now Apollo Server uses a hash of the schema,
which enables the provided document store to be shared if you choose to
do so.

## @apollo/server-integration-testsuite@4.6.0

### Patch Changes

- [#7454](#7454)
[`f6e3ae021`](f6e3ae0)
Thanks [@trevor-scheer](https://github.com/trevor-scheer)! - Start
building packages with TS 5.x, which should have no effect for users

- Updated dependencies
\[[`1e808146a`](1e80814),
[`f6e3ae021`](f6e3ae0),
[`e0db95b96`](e0db95b)]:
    -   @apollo/server@4.6.0

## @apollo/server-plugin-response-cache@4.1.2

### Patch Changes

- [#7454](#7454)
[`f6e3ae021`](f6e3ae0)
Thanks [@trevor-scheer](https://github.com/trevor-scheer)! - Start
building packages with TS 5.x, which should have no effect for users

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@trevor-scheer trevor-scheer trevor-scheer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Errors using shared documentStore with Redis KeyvAdapter
2 participants
@KGAdamCook @trevor-scheer