add errors for response validation

[Geal]

Fix https://github.com/apollographql/router/issues/5372 Closes https://github.com/apollographql/router/pull/6143

When formatting responses, the router is validating the data returned by subgraphs and replacing it with null values as appropriate. MEssages were put in the response extensions outlining the paths at which the nulls were propagated up the response (due to non nullable types) but it was not generating error messages for invalid values

---

Checklist

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

• [ ] Changes are compatible[^1]
• [ ] Documentation[^2] completed
• [ ] Performance impact assessed and acceptable
• Tests added and passing[^3]
  • [ ] Unit Tests
  • [ ] Integration Tests
  • [ ] Manual Tests

Exceptions

Note any exceptions here

Notes

[^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.

[github-actions[bot]]

@Geal, please consider creating a changeset entry in /.changesets/. These instructions describe the process and tooling.

[router-perf[bot]]

CI performance tests

• [x] 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
• [x] 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

[Geal]

I think error messages could be improved to add more info about the kind of data that was received

[garypen]

The only thing required now is to make the error message match the gateway.

[Geal]

I'm updating the error messages following https://github.com/apollographql/federation/blob/main/gateway-js/src/resultShaping.ts#L305-L430

[Geal]

6733583 follows the gateway's error messages. I'd like to modify them to not show the value we got in the error message, nor mention the subgraph. The rationale here being that we don't want to expose in the errors some sensitive data that might have been returned by error. 6733583 fails the typename_propagation2 test introduced in https://github.com/apollographql/router/issues/3978 because null is not a valid value for the __typename field. I'd argue that this test can actually be removed, as the original issue is already covered by the other tests in the PR

[Geal]

36231f5 updates the error message to expose less information but still give enough for debugging

[Geal]

in the interest of avoiding breaking changes, I'd rather not fail hard on the typename validation, but still add the error message

[Geal]

I removed the __typename validation in 965cfd2 and added a comment for more context

[Geal]

I thought I had added some tests for that, let me check if I have some locally

[Geal]

https://github.com/apollographql/router/issues/4838 is probably related

[Geal]

there's a test now. The invalid non-list value error comes from the gateway: https://github.com/apollographql/federation/blob/main/gateway-js/src/resultShaping.ts#L355 Do we want the validation error messages documented under the router docs or somewhere else in apollo docs?

[abernix]

@geal If we could add something to this page just with a note that it's a partial WIP list, that would be a good starting point that we can revisit later.

[svc-apollo-docs]

✅ Docs Preview Ready

No new or changed pages found.

[Geal]

docs and an error code added

[goto-bus-stop]

I'll push here the test Andrew made in https://github.com/apollographql/router/pull/6143, which validates that integer values fit in the Int 32 bit range.

[goto-bus-stop]

I added a small fix as well to get that test to pass & to fix the ID type name issue. The implementation looks good but I think this needs more tests to exercise all cases pointed out by Result Coercion sections in the spec

[Geal]

@goto-bus-stop when you say it needs to exercise all cases, do you mean that we could generate false positive errors or that there are invalid values that we may not be detecting yet?

[goto-bus-stop]

I'd like to see tests exercising more wrong combinations of types, instead of just one for each type. I think I'm mostly looking for test assurance that we are not missing error cases. Some combinations don't feel all that useful to test but the spec provides some freedom to servers, so I think it's good to nail down what our choices are even for the obvious stuff. For example, if a field returns a boolean true or false, while the schema mandates a String, a GraphQL server is allowed to either cast it to a string "true" or "false" or raise a field error. (We raise a field error, as I think we should)

The case that I was thinking of specifically is if a subgraph responds with a float for an Int field, as in the JSON they are not differentiated. this is what the spec says:

GraphQL services may coerce non-integer internal values to integers when reasonable without losing information, otherwise they must raise a field error. Examples of this may include returning 1 for the floating-point number 1.0, or returning 123 for the string "123". In scenarios where coercion may lose data, raising a field error is more appropriate. For example, a floating-point number 1.2 should raise a field error instead of being truncated to 1.

So it leaves a lot of free choice for us. I think we already do the right thing because serde_json's as_i64 does not lose information, and it doesn't cast non-number types, but it's probably the most useful case to test.

[Geal]

honestly I'd be more ok with missing some error cases right now and revisit in a later PR, than wait a few more months without any validation errors

[goto-bus-stop]

I found several minor issues by adding tests:

• We accept 1234.5678 floating point values for ID types, which the spec arguably ambiguously allows as a return value, but explicitly disallows as an input value for this type. I think the spec is meant to disallow floating point values for IDs in general.
• We do not coerce integer values for ID types to strings--this is a known issue
• We do not accept 1234.0 integer-valued floating-point values for Int types. The spec is vague on this point, but since JSON numbers are all floats, 1234 and 1234.0 are undeniably equivalent and I believe we should cast integer-valued floats to integers.
• There are several other cases where the error message uses the wrong type (producing errors like Invalid value for field String.b which does not make sense)

I'm surprised serde_json doesn't handle 1234.0 -> 1234 conversion in as_i64().

My proposed initial steps for the coercion issues:

• Enable ID validation with the current implementation: accepting ints, floats, and strings. The ID validation is currently disabled because of a typo in the code causing the branch to never match. It's highly unlikely that anyone is returning lists and objects and booleans from ID fields, so I think that's safe to do.
• Do not address other ID coercion issues as customers may be relying on this. I'm adding FIXME comments on the test cases that we can address later if we know it's safe, or in a major release.
• Accept 1234.0 integer-valued floating-point values for Int types, using the coerced i32 value as the value in the response. This is not a breaking change as it expands the accepted set of values. I can do that in a follow-up PR.

[goto-bus-stop]

Backing out my ID change to keep this PR focused on adding error messages only. I'll file a separate PR with low-/non-breaking fixes we can make to align with the GraphQL coercion rules

[abernix]

This PR was reverted during the 1.58.0 release candidacy life-cycle due to regressions which are best described in #6314. While it was present in v1.58.0-rc.0 it is not present in v1.58.0-rc.1 or v1.58.0-rc.2, both of which are not subject to the regression. Thanks for your understanding!

[abernix]

if we ever unrevert this, we should look at https://github.com/apollographql/router/pull/6313