这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@nicuveo
Copy link
Contributor

@nicuveo nicuveo commented Aug 31, 2020

Description

We recently found an oversight of selection sets: we don't check for duplicates. In the unlikely (but, as evidenced, not impossible) case where we don't prevent a conflict from arising, said conflict should be detected when we build the schema. And in turns out that in some degenerate cases, we would construct a schema without detecting the conflict (specifically: when the conflicting root fields would yield types with the exact same fields, that therefore can't be differentiated).

(See discussion with @codingkarthik on Slack for more info, the test case, and the context.)

Affected components

  • Server

Solution and Design

This is a very simple fix, that only fixes the problem for root fields. There are two other ways we could prevent this from happening:

  • make all combinators monadic and throw on invalid schema (bad, no)
  • make all combinators require a hashmap of field parsers, to delegate the responsibility of checking for duplicates to the callers (better, but perhaps overkill)

@netlify
Copy link

netlify bot commented Aug 31, 2020

Deploy preview for hasura-docs ready!

Built with commit fe0ca00

https://deploy-preview-5694--hasura-docs.netlify.app

@nicuveo nicuveo marked this pull request as ready for review September 1, 2020 09:26
@nicuveo nicuveo requested a review from lexi-lambda September 1, 2020 09:27
Copy link
Contributor

@abooij abooij left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @codingkarthik for adding tests!

Copy link
Contributor

@tirumaraiselvan tirumaraiselvan left a comment

Choose a reason for hiding this comment

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

no cl required

@kodiakhq kodiakhq bot merged commit f9aca45 into hasura:master Sep 4, 2020
@nicuveo nicuveo deleted the fix-schema-duplication branch September 9, 2020 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants