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: Improve GraphQL Schema Validation to allow Subscription types when not using Realtime and ensure schema does not use reserved names #9005

Merged
merged 10 commits into from
Aug 8, 2023
Merged

fix: Improve GraphQL Schema Validation to allow Subscription types when not using Realtime and ensure schema does not use reserved names #9005

merged 10 commits into from
Aug 8, 2023

Conversation

dthyresson
Copy link
Contributor

@dthyresson dthyresson commented Aug 3, 2023 •
edited
Loading

See: #8988

  1. In GraphQL there are many reserved names - Subscription being one of them.

And Redwood's schema validation checks that a validator directive is applied to queries, mutations -- and subscriptions.

However, we have cases where existing RW apps have used Subscription as a model and a type.

This PR will only check for the directive on Subscriptions if Redwood Realtime is enabled.

Note: a workaround is to keep your Prisma model named Subscription, but just rename the type to something like "CustomerSubscription" or "ProductSubscription" and rework your services to have resolver types that match your GraphQL schema.

  1. This PR also will raise errors when reserved names are uses as types ,inputs or interfaces -- like if for example you had a Prisma model and a generated type like "Float" because maybe you had a fishing or sailing store and needed to have a collection of floats :)

Note to @jtoar - this is a potentially breaking changes because some projects might be using reserved GraphQL names as types but really shouldn't to mitigate issues downstream.

@dthyresson dthyresson self-assigned this Aug 3, 2023
@dthyresson dthyresson mentioned this pull request Aug 3, 2023
Closed
1 task
@dthyresson dthyresson added the release:fix This PR is a fix label Aug 3, 2023
@dthyresson dthyresson added release:breaking This PR is a breaking change and removed release:fix This PR is a fix labels Aug 4, 2023
@dthyresson dthyresson added this to the v7.0.0 milestone Aug 4, 2023
@dthyresson dthyresson changed the title WIP: fix: Allow Subscription to bypass in directive check if Redwood Realtime not used fix: Improve GraphQL Schema Validation to allow Subscription types when not using Realtime and ensure schema does not use reserved names Aug 4, 2023
@dthyresson dthyresson marked this pull request as ready for review August 4, 2023 13:31
jtoar
jtoar reviewed Aug 8, 2023
View reviewed changes
Copy link
Contributor

@jtoar jtoar left a comment

Choose a reason for hiding this comment

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

@dthyresson what do you think about decoupling the don't-error-out-on-subscriptions part of this PR from the error-on-other-reserved-names part? We could ship the former right now as a patch and figure out when's the best time to ship the latter part.

packages/internal/src/validateSchema.ts Outdated Show resolved Hide resolved
packages/internal/src/validateSchema.ts Outdated Show resolved Hide resolved
packages/internal/src/validateSchema.ts Show resolved Hide resolved
@dthyresson dthyresson requested a review from jtoar August 8, 2023 12:55
@dthyresson
Copy link
Contributor Author

Made requested changes. Thanks for the review @jtoar !

@Josh-Walker-GM Josh-Walker-GM removed their request for review August 8, 2023 14:06
jtoar
jtoar approved these changes Aug 8, 2023
View reviewed changes
Copy link
Contributor

@jtoar jtoar left a comment

Choose a reason for hiding this comment

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

Looks good

@jtoar jtoar merged commit 0167be7 into redwoodjs:main Aug 8, 2023
jtoar added a commit that referenced this pull request Aug 9, 2023
@jtoar
cherry pick part of "fix: Improve GraphQL Schema Validation to allow …
efa86ef 
…Subscription types when not using Realtime and ensure schema does not use reserved names (#9005)"
@jtoar
Copy link
Contributor

jtoar commented Aug 9, 2023

@dthyresson I cherry picked the Subscription part of this into v6.0.4: https://github.com/redwoodjs/redwood/releases/tag/v6.0.4

@dthyresson Pullflow.com
Copy link
Contributor Author

Awesome thanks! I think I’ll have subs with auth tomorrow since it’s “working better” so can patch before you leave.

dac09 added a commit to dac09/redwood that referenced this pull request Aug 9, 2023
@dac09
Merge branch 'main' of github.com:redwoodjs/redwood into fix/router-s…
faaf446 
…croll-reset

* 'main' of github.com:redwoodjs/redwood:
  fix(deps): update dependency pino to v8.15.0 (redwoodjs#9023)
  fix(deps): update dependency eslint to v8.46.0 (redwoodjs#9022)
  fix(deps): update dependency react-hook-form to v7.45.4 (redwoodjs#9017)
  chore(docs): reversion docs to include recent changes
  fix(deps): update dependency vite to v4.4.9 (redwoodjs#9018)
  v6.0.4
  fix(docs): update quick start to fix Storybook start up (redwoodjs#9014)
  cherry pick part of "fix: Improve GraphQL Schema Validation to allow Subscription types when not using Realtime and ensure schema does not use reserved names (redwoodjs#9005)"
  fix: Improve GraphQL Schema Validation to allow Subscription types when not using Realtime and ensure schema does not use reserved names (redwoodjs#9005)
  Docs: Explain the entry.client.{jsx,tsx} file (redwoodjs#8987)
  chore(deps): update dependency esbuild to v0.18.19 (redwoodjs#8983)
  chore(deps): update dependency nx-cloud to v16.2.0 (redwoodjs#8985)
  docs(fonts): Update @font-face recommendation (redwoodjs#8986)
  Docs: remove useless code in code snippet (redwoodjs#8990)
  v6.0.3
  fix(router): Prevent rerendering authenticated routes on hash change (redwoodjs#9007)
  Remove the indexed type reference on AvailableRoutes (redwoodjs#8918)
dac09 added a commit to dac09/redwood that referenced this pull request Aug 9, 2023
@dac09
Merge branch 'main' of github.com:redwoodjs/redwood into try/apollo-s…
5204306 
…sr-stream

* 'main' of github.com:redwoodjs/redwood:
  feat(streaming-ssr): Fix build and server html injection (redwoodjs#8978)
  fix(router): Do not reset scroll on query & hash changes (redwoodjs#9004)
  fix(deps): update dependency pino to v8.15.0 (redwoodjs#9023)
  fix(deps): update dependency eslint to v8.46.0 (redwoodjs#9022)
  fix(deps): update dependency react-hook-form to v7.45.4 (redwoodjs#9017)
  chore(docs): reversion docs to include recent changes
  fix(deps): update dependency vite to v4.4.9 (redwoodjs#9018)
  v6.0.4
  fix(docs): update quick start to fix Storybook start up (redwoodjs#9014)
  cherry pick part of "fix: Improve GraphQL Schema Validation to allow Subscription types when not using Realtime and ensure schema does not use reserved names (redwoodjs#9005)"
  fix: Improve GraphQL Schema Validation to allow Subscription types when not using Realtime and ensure schema does not use reserved names (redwoodjs#9005)
  Docs: Explain the entry.client.{jsx,tsx} file (redwoodjs#8987)
  chore(deps): update dependency esbuild to v0.18.19 (redwoodjs#8983)
  chore(deps): update dependency nx-cloud to v16.2.0 (redwoodjs#8985)
jtoar added a commit that referenced this pull request Aug 12, 2023
@jtoar
Revert v7.0.0 and RSC changes
3d4ee77 
Revert "feat(streaming-ssr): Fix build and server html injection (#8978)"

This reverts commit 58a2421.

Revert "fix: Improve GraphQL Schema Validation to allow Subscription types when not using Realtime and ensure schema does not use reserved names (#9005)"

This reverts commit 0167be7.

Revert "RSC: Use experimental node loader (#8979)"

This reverts commit 2add568.

Revert "RSC: Fix noExternal rule for server build (#8961)"

This reverts commit 9ac5ab2.

Revert "@rwjs/vite: Pin acorn-loose version (#8944)"

This reverts commit 6f33914.

Revert "RSC: react-server condition. Poisoned imports (#8948)"

This reverts commit e941365.

Revert "RSC fix typo in example code (#8949)"

This reverts commit 3588ec0.

Revert "fix(ssr): Get experimental ssr setup working properly (#8922)"

This reverts commit abf229b.

Revert "RSC fix: setup description (#8906)"

This reverts commit b11bd72.

Revert "RSC: Add css files to the example (#8905)"

This reverts commit fcd7c39.

Revert "RSC build without user config (#8896)"

This reverts commit 37692a4.

Revert "Refactor cli serve command (#8958)"

This reverts commit f24016a.

Revert "RSC: Use rw serve (#8897)"

This reverts commit 921c9cb.

Revert "RSC: Build using rw build (#8893)"

This reverts commit f1d0dcb.

Revert "RSC: Fix experimental setup (#8894)"

This reverts commit c44a260.

Revert "RSC: Include entries.ts in paths (#8888)"

This reverts commit db271db.

Revert "RSC: Initial css support (#8887)"

This reverts commit 8610d58.

Revert "RSC: Use exported defineEntries() (#8886)"

This reverts commit f5fc2e2.

Revert "Include standard vite config in RSC build (#8882)"

This reverts commit c1e62c2.

Revert "Update entry.client and disable vite legacy mode (#8851)"

This reverts commit 8917ad6.

Revert "experimental feature flag for rsc (#8837)"

This reverts commit c7a5b13.

Revert "React Server Components (RSC) (#8451)"

This reverts commit f22dfbe.

Revert "Use "import type" for all types (#8827)"

This reverts commit 7a6aea7.

Revert "React 18.3.0-canary-035a41c4e-20230704 (#8826)"

This reverts commit d261819.

Revert "ViteBuildManifest and note about import-attributes (#8818)"

This reverts commit 8b85ad6.

Revert "Streaming SSR: Fix build, serve and dev (#8811)"

This reverts commit 4c51cae.

Revert "Add files needed for React Streaming SSR (#8810)"

This reverts commit 6da5aae.

Revert "entry.server and entry.client (#8808)"

This reverts commit 7cc5564.

Revert "Use existing rw-vite-build bin for SSR as well (#8806)"

This reverts commit 8b2a566.

Revert "Disable prerender when streamingSsr is enabled (#8775)"

This reverts commit e7d5361.

Revert "vite utils.ts: fix source format (#8796)"

This reverts commit e191071.

Revert "Server Rendering & Streaming (#8561)"

This reverts commit 2557bf8.

Revert "React Streaming and SSR feature flag (#8764)"

This reverts commit 565f620.
@jtoar jtoar modified the milestones: SSR, v7.0.0 Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtoar jtoar jtoar approved these changes

Assignees

@dthyresson dthyresson

Labels
release:breaking This PR is a breaking change
Projects
None yet
Milestone
v7.0.0
Development

Successfully merging this pull request may close these issues.
2 participants
@dthyresson @jtoar