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

[utils] fix broken sofa-api schema #1928

Closed
wants to merge 1 commit into from

Conversation

TrySound
Copy link
Contributor

@TrySound TrySound commented Aug 19, 2020

I got a bug while using the new version of sofa-api.

Not nullable fields for some reason got unexpected suffix like
status -> statusUserStatusNonNull. And the app crashed
because non null field resolved to not existing one.

I tracked the issue to @graphql-tools/utils and tried to drop the code.
My app started working. Tests still passed and there is not comments
why this code is necessary.

TODO:

  • If this PR is a new feature, reference an issue where a consensus about the design was reached (not necessary for small changes)
  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass

I got a bug while using the new version of sofa-api.

Not nullable fields for some reason got unexpected suffix like
`status` -> `statusUserStatusNonNull`.

I tracked the issue to @graphql-util/utils and tried to drop the code.
My app started working. Tests still passed and there is not comments
why this code is necessary.
@TrySound
Copy link
Contributor Author

Maybe I'm as usual do not setup it properly. Could you point me if there is a problem here?

const resolvers = {
  Query: {
    async user(parent, args, context, info) {
      return delegateToSchema({
        schema,
        operation: 'query',
        fieldName: 'user',
        args,
        context,
        info,
      });
    }
  }
}

const executableSchema = addResolversToSchema({
  schema: loadSchemaSync('./src/schema.graphql', {
    loaders: [new GraphQLFileLoader()],
  }),
  resolvers,
  resolverValidationOptions: {
    requireResolversForResolveType: false,
  },
});

const openApi = OpenAPI({
  schema: executableSchema,
});

const sofaRoute = useSofa({
  schema: executableSchema,
  onRoute(info) {
    openApi.addRoute(info, {
      basePath: '/api',
    });
  },
  context({ req, res }) {
    return new Context({ req, res });
  },
  async execute({
    schema,
    source,
    contextValue,
    variableValues,
    operationName,
  }) {
    return graphql({
      schema,
      source,
      contextValue,
      variableValues,
      operationName,
    })
  },
})

app.use(sofaRoute)

@TrySound
Copy link
Contributor Author

cc @ardatan

@TrySound TrySound changed the title [utils] remove really weird piece of code [utils] fix broken sofa-api schema Aug 21, 2020
@TrySound
Copy link
Contributor Author

Hi @yaacovCR. Do you have an idea why this renaming happens?

@yaacovCR
Copy link
Collaborator

Unfortunately not!

Not too familiar with sofa portion of codebase!

@ardatan
Copy link
Owner

ardatan commented Aug 26, 2020

@TrySound Not sure why it breaks for you because it is added with an alias if field names don't match;
https://github.com/ardatan/graphql-tools/pull/1928/files#diff-ad590e16bdb416ac6c38dda18f0df6bcL483

As I remember, we added this workaround for the case like I explained below.
Let's say you have a schema like;

type Query {
  entity(id: ID): Entity
}
union Entity = Book | User | Comment
type Book {
  id: ID!
  name: String
}
type User {
  id: ID
  name: String
}
type Comment {
  id: ID
  name: String
}

And this operation(something like this code used to generate before) breaks against this schema;

{
 entity(id: 2){
   ... on Book {
    id
    name
 }
 ... on User {
    id
    name
 }
}

because types of id field are not the same in these two inline fragments. Our workaround for this issue is to add an alias to one of them;

 entity(id: 2){
   ... on Book {
    id
    name
 }
 ... on User {
    id: idNonNull
    name
 }
}

But it probably causes other bugs so if you share a minimal reproduction repo or CodeSandbox, we can help you better.

@TrySound
Copy link
Contributor Author

TrySound commented Aug 26, 2020

Here's minimal reproduction. The problem is that schema is imported from another one and it's not easy to rename such fields.
Anyway here I don't use unions for which this hack was added. I find union is very problematic piece of graphql.
https://codesandbox.io/s/serene-blackwell-0x4q9?file=/src/index.js

@TrySound
Copy link
Contributor Author

TrySound commented Aug 26, 2020

Wow, preview works! Thanks!

@TrySound TrySound closed this Aug 26, 2020
@ardatan
Copy link
Owner

ardatan commented Aug 26, 2020

Fixed in 6.1.0 in @graphql-tools/utils and this package has been updated in sofa 0.8.1

@TrySound
Copy link
Contributor Author

@ardatan I don't see release in github. Did you forget to push it?

@TrySound
Copy link
Contributor Author

graphql-tools is released very often. Though sofa-api does not reflect all those releases and my project end with duplicated graphql-tools dependencies. Maybe it's worth to add ^ or at least ~ in version range?

@ardatan
Copy link
Owner

ardatan commented Aug 26, 2020

It only uses one specific function from utils so I think using an exact version would keep it more stable

@TrySound
Copy link
Contributor Author

Still not release on github. You'll get conflict when renovate will merge another pull request.
https://github.com/Urigo/SOFA/releases

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants