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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

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

wants to merge 1 commit into from

Conversation

pdeglopper
Copy link

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
Copy link

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> (ø) ⬇️
.../com/spotify/google/cloud/pubsub/client/Acker.java 87.03% <0%> (+3.7%) 20% <0%> (+2%) ⬆️

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Contributor

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.

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