-
Couldn't load subscription status.
- Fork 0
add isValueExpectedForType to handle abstract types #29
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
base: main
Are you sure you want to change the base?
Conversation
| } | ||
|
|
||
| // Parent is a concrete type - check if fixture value's __typename matches | ||
| const valueTypename = fixtureValue.__typename; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This only works if the __typename selection isn't aliased, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup. Did not realize people would actually do this but it is possible. 😢
| * Determines if a fixture value is expected for a given parent type based on its __typename. | ||
| * | ||
| * @param fixtureValue - The fixture value to check | ||
| * @param parentType - The parent type from typeInfo (concrete type if inside inline fragment, abstract if on union/interface) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
concrete type if inside inline fragment
This is not necessarily true. You can have inline fragments whose type condition is an abstract type. Example:
interface MyInterface {
myInterfaceField: String
}
type A implements MyInterface {
a: String
myInterfaceField: String
}
type B implements MyInterface {
b: String
myInterfaceField: String
}
type C {
c: String
}
union ABC = A | B | C
type Query {
abc: ABC
}{
abc {
...on MyInterface {
myInterfaceField
}
}
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a type to the schema to test this and updated the code to handle this by matching the schema's possibleTypes to the __typename value.
f61f2b1 to
d5ba41c
Compare
| ): boolean { | ||
| // If __typename wasn't selected in the query, we can't discriminate | ||
| if (!typenameKey) { | ||
| // Empty objects {} are valid if the grandparent type is a union/interface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Counterexample:
interface MyInterface {
myField: String
}
type A implements MyInterface {
myField: String
}
type Query {
myInterface: MyInterface
}{
myInterface {
...on myInterface {
myField
}
}
}In this case empty objects should not be allowed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the schema and added a test case for this.
| function isValueExpectedForType( | ||
| fixtureValue: any, | ||
| parentType: GraphQLCompositeType, | ||
| grandparentType: GraphQLNamedType | undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how I feel about this concept. Because you can have theoretically many nested fragments, I think it makes sense to keep track of the set of possible types as we descend the fragments, and compare that to the set of possible types of the parent type. If the sets are not equal then we can accept an empty object. If they are equal, we cannot and need to validate further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added logic in the latest commit to compare between the two sets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to say that the "grandparent type" alone isn't fully correct. There could be a number of types that we need to filter through. e.g.
{
myField {
...on Interface1 {
...on Interface2 {
...on Interface3 {
myOtherField
}
}
}
}
}In this case, we need to see how many possible types there are in the intersection of whatever type myField is as well as Interface1, Interface2, and Interface3.
| if (node.typeCondition) { | ||
| const namedType = schema.getType(node.typeCondition!.name.value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we changed it to node.typeCondition !== null && node.typeCondition !== undefined would it allow us to remove the !?
| const parentType = typeInfo.getParentType(); | ||
| if (!parentType) { | ||
| // This shouldn't happen with a valid query and schema - TypeInfo should always | ||
| // provide parent type information when traversing fields. This check is here to | ||
| // satisfy TypeScript's type requirements (getParentType() can return null). | ||
| errors.push(`Cannot validate ${responseKey}: missing parent type information`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this? I'm guessing we were using parentType before but now we're not?
| // If this SelectionSet has __typename, use its response key. | ||
| // Otherwise, inherit from parent. | ||
| const typenameResponseKey = typenameField && typenameField.kind === Kind.FIELD | ||
| ? typenameField.alias?.value || "__typename" | ||
| : typenameResponseKeyStack[typenameResponseKeyStack.length - 1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inheriting from the parent is only correct if the selection is part of an inline fragment, not part of a field. Is there a way we can differentiate? Maybe split this out to entering inline fragments and fields? Or I think all of the arguments to enter are actually node, key, parent, path, ancestors, so we could include those in the signature to get access to the parent to check what it is.
| id: ID! | ||
| } | ||
|
|
||
| type InterfaceImplementer4 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: this doesn't implement an interface, feels weird to call it an interface implementer
| expect(result.errors[1]).toBe("Missing expected fixture data for id"); | ||
| expect(result.errors[2]).toBe("Missing expected fixture data for count"); | ||
| expect(result.errors[3]).toBe("Missing expected fixture data for email"); | ||
| expect(result.errors[4]).toBe("Missing expected fixture data for phone"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these useful? Should we just bail when we see the first error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a bunch of new tests with valid outputs, but should we have more with errors to know that we're properly validating and not accidentally skipping validating objects?
|
(oops, accidentally hit approve when I meant to comment) |
The fixture input validator doesn't currently handle GraphQL abstract types where different items can be different concrete types.
In this PR, when a field is missing from a fixture, we use
typeInfo.getParentType()to determine the concrete type, and validate whether that field is actually expected based on the fixture value's__typename.