This repository was archived by the owner on Sep 3, 2021. It is now read-only.
Fixes: merging interface type nodes / using interfaces with no scalars #365
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
Upstream merge
used in checking that a given node type definition has at least one property type field (scalar, temporal, etc.), as when it does not, an ordering enum is not generated
conditionally generates the arguments and enum definition used for ordering a given node type, by checking for whether that type has at least 1 orderable field
when at the Query type, augmentNodeTypeFields now blocks initialization of the ast for generated ordering and filtering types, as they are never used (e.g., _QueryOrdering, etc.)
similar to what's done with nodeCreate
tests an edge case for when the user has provided their own enum and field arguments using it, but it would not have been generated and would not be successfully translated. Despite this, it should probably persist~
Codecov Report
@@ Coverage Diff @@
## master #365 +/- ##
==========================================
+ Coverage 96.31% 96.34% +0.02%
==========================================
Files 24 24
Lines 2821 2842 +21
==========================================
+ Hits 2717 2738 +21
Misses 104 104
Continue to review full report at Codecov.
|
johnymontana
approved these changes
Dec 22, 2019
|
Looks good. I think we do not want to add the |
# for free
to subscribe to this conversation on GitHub.
Already have an account?
#.
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.
This PR contains a few fixes:
Issue #361 Merge node should add interface label
This issue / inconsistency occurs when a merge node operation creates a node. I realized after the initial PR for merge operations that we should implement the same behavior that the node creation mutation has when the node is an object type implementing an interface - the node recieves both the label of its type and the label of the implemented interface.
Issue #349 Interfaces with no scalar fields generate invalid schemas
This issue concerns the ordering enum type, and corresponding
orderByfield arguments, being generated for a node type even when that type does not contain at least 1 property type field which could be used for ordering (scalar, temporal, etc.). This issue is avoided when a node is represented by an object type definition, as in such cases we guarantee the existence of at least 1 field - the_idfield. But when an interface definition represents a node, we do not currently generate an_idfield for it, so it opened up the possibility of generating and trying to use an empty ordering enum, built for an interface type with no fields appropriate for ordering.This raises the question of whether we should add the
_idfield to interfaces representing nodes. If we were to do that, it seems we would also need add the_idfield to all object types implementing that interface.To resolve this issue for now, the generation of an ordering enum for an object or interface node type, and any field arguments that use it, is now conditional on that node type having at least 1 property type field.