-
Notifications
You must be signed in to change notification settings - Fork 86
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
fix for codegen rejecting operations that contain fragments with type conditions on interfaces #243
Conversation
@barbieri any chance you will have time to take a look at this soon? |
sorry taking so long (busy at work), I'll look at it during the weekend. Thanks for your contribution!!! |
just a heads up I didn't forget about this, seems the CI is broken unrelated to this PR... I'll disable older python to see if that comes back to work |
sorry, I did click the "update branch as merge", but can you do a rebase? Let me know if that works for you |
Pull Request Test Coverage Report for Build 10851448875Details
💛 - Coveralls |
… type conditions on interfaces because it was only checking possibleTypes, which cannot contain interfaces
7c6dead
to
2429ace
Compare
Yep, had to reinstall deps to get the pre-commit tests passing but I think it's set now. |
tests/test-operation-codegen.py
Outdated
test_schema_path = os.path.join( | ||
'tests', 'test-resources', 'operation-codegen', 'operation-test.json' | ||
) | ||
test_op_path = os.path.join( | ||
'tests', 'test-resources', 'operation-codegen', 'op-gen.gql' | ||
) | ||
schema_name = ParsedSchemaName.parse_schema_name('.schema') | ||
|
||
with open(test_op_path) as op_file, open(test_schema_path) as schema_file: | ||
operation_gql = [Source(op_file.read(), op_file.name)] | ||
schema = load_schema(schema_file) | ||
gen = CodeGen( | ||
schema, | ||
schema_name, | ||
operation_gql, | ||
result_buff.write, | ||
short_names=False, | ||
) | ||
gen.write() | ||
result_buff.close() |
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'll approve, but let me know if you can convert this test to be more self contained, instead of using external files. Declare a minimal subset as strings in this file.
Also, I think it would be great to check the generated result_buff
to make sure it's what we expect :-)
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.
Yep I can do that, I will try to get to it today or a bit later on the weekend.
…just leaning on whether or not it fails, and move the test schema/query into the test file rather than in separate files
] | ||
} | ||
} | ||
""" |
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.
@barbieri I cut out a lot of the introspection output to try and keep the inlined resource small, namely the metadata about builtin things like the __Schema
and __Type
types that describe what comes back in the introspection query itself and things related to directives. The test still ran fine, but do you think that may cause an issue later or do you not anticipate the codegen ever caring about that? Trying to avoid some future change suddenly breaking this test due to it having an incomplete introspection output.
thanks! |
Description
Given a schema like this:
codegen would reject queries with fragments like this:
The check for whether or not a type in a type condition was valid was only based on the interface's
possibleTypes
, which cannot include other interfaces. This adds checking whether the type implements the interface. I believe interfaces used to not be allowed to implement interfaces according to the spec, but this was added some time ago.Related Issues
I did not open an issue or spot a related one. If there are issues with the approach in this PR I can close this and open an issue instead to keep track of it.
Progress
Pull request checklist
How to test it
I added a test that uses a schema and operation file in this PR that reproduces the issue.