Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Best-effort error-decoding fix #357

Merged
merged 5 commits into from
Aug 11, 2022
Merged

Best-effort error-decoding fix #357

merged 5 commits into from
Aug 11, 2022

Conversation

Baccata
Copy link
Contributor

@Baccata Baccata commented Aug 9, 2022

Remove the best-effort error-decoding logic

Now, when a client gets an unsuccessful response from a server, it'll
try to inspect the X-Error-Type header first, and then will try to match
the received status code against the errors it knows about
(provided the code in question is uniquely used)

If the client cannot infer a unique alternative based on this
information, it'll raise a generic "UnknownErrorResponse"

scala-steward and others added 4 commits August 8, 2022 11:15
Now, when a client gets an unsuccessful response from a server, it'll
try to inspect the X-Error-Type header first, and then will try to match
the received status code against the errors it knows about
(provided the code in question is uniquely used)

If the client cannot infer a unique alternative based on this
information, it'll raise a generic "UnknownErrorResponse"
@@ -24,6 +24,14 @@ final case class ShapeId(namespace: String, name: String) extends HasId {
}

object ShapeId extends ShapeTag.Has[ShapeId] {
def parse(string: String): Option[ShapeId] = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we really need this, I suggest we implemente it on top of ShapeId.from(s: String) from smithy-model itself. You may need to catch ShapeIdSyntaxException

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

smithy4s-core cannot depend on smithy-model, as it's a java-only library. Also it'd be pretty overkill to pull smithy-model for a single method.

@daddykotex
Copy link
Contributor

I'm having trouble figuring out what exactly changes from before just by looking at the code. can you help me out here?

@daddykotex
Copy link
Contributor

I'm having trouble figuring out what exactly changes from before just by looking at the code. can you help me out here?

There is support for the NameOnly of the shape in the header which was not there before but I don't think that's it.

@Baccata
Copy link
Contributor Author

Baccata commented Aug 10, 2022

I'm having trouble figuring out what exactly changes from before just by looking at the code. can you help me out here?

@daddykotex sure. So the behaviour change is that now, if we can't discriminate by neither X-Error-Type header value NOR status code (because several errors have the same status code in the smithy model), we raise an UnkownError. Previously, if we couldn't discriminate, we'd apply an untagged-union kind of decoding logic by doing a "is there a client-error that we can decode to ? if so, raise it", but it turned out to be a terrible idea, as teams are apparently using the same "structure" for a number of their errors, which led our logic to raise errors on the client side that were definitely incorrect.

The previous behaviour was only matching the name of the error in X-Error-Type, I've added logic to future-proof it by accepting fully-qualified names (the case that references ShapeId), as using the full qualified name as a discriminator would be more precise.

@daddykotex
Copy link
Contributor

Ah ok, that was my high level understanding of what we had discussed. Makes sense to me.

Copy link
Contributor

@daddykotex daddykotex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM,

thanks for the cleanup of my code :D

@Baccata Baccata marked this pull request as ready for review August 11, 2022 06:56
@Baccata Baccata merged commit 7bb4c8a into 0.0.15 Aug 11, 2022
@Baccata Baccata deleted the best-effort-error-fix branch August 11, 2022 06:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants