generate searchable aggregateItems 2 additional levels #268
Conversation
Codecov Report
@@ Coverage Diff @@
## master #268 +/- ##
==========================================
- Coverage 85.53% 85.32% -0.22%
==========================================
Files 144 144
Lines 6617 6779 +162
Branches 1553 1709 +156
==========================================
+ Hits 5660 5784 +124
- Misses 883 905 +22
- Partials 74 90 +16
Continue to review full report at Codecov.
|
|
|
||
| function adjustDepth(fieldName, depth) { | ||
| if (fieldName == 'aggregateItems') { | ||
| return depth + 1; |
There was a problem hiding this comment.
can the depth be increased indefinitely? Would need to check for some upper bound.
There was a problem hiding this comment.
I've added a statement to check for the typename. Since 'SearchableAggregateResult' type is generated by CLI V2 transformer, we can be sure this doesn't go through infinite loop.
There was a problem hiding this comment.
in the case of isGraphQLAggregateField(field) is true and depth >= maxDepth, we may need to either return depth or throw errorr. Currently it'll return depth-1 which may not be what's intended.
There was a problem hiding this comment.
Same question as above. If it is the aggregate field but the depth reaches maxDepth, we cannot return depth-1 here. I suggest throwing an error.
There was a problem hiding this comment.
The only way this can reach maxDepth is if someone have a schema like below.
type SearchableAggregateResult {
aggregateItems: SearchableAggregateResult
}Modified to throw an error if depth exceeds 100.
|
|
||
| function adjustDepth(fieldName, depth) { | ||
| if (fieldName == 'aggregateItems') { | ||
| return depth + 1; |
There was a problem hiding this comment.
Same question as above. If it is the aggregate field but the depth reaches maxDepth, we cannot return depth-1 here. I suggest throwing an error.
| const getPossibleTypeSpy = jest.spyOn(schema, 'getPossibleTypes'); | ||
| getFields(schema.getQueryType().getFields().aggregateItems, schema, maxDepth, { useExternalFragmentForS3Object: false }); | ||
| expect(getPossibleTypeSpy).toHaveBeenCalled(); | ||
| expect(getFragment).toHaveBeenCalled(); |
There was a problem hiding this comment.
The changes happen on getField but is not covered in the test
There was a problem hiding this comment.
The main reason to traverse two additional levels for 'aggregateItems' field is to make sure that the union type (fragments) are generated with default depth of 2. In this test, we are invoking getFields with default depth of 2 and make sure that the union type is generated fully. This will happen only if the getFields is looking for two additional levels for 'aggregateItems' field.
|
|
||
| expect(getFragment.mock.calls[1][0]).toEqual(aggregateBucketResult); | ||
| expect(getFragment.mock.calls[1][1]).toEqual(schema); | ||
| expect(getFragment.mock.calls[1][2]).toEqual(maxDepth - 1); |
There was a problem hiding this comment.
If the getField is tested, the expect value should be depth+1
There was a problem hiding this comment.
If the depth is not traversing two additional levels, then getFragment would never be called for the schema being tested here. getFragment being invoked twice confirms that getFields is looking for two additional levels when field is of type 'SearchableAggregateResult' > 'aggregateItems'.
| return depth - 1; | ||
| } | ||
|
|
||
| function isGraphQLAggregateField(field) { |
There was a problem hiding this comment.
nit: could be simplified to:
function isGraphQLAggregateField(field) {
return (
field &&
field.name == 'aggregateItems' &&
field.type?.ofType?.name == 'SearchableAggregateResult'
):
}
Description of changes
GraphQL searchable aggregateItems field contains a union type for the result field. It requires two additional level of traversing with the default statement depth (2) to generate the queries as expected.
Description of how you validated changes
Checklist
yarn testpassesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.