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

[Bug?]: v6 doesn't allow for a Subscription sdl type. #8988

Closed
1 task
noire-munich opened this issue Jul 28, 2023 · 4 comments
Closed
1 task

[Bug?]: v6 doesn't allow for a Subscription sdl type. #8988

noire-munich opened this issue Jul 28, 2023 · 4 comments
Assignees
Labels
bug/confirmed We have confirmed this is a bug

Comments

@noire-munich
Copy link
Collaborator

noire-munich commented Jul 28, 2023

What's not working?

What's happening

Going through the normal process to upgrade to v6-rc, here's what I got when running yarn rw dev, an issue with my sdl types:

af1e8869f08b7a24f4d99de282a4bc4a09754c96

As mentioned here: https://community.redwoodjs.com/t/redwood-v6-0-0-upgrade-guide/5044/33?u=noire.munich.

I've tried with other types, here's what I got:
image

Interestingly, the Client type isn't failing.

What's expected

I'm not a fan of having a functionaly meaningful term being reserved so ideally, a Subscription type should not be a problem. If this is not possible, a warning once the upgrade is complete would do nicely in informing of the reserved names being used in sdl types, maybe with a link to some documentation explaining why it's better to keep those terms reserved.

How do we reproduce the bug?

All you need is to export such an sdl, only the names of the types matter, not the fields they have:

export const schema = gql`
  type Mutation {
    id: Int!
    label: String!
    i18nKey: String!
    slug: String!
    status: SubscriptionStatus!
    description: String!
    stripe_id: String!
    base_amount: Int!
    accounts: [Account]!
  }
  type Query {
    id: Int!
    label: String!
    i18nKey: String!
    slug: String!
    status: SubscriptionStatus!
    description: String!
    stripe_id: String!
    base_amount: Int!
    accounts: [Account]!
  }
  type Client {
    id: Int!
    label: String!
    i18nKey: String!
    slug: String!
    status: SubscriptionStatus!
    description: String!
    stripe_id: String!
    base_amount: Int!
    accounts: [Account]!
  }
`

What's your environment? (If it applies)

No response

Are you interested in working on this?

  • I'm interested in working on this
@noire-munich noire-munich added the bug/needs-info More information is needed for reproduction label Jul 28, 2023
@ketang
Copy link

ketang commented Jul 31, 2023

I have encountered the same.

@dthyresson dthyresson self-assigned this Aug 3, 2023
@dthyresson dthyresson added bug/confirmed We have confirmed this is a bug and removed bug/needs-info More information is needed for reproduction labels Aug 3, 2023
@dthyresson
Copy link
Contributor

dthyresson commented Aug 3, 2023

I believe that the following are reserved types:

The following names are reserved and can’t be used to define any other identifiers:

Int
Float
Boolean
String
DateTime
ID
uid
Subscription
as (case-insensitive)
Query
Mutation
Point
PointList
Polygon
MultiPolygon
Aggregate (as a suffix of any identifier name)

Therefore if you had a Prisma model and sdl type called Query, this would be a problem, too.

I think the solution isn't to make the type check more lenient, but instead discourage or warn that a model or type uses a reserved term.

For now, the only workaround I can think of is to rename the model and type from Subscription to maybe something like ProductSubscription etc.

I'll try to think of a better workaround, but that's the only option I have at the moment.

Or ... maybe I can detect if Redwood Realtime subscriptions are enabled and only check for that reserved term is used. That might be better?

Edit: Actually, you can need the Prisma model the same as Subscription, just change your SDL and resolver types to ProductSubscription or CustomerSubscription or MagazineSubscription or whatever is appropriate,

@dthyresson
Copy link
Contributor

I confirmed that it is possible to:

  • check for some reserved GraphQL types that shouldn't be used (like Float, Polygon, String etc)
  • only check directives of Subscription types if realtime is enabled.

In the latter case, if one uses graphQL Subscriptions you should rename your Type.

You don't have to rename your model, just the types and in your service, return the data for that type instead like 'ProductSubscription'

@dthyresson
Copy link
Contributor

@noire-munich have a WIP here #9005 with an explanation.

jtoar added a commit that referenced this issue Aug 8, 2023
…en not using Realtime and ensure schema does not use reserved names (#9005)

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.

2. 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.

---------

Co-authored-by: Dominic Saadi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug/confirmed We have confirmed this is a bug
Projects
None yet
Development

No branches or pull requests

3 participants