rewrite fragments #152
Closed
rewrite fragments #152
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kindermax
force-pushed
the
rewrite-fragments
branch
7 times, most recently
from
a2503a2 to
ce972e1
Compare
In this PR we addressed the issue with broken fragments merging. Historically, hiku has not supported unions/interfaces so fragments merging were ok. But after we added unions/intefaces support + fragments support, things went in the wrong direction. Fragments merging (fields merging to be more accurate) has its own rules in graphql spec and we did not complied. So why thought ? Hiku arhitecture was build without fragments in mind, that is - engine and denormalization modules can not work with fragments. So to avoid complex rewriting for engine and denormalization we decided to merge fragments to avoid fields duplication - the most hard thing to support in engine. In this PR, we try to mimic graphql-py behavior or merging fields: - We remove complext merging in query parsing step, now all fields and fragments are stored in hiku ast as is - In engine we adapted SplitQuery to group fields and link: - Fields are leaf nodes so it is safe to take first field from fields_info (list of field instances) as long as field args are the same - Same link is collected and will be merged before `schedule_link` call, so that no duplicated links will be processed - Cache works as previous but hashes are changed sihce link node fields are merged - Denormalize adapted to work with links/fields resolved multiple times and checks that field already presents in result skipping its serialization So basically we moved handling of fields merging to engine/denormalization stage and simplified parsing. Some other changes: - Refactor SplitQuery types, introduce FieldInfo and LinkInfo instead of tuples - Refactor GroupQuery to use FieldInfo/LinkInfo - Fix cache tests - Drop fragments hack from result.py:Proxy since we now provide proper Proxy to index for each fragment - Add name for Fragment, if name is None - this is an InlineFragment - Node.fragments_map only returns named fragments map
kindermax
force-pushed
the
rewrite-fragments
branch
from
ce972e1 to
64b1877
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In this PR we addressed the issue with broken fragments merging. Historically, hiku had no support for unions/interfaces so fragment merging was ok. But things went in the wrong direction after we added unions/interfaces support + fragments support.
Fragments merging (fields merging to be more accurate) has its rules in graphql spec, and we did not comply to them. So why thought ?
Hiku architecture was built without fragments in mind, that is - engine and denormalization modules could not work with fragments. So to avoid complex rewriting of the engine and denormalization modules, we decided to merge fragments to avoid field duplication - the hardest thing to support in the engine.
In this PR, we try to mimic graphql-py behavior or merging fields:
schedule_linkcall, so that no duplicated links will be processedSo basically we moved handling of fields merging to engine/denormalization stage and simplified parsing.
Some other changes: