Fix value type behavior within composition and execution #3182
Conversation
There was a problem hiding this comment.
@trevor-scheer I think your comment on a different approach is the right direction. Even though removing value types from the type map is a quicker approach, I think it may lead to confusion down the road. I'd rather us model the value type directly. Something akin to isSharedValueType or even creating a list of all services that share the value type would allow us to better compute the right query plan and give better errors when the planner reaches an unexpected state. Let's take another revision of this including more time on the test suite to model the possible interactions
packages/apollo-federation/src/composition/__tests__/composeAndValidate.test.ts
Outdated
Show resolved
Hide resolved
| expect.addSnapshotSerializer(astSerializer); | ||
| expect.addSnapshotSerializer(queryPlanSerializer); | ||
|
|
||
| describe('value types', () => { |
There was a problem hiding this comment.
It will take more time, but I think we should verify all types of ValueTypes in this test suite instead of just a single object type. We would be remiss if we made a change that broke unions by accident without a strong test coverage.
trevor-scheer
force-pushed
the
trevor/value-types-bug
branch
from
ba1da07 to
3818c6b
Compare
There was a problem hiding this comment.
@trevor-scheer I've left a few questions about the actual implementation here, but I think the result is the one for which we are looking!
I do notice we are doing a lot of accessing the last element in the serviceNames array. I wonder if there is a better way to represent the "winning" service here?
| ); | ||
|
|
||
| const isValueType = | ||
| name.length === 0 && |
There was a problem hiding this comment.
Are name and kind arrays here? Why are you checking length?
There was a problem hiding this comment.
They are - in the case that they're different, the diffing function returns both names in an array which is useful for messaging. I've written a function to hide the impl. details of diffing for simple use cases like this one.
| * A map for tracking which types have been determined to be a value type, a type | ||
| * shared across at least 2 services. | ||
| */ | ||
| export interface ValueTypesMap { |
There was a problem hiding this comment.
Probably would be better to make this a Set since it isn't a map that stores anything.
| ...(keyDirectivesMap[typeName] && { | ||
| keys: keyDirectivesMap[typeName], | ||
| }), | ||
| isValueType: Boolean(valueTypesMap[typeName]), |
There was a problem hiding this comment.
If this was a Set, you could write this as valueTypesSet.has(typename)
| @@ -343,7 +378,7 @@ export function addFederationMetadataToSchemaNodes({ | |||
| ) { | |||
| field.federation = { | |||
| ...field.federation, | |||
| serviceName: baseServiceName, | |||
| serviceName: serviceNames && serviceNames[serviceNames.length - 1], | |||
There was a problem hiding this comment.
hmm, so in this case the last service in the list wins as the serviceName for the federation field? Does this still apply for value types?
There was a problem hiding this comment.
This is a good point, and made me consider the effect and validity of @provides on value type fields.
Resolved in 053d1a3
This is a first attempt at completing the implementation of value types. This implementation is very inline with a suggestion from a contributor (@victorandree) that can be found here: #3141 (comment) The solution is relatively straightforward, though a bit hacky. After the maps are constructed, we go back and "clean them up" by identifying value types in the map and removing any signs of service ownership from them. While building a query plan, the gateway now sees an "unowned type" as belonging to the parentGroup's service. I'm unsure if this has any reaching implications (previously an unowned type or field would throw), however this code path was never exercised in our test cases.
* Clean up artifacts of previous impl * Begin refactor by preserving serviceNames on types rather than keeping only the latest
This commit implements value types as a core part of composition and query planning. With this work, we now: * build another metadata map for tracking value types * attach value type metadata to schema type nodes * make query planning decisions based on value type metadata
With value types, it doesn't make sense for a since service to own the fields on the type. Update the types and implementation to reflect this. This change surfaced a potential bug, so I added a couple tests to ensure things work and resolve as expected when @provides directives are used on fields belonging to a value type.
While the implementation currently works, it depends on a field's `serviceName` being falsey rather than leveraging the new `belongsToValueType` on the field's `federation` object.
trevor-scheer
force-pushed
the
trevor/value-types-bug
branch
from
59e5047 to
cd3934b
Compare
…ql/apollo-server#3182) This commit implements value types as a core part of composition and query planning. With this work, we now: * build another metadata map for tracking value types * attach value type metadata to schema type nodes * attach value type metadata to fields on value types * make query planning decisions based on value type metadata With value types, it doesn't make sense for a single service to own the fields on said value type. Update the types and implementation to reflect this. Apollo-Orig-Commit-AS: apollographql/apollo-server@1852033
This PR addresses my oversights in the original value type implementation here:
#3063
This PR implements value types as a core part of composition
and query planning. With this work, we now:
Fixes #3141 (see for more context)