Fix serialization of sets

CHANGELOG.md

@@ -1,5 +1,15 @@
 # Changelog
 
+## 8.10.0
+
+Not yet released.
+
+- `ContainerConfig.exposedPorts` is serialized as a map of keys to empty map placeholders. `exposedPorts` had been serialized as keys -> `null`. (Fixes [893][])
+- Breaking API change: The method `ContainerConfig.volumes()` has changed signature from `ImmutableMap<String, Map> volumes()` (where the second `Map` was always empty) to `ImmutableSet<String> volumes()`. This change simplifies the code internally. ([898][])
+
+[893]: https://github.com/spotify/docker-client/issues/893
+[898]: https://github.com/spotify/docker-client/issues/898
+
 ## 8.9.2
 
 Released November 15, 2017

src/main/java/com/spotify/docker/client/ObjectMapperProvider.java

@@ -36,6 +36,7 @@
 import com.google.common.collect.Maps;
 
 import java.io.IOException;
+import java.util.Collections;
 import java.util.Map;
 import java.util.Set;
 
@@ -54,10 +55,10 @@ public class ObjectMapperProvider implements ContextResolver<ObjectMapper> {
 
   private static final Logger log = LoggerFactory.getLogger(ObjectMapperProvider.class);
 
-  private static final Function<? super Object, ?> VOID_VALUE = new Function<Object, Object>() {
+  private static final Function<? super Object, ?> EMPTY_MAP = new Function<Object, Object>() {
     @Override
     public Object apply(final Object input) {
-      return null;
+      return Collections.emptyMap();
     }
   };
 
@@ -95,7 +96,7 @@ private static class SetSerializer extends JsonSerializer<Set> {
     @Override
     public void serialize(final Set value, final JsonGenerator jgen,
                           final SerializerProvider provider) throws IOException {
-      final Map map = (value == null) ? null : Maps.asMap(value, VOID_VALUE);
+      final Map map = (value == null) ? null : Maps.asMap(value, EMPTY_MAP);
       OBJECT_MAPPER.writeValue(jgen, map);
     }
   }
@@ -115,7 +116,7 @@ private static class ImmutableSetSerializer extends JsonSerializer<ImmutableSet>
     @Override
     public void serialize(final ImmutableSet value, final JsonGenerator jgen,
                           final SerializerProvider provider) throws IOException {
-      final Map map = (value == null) ? null : Maps.asMap(value, VOID_VALUE);
+      final Map map = (value == null) ? null : Maps.asMap(value, EMPTY_MAP);
       OBJECT_MAPPER.writeValue(jgen, map);
     }
   }

src/main/java/com/spotify/docker/client/messages/ContainerConfig.java

@@ -100,15 +100,17 @@ public abstract class ContainerConfig {
   @JsonProperty("Image")
   public abstract String image();
 
-  @SuppressFBWarnings("NP_NULL_ON_SOME_PATH")
+  /**
+   * @deprecated As of 8.10.0, use {@link #volumes()}.
+   */
+  @Deprecated
   public Set<String> volumeNames() {
-    //noinspection ConstantConditions
-    return volumes() == null ? Collections.<String>emptySet() : volumes().keySet();
+    return volumes();
   }
 
   @Nullable
   @JsonProperty("Volumes")
-  public abstract ImmutableMap<String, Map> volumes();
+  public abstract ImmutableSet<String> volumes();
 
   @Nullable
   @JsonProperty("WorkingDir")
@@ -174,7 +176,7 @@ static ContainerConfig create(
       @JsonProperty("Env") final List<String> env,
       @JsonProperty("Cmd") final List<String> cmd,
       @JsonProperty("Image") final String image,
-      @JsonProperty("Volumes") final Map<String, Map> volumes,
+      @JsonProperty("Volumes") final Set<String> volumes,
       @JsonProperty("WorkingDir") final String workingDir,
       @JsonProperty("Entrypoint") final List<String> entrypoint,
       @JsonProperty("NetworkDisabled") final Boolean networkDisabled,
@@ -185,7 +187,7 @@ static ContainerConfig create(
       @JsonProperty("StopSignal") final String stopSignal,
       @JsonProperty("Healthcheck") final Healthcheck healthcheck,
       @JsonProperty("NetworkingConfig") final NetworkingConfig networkingConfig) {
-    final Builder builder = builder()
+    return builder()
         .hostname(hostname)
         .domainname(domainname)
         .user(user)
@@ -201,37 +203,17 @@ static ContainerConfig create(
         .macAddress(macAddress)
         .hostConfig(hostConfig)
         .stopSignal(stopSignal)
-        .networkingConfig(networkingConfig);
-
-    if (portSpecs != null) {
-      builder.portSpecs(portSpecs);
-    }
-    if (exposedPorts != null) {
-      builder.exposedPorts(exposedPorts);
-    }
-    if (env != null) {
-      builder.env(env);
-    }
-    if (cmd != null) {
-      builder.cmd(cmd);
-    }
-    if (volumes != null)  {
-      builder.volumes(volumes);
-    }
-    if (entrypoint != null) {
-      builder.entrypoint(entrypoint);
-    }
-    if (onBuild != null) {
-      builder.onBuild(onBuild);
-    }
-    if (labels != null) {
-      builder.labels(labels);
-    }
-    if (healthcheck != null) {
-      builder.healthcheck(healthcheck);
-    }
-
-    return builder.build();
+        .networkingConfig(networkingConfig)
+        .volumes(volumes)
+        .portSpecs(portSpecs)
+        .exposedPorts(exposedPorts)
+        .env(env)
+        .cmd(cmd)
+        .entrypoint(entrypoint)
+        .onBuild(onBuild)
+        .labels(labels)
+        .healthcheck(healthcheck)
+        .build();
   }
 
   public abstract Builder toBuilder();
@@ -279,47 +261,33 @@ public abstract static class Builder {
 
     public abstract Builder image(final String image);
 
-    abstract ImmutableMap.Builder<String, Map> volumesBuilder();
+    abstract ImmutableSet.Builder<String> volumesBuilder();
 
     public Builder addVolume(final String volume) {
-      volumesBuilder().put(volume, new HashMap());
+      volumesBuilder().add(volume);
       return this;
     }
 
     public Builder addVolumes(final String... volumes) {
       for (final String volume : volumes) {
-        volumesBuilder().put(volume, new HashMap());
+        volumesBuilder().add(volume);
       }
       return this;
     }
 
-    public abstract Builder volumes(final Map<String, Map> volumes);
-
     /**
-     * @deprecated  As of release 7.0.0, replaced by {@link #volumes(Map)}.
+     * @deprecated As of 8.10.0, use {@link #volumes(Set)} or
+     *             {@link #volumes(String...)}.
      */
     @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());
-      }
+    public Builder volumes(final Map<String, Map> volumes) {
+      this.volumes(volumes.keySet());
       return this;
     }
 
-    /**
-     * @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;
-    }
+    public abstract Builder volumes(final Set<String> volumes);
+
+    public abstract Builder volumes(final String... volumes);
 
     public abstract Builder workingDir(final String workingDir);
 

src/test/java/com/spotify/docker/client/DefaultDockerClientTest.java

@@ -2733,7 +2733,7 @@ public void testContainerVolumesOldStyle() throws Exception {
     final String expectedLocalPath = "/local/path";
     assertThat(volumeContainer.volumes().values(), hasItem(containsString(expectedLocalPath)));
 
-    assertThat(volumeContainer.config().volumeNames(), hasItem("/foo"));
+    assertThat(volumeContainer.config().volumes(), hasItem("/foo"));
   }
 
   @Test
@@ -2990,7 +2990,7 @@ public boolean apply(ContainerMount mount) {
       }
     }
 
-    assertThat(containerInfo.config().volumeNames(), hasItem(anonVolumeTo));
+    assertThat(containerInfo.config().volumes(), hasItem(anonVolumeTo));
   }
 
   @Test

src/test/java/com/spotify/docker/client/messages/ContainerConfigTest.java

@@ -21,6 +21,9 @@
 package com.spotify.docker.client.messages;
 
 import static com.spotify.docker.FixtureUtil.fixture;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.nullValue;
 
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.spotify.docker.client.ObjectMapperProvider;
@@ -46,4 +49,18 @@ public void test1_29() throws Exception {
     objectMapper.readValue(fixture("fixtures/1.29/containerConfig.json"), ContainerConfig.class);
   }
 
+  @Test
+  public void test1_29_WithoutNullables() throws Exception {
+    final ContainerConfig config = objectMapper.readValue(
+        fixture("fixtures/1.29/containerConfigWithoutNullables.json"), ContainerConfig.class);
+    assertThat(config.portSpecs(), is(nullValue()));
+    assertThat(config.exposedPorts(), is(nullValue()));
+    assertThat(config.env(), is(nullValue()));
+    assertThat(config.cmd(), is(nullValue()));
+    assertThat(config.entrypoint(), is(nullValue()));
+    assertThat(config.onBuild(), is(nullValue()));
+    assertThat(config.labels(), is(nullValue()));
+    assertThat(config.healthcheck(), is(nullValue()));
+  }
+
 }

src/test/resources/fixtures/1.29/containerConfigWithoutNullables.json

@@ -0,0 +1,24 @@
+{
+  "Hostname": "string",
+  "Domainname": "string",
+  "User": "string",
+  "AttachStdin": false,
+  "AttachStdout": true,
+  "AttachStderr": true,
+  "Tty": false,
+  "OpenStdin": false,
+  "StdinOnce": false,
+  "ArgsEscaped": true,
+  "Image": "string",
+  "Volumes": {
+    "additionalProperties": {}
+  },
+  "WorkingDir": "string",
+  "NetworkDisabled": true,
+  "MacAddress": "string",
+  "StopSignal": "SIGTERM",
+  "StopTimeout": 10,
+  "Shell": [
+    "string"
+  ]
+}