feat(federation/composition): SingleCompositionError #7590
Conversation
|
@TylerBloom, please consider creating a changeset entry in |
✅ Docs preview has no changesThe preview was not built because there were no changes. Build ID: 7044316798c0acb71580739d |
TylerBloom
force-pushed
the
tylerb/fed-599
branch
from
1329eeb to
4a84d1e
Compare
apollo-federation/src/error/mod.rs
Outdated
| @@ -114,6 +115,48 @@ pub enum UnsupportedFeatureKind { | |||
| Alias, | |||
| } | |||
|
|
|||
| #[derive(Debug, Clone, thiserror::Error)] | |||
| pub enum SingleCompositionError { | |||
There was a problem hiding this comment.
I was under impression that we don't want to preserve the existing Single/Multi/Aggregate federation error enum. I thought we just want a CompositionError and return a vec if we need multiple errors?
There was a problem hiding this comment.
You are correct. That is our long term goal, but removing the multiple fed error variants is a lot of work. At one point in build_query_plan, we could get multiple errors (it's very early on). Fixing that is outside the scope of this ticket, so removing the two multiple fed errors variants is too.
We are using MultipleFederationErrors in composition code. I can replace those with a Vec of composition errors, but those are currently being converted into a SubgraphError and then into a composition error. This largely happens in the upgrade and validation code.
I think the final goal is to identify the subset of errors that might get turned into SubgraphError and use that error type in the locations I mentioned above. Then, merging and satisfiability can add to SingleCompositionError and the errors that need to be SubgraphErrors can be. I think this should be a follow up PR, though.
There was a problem hiding this comment.
Should we just start with CompositionError and tackle the MultipleFederationErrors later on?
There was a problem hiding this comment.
We should, and that's my goal here. What I don't think we can do (yet) is have upgrade and validate use composition errors inside of MultipleFederationErrors. Otherwise, you end up with the same recursive error problem with SubgraphError I got rid of here.
TylerBloom
force-pushed
the
tylerb/fed-599
branch
from
b1d0eb4 to
0a27d2a
Compare
|
Probably should make changes to the |
| @@ -361,7 +361,6 @@ fn add_requires_error( | |||
| application: requires_application.to_string(), | |||
| message, | |||
| } | |||
| .add_subgraph(subgraph_name.to_owned()) | |||
There was a problem hiding this comment.
We should create a ticket to add this subgraph info back in later (I'm guessing it'll happen when the errors argument changes to a vector of composition errors).
|
There is a lint error. |
This PR splits out the
SubgraphErrorvariant ofSingleFederationErrorinto a new enum,SingleCompositionError. This includes a conversion fromSubgraphErrortoSingleCompositionErrorbut not fromFederationError/SingleFederationError. This is to ensure all federation errors that are intended to beSubgraphErrors do. Lastly, any function that was returning a vec of (single) federation errors is now returning aVec<SingleCompositionError>Checklist
Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.
Exceptions
Note any exceptions here
Notes
Footnotes
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. ↩
Configuration is an important part of many changes. Where applicable please try to document configuration examples. ↩
A lot of (if not most) features benefit from built-in observability and
debug-level logs. Please read this guidance on metrics best-practices. ↩Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions. ↩