spotify / async-google-pubsub-client

[SUNSET] Async Google Pubsub Client
Apache License 2.0
158 stars 40 forks source link

allow null data - valid if at least one attribute exists #31

Open pdeglopper opened 7 years ago

pdeglopper commented 7 years ago

This should be enough to enable deserializing messages that omit the data field, which appears to be valid if there's at least one attribute set. I don't really have the insight at present into the ways this client is used to send events to say whether or not it needs to validate that one of those two is set before building the message.

codecov-io commented 7 years ago

Codecov Report

Merging #31 into master will increase coverage by 0.44%. The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #31      +/-   ##
============================================
+ Coverage     75.83%   76.28%   +0.44%     
- Complexity      189      191       +2     
============================================
  Files            25       25              
  Lines           894      894              
  Branches         59       59              
============================================
+ Hits            678      682       +4     
+ Misses          183      181       -2     
+ Partials         33       31       -2
Impacted Files Coverage Δ Complexity Δ
...om/spotify/google/cloud/pubsub/client/Message.java 59.09% <ø> (ø) 13 <0> (ø) :arrow_down:
.../com/spotify/google/cloud/pubsub/client/Acker.java 87.03% <0%> (+3.7%) 20% <0%> (+2%) :arrow_up:

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 d0c10af...003e0c0. Read the comment docs.

danielnorberg commented 7 years ago

Silently changing data() to allow null seems like it might make existing code assuming non-null fail. Changing the field type to Optional<String> would be a painful breaking change. Would it make sense to represent null data as an empty string instead?

pdeglopper commented 7 years ago

The down side there is that as far as I can tell a message could validly contain either the empty string or null as data, so treating them identically when serializing makes the serializing and then deserializing not the identity function. That's an edge case, but null data is something the google API spec and command line tools allow so existing code probably shouldn't be assuming it. I don't feel very strongly about it, though.

danielnorberg commented 7 years ago

Docs are a bit vague but they seem to indicate that empty data is treated same as null data. So might be fine for us to represent null data as empty data.

https://cloud.google.com/pubsub/docs/reference/rest/v1/PubsubMessage

danielnorberg commented 6 years ago

Pubsub seems to treat messages where (1) data is an empty string, (2) data is null and (3) data is absent (field not present in message object) the same. Pulling the above yields messages with the data field absent.

Thus it would seem safe for this client to represent null/absent data as the empty string.