Storage: Allow null values in labels map #6859
Conversation
googlebot
added
the
cla: yes
| new Function<String, String>() { | ||
| @Override | ||
| public String apply(String input) { | ||
| return input == null ? "" : input; |
There was a problem hiding this comment.
For consistency could you use Data.<String>nullOf(String.class).
There was a problem hiding this comment.
Seems obtuse to me, but I'll do it for consistency. Is there a reason it wouldn't be better/more readable to do it the other way around and replace other instances of Data.nullOf(String.class) with "" for readability?
There was a problem hiding this comment.
Could you file an issue to better address this in the library for readability? So it's done all at once instead of a one-off and then we can maintain consistency for now.
| @@ -1209,7 +1211,17 @@ public Builder setDefaultAcl(Iterable<Acl> acl) { | |||
|
|
|||
| @Override | |||
| public Builder setLabels(Map<String, String> labels) { | |||
| this.labels = labels != null ? ImmutableMap.copyOf(labels) : null; | |||
| if (labels != null) { | |||
There was a problem hiding this comment.
Add an integration test to exercise this case.
Codecov Report
@@ Coverage Diff @@
## master #6859 +/- ##
============================================
+ Coverage 46.37% 46.37% +<.01%
+ Complexity 28045 28029 -16
============================================
Files 2615 2615
Lines 288309 288313 +4
Branches 33804 33810 +6
============================================
+ Hits 133712 133716 +4
Misses 144333 144333
Partials 10264 10264
Continue to review full report at Codecov.
|
|
I see you added a unit test. could you add an integration test as well? Overall LGTM. |
…a into fixsetlabels
…a into fixsetlabels
There was a problem hiding this comment.
Pending tests passing, otherwise LGTM. Thanks @JesseLovelace!
The way to delete a label is to set its value in the labels map to null. Currently, the labels map is converted to an ImmutableMap, which can't have null values. It's still possible to delete values by setting their values to the empty string, but that's not intuitive, users would generally expect setting null to work based on the documentation. This converts null values to the empty string in the builder to accomplish this. This was discovered while updating Java samples