-
Notifications
You must be signed in to change notification settings - Fork 550
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
Remove URLDecodedKey from S3Object #335
Remove URLDecodedKey from S3Object #335
Conversation
5fe0657
to
e6ca184
Compare
Codecov Report
@@ Coverage Diff @@
## master #335 +/- ##
==========================================
- Coverage 72.46% 72.22% -0.25%
==========================================
Files 18 19 +1
Lines 730 738 +8
==========================================
+ Hits 529 533 +4
- Misses 136 138 +2
- Partials 65 67 +2
Continue to review full report at Codecov.
|
Instead of removing it, maybe we can fix it with a custom deserializer |
Happy to do that, but shouldn’t it be a method computed demand instead of a property or should it be computed every time even if not needed? For backwards compatibility the later seems better, but feels kind of wasteful. Preferences? |
We could benchmark it but my guess is that the performance impact of keeping it would be very small. I would personally prefer for the package to keep to the Go backwards compatibility promise as much as is possible / reasonable. |
e6ca184
to
a62ba4a
Compare
a62ba4a
to
b2d7eb7
Compare
Updated to always populate the property instead of removing it. |
I think that we should also change the json tag, to help signal to others that urlDecodedKey is not a part of the event model
|
I was also thinking about that, but discarded the idea as that also seems like a breaking change. If someone serialises the current object to json, this would modify the shape of the json. |
Unfortunate but probably true. I wonder if we should consider keeping a list of "stuff we should consider fixing in a MV 2"? Maybe create an issue with some kind of "mv2" tag? |
We can't guarantee full compatibility for consumers of the serialized version of the object, arguably it breaks either way.
CC @carlzogh what do you think? Have we run into this situation in the java project recently? My preference right now, is to take breaking changes if the type would otherwise be documenting something false. Were we code-generating these, and the schema updated, I think we'd default into that behavior.
@harrisonhjones I've been using the |
The change here brings this library to parity with the Java events library (ref. From a consumer's perspective, I don't see this as a very risky change as a property that was previously empty now holds a value. We're not changing the JSON contract - were we to remove the field, I'd see that as a breaking change and would probably think twice about it. I can't think of consumer use-cases that would break as a result of us merging this in, aside from the fact that we're not differentiating between service event models and fields that |
Agree on not holding up this change, since it doesn't make the existing problem worse :) I'll move my thoughts on #335 (comment) to a separate issue to track |
Issue #, if available: Fixes #82
Description of changes: Populates the
URLDecodedKey
property from theS3Object
struct as it is currently empty and is never populated by S3 itself https://docs.aws.amazon.com/AmazonS3/latest/dev/notification-content-structure.htmlThe population implements the convenience method as the lambda Java SDK https://github.com/aws/aws-sdk-java/blob/6a4c873c71320ef0175ca1c13188e9c850a85e51/aws-java-sdk-s3/src/main/java/com/amazonaws/services/s3/event/S3EventNotification.java#L176-L183
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.