Do not override concrete type names with interface names when merging responses

[tninesling]

When using @interfaceObject, the planner can request an object's concrete __typename in parallel with its data. If the data comes back with an interface name, and the router receives it after receiving the concrete name, the existing implementation will overwrite the concrete name with the interface name. The downstream result is that the router will nullify any data paths with a __typename that doesn't correspond to a concrete type in the schema.

This fix makes the response merging type-aware. For any field which isn't a type name, we follow the existing deep merge logic for JSON values. If we are merging two instances of __typename, this now checks the schema to see if the existing type is an implementer or subtype of the incoming type. If it is, we do not overwrite it with the more generic type. If the existing type is not a descendant of the incoming one, such as when they are unrelated, we overwrite it with the incoming one as usual.

---

Checklist

Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.

• Changes are compatible¹
• Documentation² completed
• Performance impact assessed and acceptable
• Tests added and passing³
  • Unit Tests
  • Integration Tests
  • Manual Tests

Exceptions

Note any exceptions here

Notes

Footnotes

1. It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this. ↩

2. Configuration is an important part of many changes. Where applicable please try to document configuration examples. ↩

3. Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions. ↩

[svc-apollo-docs]

✅ Docs Preview Ready

No new or changed pages found.

[router-perf]

CI performance tests

• connectors-const - Connectors stress test that runs with a constant number of users
• const - Basic stress test that runs with a constant number of users
• demand-control-instrumented - A copy of the step test, but with demand control monitoring and metrics enabled
• demand-control-uninstrumented - A copy of the step test, but with demand control monitoring enabled
• enhanced-signature - Enhanced signature enabled
• events - Stress test for events with a lot of users and deduplication ENABLED
• events_big_cap_high_rate - Stress test for events with a lot of users, deduplication enabled and high rate event with a big queue capacity
• events_big_cap_high_rate_callback - Stress test for events with a lot of users, deduplication enabled and high rate event with a big queue capacity using callback mode
• events_callback - Stress test for events with a lot of users and deduplication ENABLED in callback mode
• events_without_dedup - Stress test for events with a lot of users and deduplication DISABLED
• events_without_dedup_callback - Stress test for events with a lot of users and deduplication DISABLED using callback mode
• extended-reference-mode - Extended reference mode enabled
• large-request - Stress test with a 1 MB request payload
• no-tracing - Basic stress test, no tracing
• reload - Reload test over a long period of time at a constant rate of users
• step-jemalloc-tuning - Clone of the basic stress test for jemalloc tuning
• step-local-metrics - Field stats that are generated from the router rather than FTV1
• step-with-prometheus - A copy of the step test with the Prometheus metrics exporter enabled
• step - Basic stress test that steps up the number of users over time
• xlarge-request - Stress test with 10 MB request payload
• xxlarge-request - Stress test with 100 MB request payload

[lrlna]

I think this potentially needs to live in the same place as .deep_merge implementation currently lives.

Do you know whether or not there are any performance implication for this change?

[Geal]

when using interfaceObject, the planner is supposed to provide the input_rewrites and output_rewrites fields in fetch nodes to indicate how the typename field has to change. Before merging this, could it be that you found a case where the planner did not provide it?

[tninesling]

when using interfaceObject, the planner is supposed to provide the input_rewrites and output_rewrites fields in fetch nodes to indicate how the typename field has to change. Before merging this, could it be that you found a case where the planner did not provide it?

I'm not familiar with the rewriting, would you be able to explain a bit more how this works? I'm not sure it would work in the situation this PR addresses because the type name and the data are being fetched separately in parallel nodes, but the problem only seems to surface when the client explicitly requests the typename for an entity.

[sachindshinde]

@Geal

when using interfaceObject, the planner is supposed to provide the input_rewrites and output_rewrites fields in fetch nodes to indicate how the typename field has to change. Before merging this, could it be that you found a case where the planner did not provide it?

Those query plan fields won't help in this case. To elaborate:

1. When querying an @interfaceObject type from a subgraph, we'll query and get back a __typename which is the interface type name instead of an object one.
   • We can't use output rewrites to overwrite it, since QP doesn't know what the actual type name is. (That's determined by a later query that fetches __typename from an interface-@key subgraph.)
   • We can't just not query __typename either, since query plan execution needs to track the type information somehow.
2. This means the response data will have a __typename that is incorrect, until the query to the interface-@key subgraph is made (which should overwrite it).

The problem is that we've encountered query plans where, even though the interface-@key subgraph query is ordered afterwards, the incorrect __typename isn't getting overwritten in execution. The real problem here has to do with how we track this kind of type information. I intend to fix this more generally when we get to source-awareness as part of the @interfaceObject changes, but in the meanwhile, we're looking to get a quick fix out to folks.

The fix in this PR is to avoid clobbering the correct __typename with the incorrect one when merging, so it'll work regardless of merge order.