add errors for response validation

[Geal]

Fix #5372
Closes #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¹
• 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. ↩

[router-perf]

CI performance tests

• 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

[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 #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

[goto-bus-stop]

My change: this call is recursing from Int! to Int, it's not entering a new field, so the parent type remains the same.
Before this change you could get an error message like Invalid value found for field Int!.field, which doesn't make sense

[Geal]

so this is not a huge issue because parent_type is only used for that error message, but I worry that this would break assumptions when we change the code later. Could we move it back to field_type, but use parent_type.inner_named_type() in the error message?

[goto-bus-stop]

The code should assume that the parent type is the type that the selection is executed on, while the field type is the type that the selection returns. inner_named_type() is also incorrect as it would mention Int.someNumber in the error, when Int is the return type, not the type that has the field someNumber.

Before this change parent_type would sometimes contain the parent type, but sometimes not contain the parent type at all. It could be two entirely different things that must be used differently. So I don't think it could possibly be correct before

addressed

[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

[goto-bus-stop]

This should be good to, I reverted my unnecessary changes so the only things I added are more tests encoding the current behaviour, and a fix to the error message.

[glasser]

I think there's a missing if !input.is_null() or something here. If I understand correctly, the executable::Type::Named(type_name) matcher arm that this is under actually means "nullable named type" (vs executable::Type::NonNullNamed), which means it is valid for input to be null. This change is leading to the addition of an incorrect Expected a valid enum value for type {} error to responses any time that a subgraph returns null for a nullable enum field.

[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!