feat(rover-client): improves error handling #209
Conversation
EverlastingBugstopper
added
feature 🎉
EverlastingBugstopper
requested review from
JakeDawkins and
lrlna
as code owners
EverlastingBugstopper
force-pushed
the
avery/my-number-one-client
branch
2 times, most recently
from
0116589 to
101ae48
Compare
EverlastingBugstopper
force-pushed
the
avery/my-number-one-client
branch
2 times, most recently
from
5b58b98 to
2d908cd
Compare
| fn get_push_response_from_data( | ||
| data: push_schema_mutation::ResponseData, | ||
| graph: String, | ||
| ) -> Result<push_schema_mutation::PushSchemaMutationServiceUploadSchema, RoverClientError> { | ||
| // then, from the response data, get .service?.upload_schema? | ||
| let service_data = match data.service { | ||
| Some(data) => data, | ||
| None => return Err(RoverClientError::NoService), | ||
| None => return Err(RoverClientError::NoService { graph }), | ||
| }; | ||
|
|
||
| if let Some(opt_data) = service_data.upload_schema { | ||
| Ok(opt_data) | ||
| } else { | ||
| Err(RoverClientError::HandleResponse { | ||
| msg: "No response from mutation. Check your API key & graph name".to_string(), | ||
| Err(RoverClientError::AdhocError { | ||
| msg: "No response from the graph registry. Check your API key & graph name".to_string(), | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
RoverClientError::NoService typically implies that either the API key is invalid, or the graph does not exist - so what's the deal if upload_schema is null? - I think since we already know that data.service is not null, then the API key AND the graph name should both be valid, meaning the error message we have here is not correct. Is this a malformed response? Why does the API allow this field to return null? What does it signify? These numbers Mason, what do they mean?
There was a problem hiding this comment.
https://github.com/mdg-private/monorepo/blob/abca4af89d0a84ee51add654db3362374e5ba2fa/registry/src/resolvers/ServiceMutation.ts#L1019-L1037
Looking at the resolver that runs this code, it looks like the null value here would mean that the operation on the backend to actually store the schema failed. There's a vaild graph and variant, but something went wrong with the actual storage operation of the schema. I feel like this could've been rolled into the success and message here instead of making this nullable but that's where we're at
There was a problem hiding this comment.
Oh! Can we then say exactly what you just wrote: 'Apollo Studio wasn't able to store the schema`?
There was a problem hiding this comment.
I've added null_field to MalformedResponse since it seems like this would be a bug that the user can't fix. Do we think that's OK here or should we make this a typed error and then have the suggestion be "rerun this command and submit an issue if it keeps happening?"
i think for now we should keep it as MalformedResponse and then if it happens frequently we'll get people reporting issues that we can then escalate
EverlastingBugstopper
force-pushed
the
avery/my-number-one-client
branch
2 times, most recently
from
512dc87 to
74e67db
Compare
| Err(RoverClientError::HandleResponse { | ||
| msg: "No response from mutation. Check your API key & graph name".to_string(), | ||
| }) | ||
| Err(RoverClientError::MalformedResponse) |
There was a problem hiding this comment.
| Err(RoverClientError::MalformedResponse) | |
| Err(RoverClientError::AdhocError { | |
| msg: "Apollo Studio failed to store the schema".to_string() | |
| }) |
There was a problem hiding this comment.
I just added null_field to MalformedResponse so when we tell folks to submit an issue they can tell us what field was null
There was a problem hiding this comment.
I'm really hoping these MalformedResponse errors just.. never occur! but since they can be null, they likely will. I'd like to treat those as bugs though, so if we get an issue where folks are getting this, we need to either handle the error better or fix it in the API
EverlastingBugstopper
force-pushed
the
avery/my-number-one-client
branch
2 times, most recently
from
30bceb8 to
3f8b96c
Compare
EverlastingBugstopper
added
status - needs review
and removed
status - changes requested
labels
Adds some more strongly typed errors in rover-client in addition to some new suggestions in rover.
EverlastingBugstopper
force-pushed
the
avery/my-number-one-client
branch
from
3f8b96c to
0d595cc
Compare
Adds some more strongly typed errors in rover-client
in addition to some new suggestions in rover.