Remove URLDecodedKey from S3Object

[johanneswuerbach]

Issue #, if available: Fixes #82

Description of changes: Populates the URLDecodedKey property from the S3Object struct as it is currently empty and is never populated by S3 itself https://docs.aws.amazon.com/AmazonS3/latest/dev/notification-content-structure.html

The 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.

[codecov-io]

Codecov Report

Merging #335 (b2d7eb7) into master (dda2bc7) will decrease coverage by 0.24%.
The diff coverage is 50.00%.

@@            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     

Impacted Files	Coverage Δ
events/s3.go	50.00% <50.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dda2bc7...b2d7eb7. Read the comment docs.

[bmoffatt]

Instead of removing it, maybe we can fix it with a custom deserializer

[johanneswuerbach]

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?

[harrisonhjones]

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.

[johanneswuerbach]

Updated to always populate the property instead of removing it.

[bmoffatt]

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

type S3Object {
        ....
	URLDecodedKey string `json:"-"` // populated by custom deserializer
        ....
}

[johanneswuerbach]

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.

[harrisonhjones]

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?

[bmoffatt]

We can't guarantee full compatibility for consumers of the serialized version of the object, arguably it breaks either way.

1. if the tag stays, consumer sees {"urlDecodedKey": "" } -> {"urlDecodedKey": "some value"}
2. if the field is ignored, consumer sees {"urlDecodedKey": ""} -> {}

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.

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?

@harrisonhjones I've been using the requires-v2 tag for this

[carlzogh]

The change here brings this library to parity with the Java events library (ref. urlDecodedKey) and I think this is the right approach.

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 aws-lambda-go adds in to these events (eg. urlDecodedKey). This being a pre-existing issue that is not made worse by this PR, I wouldn't hold up merging this in but I still think it's worth trying to figure out how we can better draw this line - potentially in v2.

[bmoffatt]

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