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

OneOf validation rule location suggestions #1

Open
wants to merge 1 commit into
base: oneof-v2
Choose a base branch
from

Conversation

yaacovCR
Copy link

Existing "Values of Correct Type" covers most OneOf specifics

We don't need a specific rule to validate that the values for OneOf Input Objects coerce properly => That is covered by the "Values of Correct Type" Rule which states that

https://spec.graphql.org/draft/#sel-HALXDDDBCAAEDDDBlkc

  • For each input Value value in the document:
    • Let type be the type expected in the position value is found.
    • value must be coercible to type.

Because the OneOf PR has detailed coercion rules for OneOf, everything should be covered there, and the new rule does not seem necessary.

Allowed Variable Positions are Changed with the Introduction of OneOf

We do need to update the positions within which variables are allowed, because non-null variables are not allowed in OneOf Input Object field positions. [For simplicity, this PR retains the same problems for variables with defaults that are fixed by Strict All Variable Usages Are Allowed]. This is by design to better demonstrate that this is where these rule updates belong, and could be changed as desired.

…sitions

for simplicity, this PR retains the same problems for variables with defaults that are fixed by strict All Variable Usages Are Allowed
@benjie
Copy link
Owner

benjie commented Sep 23, 2024

We don't need a specific rule to validate that the values for OneOf Input Objects coerce properly => That is covered by the "Values of Correct Type" Rule which states that

I'm not sure that that is sufficiently thorough; for example:

      - Let {variableType} be the expected type of {variableDefinition}.
      - {variableType} must be a non-null type.

I don't see this required by the value coercion that is able to run at validation time? (Update: ah; I see you've handled this separately.)

From a quick skim I'm not even sure that "Values of Correct Type" is safe to use in this way, because we do not have variable values at validation time, so we cannot fully coerce a value. The coercion tables rely on runtime variable values, but they don't exist yet... I fear the "Values of Correct Type" rule might be under-specified for this purpose, maybe that rule needs revisiting in general!

@benjie
Copy link
Owner

benjie commented Sep 23, 2024

Do you want to bring this to the next WG? I'd love to get Lee's opinion on it.

@yaacovCR
Copy link
Author

I fear the "Values of Correct Type" rule might be under-specified for this purpose, maybe that rule needs revisiting in general!

Could be! Across the spec, for all values, The "Values of Correct Type" rules says that values are of the correct type if they coerce. In terms of variables, we make two implicit stipulations, (1) we do not examine their values, because they have not and cannot be supplied when validating statically, and (2) we assume that they are in allowed positions, presumably that is covered by the separate "Variables are in Allowed Positions". These two assumptions seem pretty fair, but presumably they could be made more explicit.

Do you agree though that OneOf should not be an exception to this?

Do you want to bring this to the next WG? I'd love to get Lee's opinion on it.

Checking the calendar, it looks like I will not be able to make the next WG meeting due to coincidence with one of the fall Jewish holidays. I do not think it makes sense to postpone discussion on this to a secondary meeting. I think this PR could be a reference for what I would have said if I was there. :)

@yaacovCR
Copy link
Author

Cross-posting graphql#1113

@yaacovCR
Copy link
Author

We have progressed within the default value coercion PR stack to graphql/graphql-js#3813 which relies on this proposed spec edit with respect to VariablesInAllowedPositionRule and its corresponding implementation at graphql/graphql-js#4194.

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.

2 participants