Repository navigation
Network and update-container commands accept a missing (null) resource ID without a local, named error #2702
Description
Location (docker-java/docker-java @ default branch 002b6eb):
docker-java-core/src/main/java/com/github/dockerjava/core/command/RemoveNetworkCmdImpl.java:10-24docker-java-core/src/main/java/com/github/dockerjava/core/command/ConnectToNetworkCmdImpl.java:39-49docker-java-core/src/main/java/com/github/dockerjava/core/command/DisconnectFromNetworkCmdImpl.java:40docker-java-core/src/main/java/com/github/dockerjava/core/command/DisconnectFromNetworkCmdImpl.java:46docker-java-core/src/main/java/com/github/dockerjava/core/command/InpectNetworkCmdImpl.java:21docker-java-core/src/main/java/com/github/dockerjava/core/command/UpdateContainerCmdImpl.java:116-136docker-java-core/src/main/java/com/github/dockerjava/core/exec/RemoveNetworkCmdExec.java:20-21
// RemoveNetworkCmdImpl.java
@Override
public RemoveNetworkCmd withNetworkId(String networkId) {
this.networkId = networkId;
return this;
}
// RemoveNetworkCmdExec.java
WebTarget webTarget = getBaseResource().path("/networks/" + command.getNetworkId());Problem
When a caller forgets to set a resource ID (e.g. a networkId that came back null from earlier code), most docker-java commands fail immediately with a clear message, for example StartContainerCmdImpl.withContainerId:
this.containerId = Objects.requireNonNull(containerId, "containerId was not specified");A few commands don't do this, so the mistake is not reported where it was made and the eventual failure does not name the missing parameter. These setters store null as-is, even though the API interfaces annotate the parameters @Nonnull (e.g. ConnectToNetworkCmd): RemoveNetworkCmdImpl.withNetworkId (also used by its constructor, i.e. removeNetworkCmd(null)), ConnectToNetworkCmdImpl.withNetworkId / withContainerId, DisconnectFromNetworkCmdImpl.withNetworkId / withContainerId, InpectNetworkCmdImpl.withNetworkId and UpdateContainerCmdImpl.withContainerId (also via its constructor).
The null value is only used later when the command executes, e.g. RemoveNetworkCmdExec builds the target with "/networks/" + command.getNetworkId(), i.e. the string "/networks/null". Whatever happens next (daemon error, transport error) does not say that networkId was never set.
Calls with valid IDs are unaffected, and this does not claim any request with a null ID reaches a daemon or affects resources; it is purely about where and how the caller mistake is reported.
Reproduction
dockerClient.removeNetworkCmd(null); // accepted: no local error naming networkIdRead from the code at the commit below; no failing output is attached because this is a diagnostic improvement, not a test failure.
Environment: commit 002b6eb7aa2b892be729c7c2a5e95f782924db38 (main).
Workaround: callers can add their own Objects.requireNonNull before building each command.
Already implemented in docker/docker-py
PR: docker/docker-py#564 — "Added check_resource decorator" (merged 2015-04-27T18:29:05Z, merge commit 881a364e9da). It adds a check_resource decorator that raises a named NullResource error locally when the container/image argument is None, and applies it across the client's resource methods (one call site shown). docker-py is another client for the same Docker API, so this is precedent, not an obligation.
How docker-py fixed it:
--- a/docker/utils/decorators.py
+++ b/docker/utils/decorators.py
@@ -0,0 +1,13 @@
+import docker.errors
+
+
+def check_resource(f):
+ def wrapped(self, resource_id=None, *args, **kwargs):
+ if resource_id is None and (
+ kwargs.get('container') is None and kwargs.get('image') is None
+ ):
+ raise docker.errors.NullResource(
+ 'image or container param is None'
+ )
+ return f(self, resource_id, *args, **kwargs)
+ return wrapped
--- a/docker/errors.py
+++ b/docker/errors.py
@@ -78,3 +78,7 @@ def __str__(self):
"client configurations. See "
"http://docs.docker.com/examples/https/ for "
"API details.")
+
+
+class NullResource(DockerException, ValueError):
+ pass
--- a/docker/client.py
+++ b/docker/client.py
@@ -249,6 +250,7 @@ def _multiplexed_response_stream_helper(self, response):
def api_version(self):
return self._version
+ @check_resource
def attach(self, container, stdout=True, stderr=True,Suggested fix
Validate the IDs in these setters with Objects.requireNonNull(..., "<field> was not specified"), the same check StartContainerCmdImpl.withContainerId uses; the RemoveNetworkCmdImpl and UpdateContainerCmdImpl constructors go through the setters, so removeNetworkCmd(null) and updateContainerCmd(null) fail immediately too:
diff --git a/docker-java-core/src/main/java/com/github/dockerjava/core/command/ConnectToNetworkCmdImpl.java b/docker-java-core/src/main/java/com/github/dockerjava/core/command/ConnectToNetworkCmdImpl.java
--- a/docker-java-core/src/main/java/com/github/dockerjava/core/command/ConnectToNetworkCmdImpl.java
+++ b/docker-java-core/src/main/java/com/github/dockerjava/core/command/ConnectToNetworkCmdImpl.java
@@ -1,5 +1,7 @@
package com.github.dockerjava.core.command;
+import java.util.Objects;
+
import com.fasterxml.jackson.annotation.JsonIgnore;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.github.dockerjava.api.command.ConnectToNetworkCmd;
@@ -38,13 +40,13 @@ public class ConnectToNetworkCmdImpl extends AbstrDockerCmd<ConnectToNetworkCmd,
@Override
public ConnectToNetworkCmd withNetworkId(String networkId) {
- this.networkId = networkId;
+ this.networkId = Objects.requireNonNull(networkId, "networkId was not specified");
return this;
}
@Override
public ConnectToNetworkCmd withContainerId(String containerId) {
- this.containerId = containerId;
+ this.containerId = Objects.requireNonNull(containerId, "containerId was not specified");
return this;
}
diff --git a/docker-java-core/src/main/java/com/github/dockerjava/core/command/DisconnectFromNetworkCmdImpl.java b/docker-java-core/src/main/java/com/github/dockerjava/core/command/DisconnectFromNetworkCmdImpl.java
--- a/docker-java-core/src/main/java/com/github/dockerjava/core/command/DisconnectFromNetworkCmdImpl.java
+++ b/docker-java-core/src/main/java/com/github/dockerjava/core/command/DisconnectFromNetworkCmdImpl.java
@@ -1,5 +1,7 @@
package com.github.dockerjava.core.command;
+import java.util.Objects;
+
import com.fasterxml.jackson.annotation.JsonIgnore;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.github.dockerjava.api.command.DisconnectFromNetworkCmd;
@@ -38,13 +40,13 @@ public class DisconnectFromNetworkCmdImpl extends AbstrDockerCmd<DisconnectFromN
@Override
public DisconnectFromNetworkCmd withNetworkId(String networkId) {
- this.networkId = networkId;
+ this.networkId = Objects.requireNonNull(networkId, "networkId was not specified");
return this;
}
@Override
public DisconnectFromNetworkCmd withContainerId(String containerId) {
- this.containerId = containerId;
+ this.containerId = Objects.requireNonNull(containerId, "containerId was not specified");
return this;
}
diff --git a/docker-java-core/src/main/java/com/github/dockerjava/core/command/InpectNetworkCmdImpl.java b/docker-java-core/src/main/java/com/github/dockerjava/core/command/InpectNetworkCmdImpl.java
--- a/docker-java-core/src/main/java/com/github/dockerjava/core/command/InpectNetworkCmdImpl.java
+++ b/docker-java-core/src/main/java/com/github/dockerjava/core/command/InpectNetworkCmdImpl.java
@@ -1,5 +1,7 @@
package com.github.dockerjava.core.command;
+import java.util.Objects;
+
import com.github.dockerjava.api.command.DockerCmdSyncExec;
import com.github.dockerjava.api.command.InspectNetworkCmd;
import com.github.dockerjava.api.model.Network;
@@ -19,8 +21,7 @@ public class InpectNetworkCmdImpl extends AbstrDockerCmd<InspectNetworkCmd, Netw
@Override
public InspectNetworkCmd withNetworkId(String networkId) {
-
- this.networkId = networkId;
+ this.networkId = Objects.requireNonNull(networkId, "networkId was not specified");
return this;
}
}
diff --git a/docker-java-core/src/main/java/com/github/dockerjava/core/command/RemoveNetworkCmdImpl.java b/docker-java-core/src/main/java/com/github/dockerjava/core/command/RemoveNetworkCmdImpl.java
--- a/docker-java-core/src/main/java/com/github/dockerjava/core/command/RemoveNetworkCmdImpl.java
+++ b/docker-java-core/src/main/java/com/github/dockerjava/core/command/RemoveNetworkCmdImpl.java
@@ -1,5 +1,7 @@
package com.github.dockerjava.core.command;
+import java.util.Objects;
+
import com.github.dockerjava.api.command.DockerCmdSyncExec;
import com.github.dockerjava.api.command.RemoveNetworkCmd;
@@ -19,7 +21,7 @@ public class RemoveNetworkCmdImpl extends AbstrDockerCmd<RemoveNetworkCmd, Void>
@Override
public RemoveNetworkCmd withNetworkId(String networkId) {
- this.networkId = networkId;
+ this.networkId = Objects.requireNonNull(networkId, "networkId was not specified");
return this;
}
}
diff --git a/docker-java-core/src/main/java/com/github/dockerjava/core/command/UpdateContainerCmdImpl.java b/docker-java-core/src/main/java/com/github/dockerjava/core/command/UpdateContainerCmdImpl.java
--- a/docker-java-core/src/main/java/com/github/dockerjava/core/command/UpdateContainerCmdImpl.java
+++ b/docker-java-core/src/main/java/com/github/dockerjava/core/command/UpdateContainerCmdImpl.java
@@ -19,6 +19,7 @@ import org.apache.commons.lang3.builder.ToStringBuilder;
import javax.annotation.CheckForNull;
import javax.annotation.Nonnull;
import java.util.List;
+import java.util.Objects;
/**
* @author Kanstantsin Shautsou
@@ -131,7 +132,7 @@ public class UpdateContainerCmdImpl extends AbstrDockerCmd<UpdateContainerCmd, U
* @see #containerId
*/
public UpdateContainerCmd withContainerId(@Nonnull String containerId) {
- this.containerId = containerId;
+ this.containerId = Objects.requireNonNull(containerId, "containerId was not specified");
return this;
}
Verified on 002b6eb (OpenJDK 21.0.2, Maven 3.9.16): a test that passes null to each of these setters/constructors and expects a NullPointerException fails without this patch (all 7 cases) and passes with it; no other test results change (mvn -pl docker-java test, which excludes the integration tests that need a Docker daemon).
I can submit a PR for this.
Checked: searches for requireNonNull networkId, null networkId, "was not specified" network found no existing report; hits such as #1940 (null map key in network aliases) are unrelated.
Activity