Fix serialization of sets

[johnflavin]

All Sets are now serialized as maps of keys to empty maps, rather than maps of keys to null.

This specific motivating case is ContainerConfig.exposedPorts, but every other Set in the model should have the same behavior. As far as I am aware we only use Sets to model docker's "map of keys to empty objects" API pattern. I infer they use this pattern as a workaround for a lack of a native set type in Go.

I also refactored ContainerConfig.volumes to be a Set rather than being a hard-coded Map<String, Map>. I had implemented volumes as the latter because I was unaware we were using a custom serializer for Set to reproduce docker's API pattern. Refactoring volumes to also be a Set simplifies the code but leaves the serialization the same.

While I was in ContainerConfig, I cleaned up all the if (foo != null) { builder.foo(foo); } statements for properties that are @Nullable. AutoValue doesn't care if you feed a null into those builder methods, so we don't need to check.

Fixes #893.

[codecov-io]

Codecov Report

Merging #898 into master will increase coverage by 0.19%.
The diff coverage is 44.44%.

@@             Coverage Diff              @@
##             master     #898      +/-   ##
============================================
+ Coverage     66.89%   67.09%   +0.19%     
+ Complexity      757      748       -9     
============================================
  Files           170      170              
  Lines          3157     3130      -27     
  Branches        370      358      -12     
============================================
- Hits           2112     2100      -12     
+ Misses          890      877      -13     
+ Partials        155      153       -2

[mattnworb]

LGTM overall, thanks for doing this. I'd like to get @davidxia's opinion too when he has a chance to look at this.

I guess there were no existing tests for the serializers in ObjectMapperProvider? that is unfortunate.

[flozano]

One Q, when is this going to be merged and released?

[davidxia]

Would this need to be a major version bump because we're changing Map to Set here?

[johnflavin]

I think you’re right. I had thought that I kept the API the same and just shuffled things around internally, but this is an API change.

Should I bump this line and the version in pom.xml up to 8.10.0?

[johnflavin]

I guess, regardless of what I change the version to, I cannot make it 8.9.2. That version has already been released.

[davidxia]

Thanks. Don't know why I did this.

[davidxia]

Just wanted to point out that the behavior is different here. This replaces whereas the previous version was cumulative. I prefer this one, but it would be backwards incompatible. We should point this out in the changelog if we merge this.

[johnflavin]

No, this is the same. The old code had a method with the same signature which also replaced the volumes.

This method has been removed, but had the same signature as the new abstract method. I can show that it did replace and not accumulate. It was explicitly implemented to turned the varargs into a Set and pass it to another method:

/**		 
 * @deprecated  As of release 7.0.0, replaced by {@link #volumes(Map)}.
 */		
@Deprecated		
public Builder volumes(final String... volumes) {		
  if (volumes != null && volumes.length > 0) {		
    volumes(ImmutableSet.copyOf(volumes));		
  }		
  return this;
}

Just from that we can't tell if it replaced or accumulated. But we can look at that second Builder method, the one that took a Set and turned it into a Map<String, Map>:

/**
 * @deprecated  As of release 7.0.0, replaced by {@link #volumes(Map)}.		
 */		
@Deprecated		
public Builder volumes(final Set<String> volumes) {		
  if (volumes != null && !volumes.isEmpty()) {		
    final ImmutableMap.Builder<String, Map> volumesBuilder = ImmutableMap.builder();		
    for (final String volume : volumes) {		
      volumesBuilder.put(volume, new HashMap());		
    }		
    volumes(volumesBuilder.build());		
  }		
  return this;		
}

This passes the Map<String, Map> into the abstract Builder method that replaces the volumes. So the whole chain, from volumes(String... volumes), all were replacing any existing contents.

The new method does the same.

[johnflavin]

There was a method that accumulated: addVolumes(String...). That one got removed. I'll add it back.

I'll also add back what had been the primary Builder method for volumes: volumes(Map<String, Map>). I removed that, but I should have deprecated it.

[davidxia]

@johnflavin Thanks so much. See comments above.

[davidxia]

Can you also add this commit for testing right before the commit where you remove the null checks?

I removed some null checks in other autovalue classes and found the deserialization sometimes changes from the attribute being null to being an empty map. See https://github.com/spotify/docker-client/pull/925/files#r150894093

[johnflavin]

Summary of recent changes:

• Rebased on master.
• Bumped version (in CHANGELOG, not yet in pom) to 8.10.0 in response to comment.
• Noted breaking API change in CHANGELOG. ibid.
• Added back two ContainerConfig.Builder methods that had been removed: addVolumes(String...) which accumulates volumes, and a deprecated volumes(Map<String, Map>). See comment.

[davidxia]

Thanks for the fixes.