Adds support for parsing errors with a top level root XML structure #2060
Conversation
cenedhryn
force-pushed
the
salande/s3control-exception-unmarshalling
branch
2 times, most recently
from
b86fe87
to
a1d839f
Compare
Codecov Report
@@ Coverage Diff @@
## master #2060 +/- ##
============================================
- Coverage 78.35% 77.32% -1.04%
- Complexity 153 235 +82
============================================
Files 22 1137 +1115
Lines 804 34879 +34075
Branches 32 2728 +2696
============================================
+ Hits 630 26971 +26341
- Misses 167 6554 +6387
- Partials 7 1354 +1347
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
…ture such as InvalidRequest errors
cenedhryn
force-pushed
the
salande/s3control-exception-unmarshalling
branch
from
a1d839f
to
8425865
Compare
|
SonarCloud Quality Gate failed.
|
| <dependency> | ||
| <groupId>software.amazon.awssdk</groupId> | ||
| <artifactId>aws-query-protocol</artifactId> | ||
| <version>${awsjavasdk.version}</version> | ||
| </dependency> |
There was a problem hiding this comment.
Is this a new dependency, or has it always been there?
There was a problem hiding this comment.
It's a new dependency, on the XmlDomParser
There was a problem hiding this comment.
Unfortunately it's not backwards-compatible to add new dependencies on existing packages, since it causes Brazil major version conflicts. Is there any way to avoid this new dependency?
| * Translate S3 style exceptions, which have the Error tag at root instead of wrapped in ErrorResponse. | ||
| * If the exception follows this structure but isn't known, create an S3ControlException with the | ||
| * error code and message. |
There was a problem hiding this comment.
Isn't this already implemented in the S3 client?
There was a problem hiding this comment.
In S3 the whole client is customized to use the AwsS3ProtocolFactory instead of AwsXmlProtocolFactory. It's not possible to use it dynamically for one or two exception types.
There was a problem hiding this comment.
Can we not use the AwsS3ProtocolFactory for all exception types?
| log4j.logger.org.apache.http=DEBUG | ||
| log4j.logger.org.apache.http.wire=DEBUG |
There was a problem hiding this comment.
That's a lot of logging for the default.
There was a problem hiding this comment.
Indeed. Good catch, in a previous incarnation it was commented out but missed it in following commits.
|
Superceded by a different PR |
…4bd73861b Pull request: release <- staging/c25e67e9-b2b6-46b4-b4cf-63d4bd73861b
Description
InvalidRequestExceptionMotivation and Context
Errors from the server side that contain an XML body with error description should have this structure:
However at least one error type, InvalidRequest, that S3Control returns (observed through the Jobs CRUD operations) use the S3-structure with no wrapping top level tag:
The error parser that S3 control uses cannot parse the XML document with the Error as the document root and therefore creates an exception with no information.
Testing
Added wiremock tests and integration tests, verified that existing errors unmarshall correctly.
Types of changes
Checklist
mvn installsucceedsLicense