From 47bc7c206146b943dbeb09a178082bea8df073b8 Mon Sep 17 00:00:00 2001 From: Thomas Vitale Date: Thu, 16 Jul 2026 22:24:03 +0200 Subject: [PATCH] chore: Add missing JSpecify annotations to docling-testcontainers * Add missing JSpecify annotations. * Fix bug affecting the containerEnv map being immutable when adding more variables (e.g. after calling the toBuilder() method). * Add unit tests for the config builder. Signed-off-by: Thomas Vitale --- .../serve/DoclingServeContainer.java | 12 +-- .../config/DefaultDoclingContainerConfig.java | 2 +- .../config/DoclingServeContainerConfig.java | 58 ++++++++---- .../DoclingServeContainerConfigTests.java | 91 +++++++++++++++++++ 4 files changed, 140 insertions(+), 23 deletions(-) create mode 100644 docling-testcontainers/src/test/java/ai/docling/testcontainers/serve/config/DoclingServeContainerConfigTests.java diff --git a/docling-testcontainers/src/main/java/ai/docling/testcontainers/serve/DoclingServeContainer.java b/docling-testcontainers/src/main/java/ai/docling/testcontainers/serve/DoclingServeContainer.java index 10909c2e..0b0c8ebd 100644 --- a/docling-testcontainers/src/main/java/ai/docling/testcontainers/serve/DoclingServeContainer.java +++ b/docling-testcontainers/src/main/java/ai/docling/testcontainers/serve/DoclingServeContainer.java @@ -44,19 +44,19 @@ public DoclingServeContainer(DoclingServeContainerConfig config) { this.config = config; // Configure the container - withExposedPorts(DEFAULT_DOCLING_PORT); - withEnv(config.containerEnv()); - waitingFor(Wait.forHttp("/health")); + this.withExposedPorts(DEFAULT_DOCLING_PORT); + this.withEnv(config.containerEnv()); + this.waitingFor(Wait.forHttp("/health")); if (config.enableUi()) { - withEnv("DOCLING_SERVE_ENABLE_UI", "true"); + this.withEnv("DOCLING_SERVE_ENABLE_UI", "true"); } - withStartupTimeout(config.startupTimeout()); + this.withStartupTimeout(config.startupTimeout()); Optional.ofNullable(config.apiKey()) .map(String::strip) - .ifPresent(apiKey -> withEnv("DOCLING_SERVE_API_KEY", apiKey)); + .ifPresent(apiKey -> this.withEnv("DOCLING_SERVE_API_KEY", apiKey)); } /** diff --git a/docling-testcontainers/src/main/java/ai/docling/testcontainers/serve/config/DefaultDoclingContainerConfig.java b/docling-testcontainers/src/main/java/ai/docling/testcontainers/serve/config/DefaultDoclingContainerConfig.java index f9503fab..02d0d29c 100644 --- a/docling-testcontainers/src/main/java/ai/docling/testcontainers/serve/config/DefaultDoclingContainerConfig.java +++ b/docling-testcontainers/src/main/java/ai/docling/testcontainers/serve/config/DefaultDoclingContainerConfig.java @@ -17,7 +17,7 @@ final class DefaultDoclingContainerConfig implements DoclingServeContainerConfig private final String apiKey; DefaultDoclingContainerConfig(Builder builder) { - if ((builder.image == null) || builder.image.strip().isEmpty()) { + if ((builder.image == null) || builder.image.isBlank()) { throw new IllegalArgumentException("image must be specified"); } diff --git a/docling-testcontainers/src/main/java/ai/docling/testcontainers/serve/config/DoclingServeContainerConfig.java b/docling-testcontainers/src/main/java/ai/docling/testcontainers/serve/config/DoclingServeContainerConfig.java index 2633142d..7e80d104 100644 --- a/docling-testcontainers/src/main/java/ai/docling/testcontainers/serve/config/DoclingServeContainerConfig.java +++ b/docling-testcontainers/src/main/java/ai/docling/testcontainers/serve/config/DoclingServeContainerConfig.java @@ -1,7 +1,9 @@ package ai.docling.testcontainers.serve.config; import java.time.Duration; +import java.util.HashMap; import java.util.Map; +import java.util.Objects; import org.jspecify.annotations.Nullable; @@ -106,8 +108,13 @@ class Builder { * The name of the container image to be used in the configuration. * This variable holds the identifier or tag of the container image * that will be utilized when creating or configuring container instances. - * It is a required field and must be set to a non-null, non-empty value. + *

+ * It may be {@code null} while the builder is still being configured, but a + * non-null, non-empty, non-blank value is required by the time {@link #build()} is called; + * otherwise {@link #build()} throws {@link IllegalArgumentException}. + *

*/ + @Nullable protected String image; /** @@ -122,13 +129,16 @@ class Builder { * A map representing the environment variables to be passed to the container. * The keys in the map represent the names of the environment variables, * and the corresponding values represent their assigned values. - * + *

* This field is used to configure additional environment-specific settings * during the container's initialization and execution. - * - * Modifications to this map may be reflected in the container's runtime environment, - * affecting its behavior as defined by the provided environment variables. + *

+ *

+ * Mutations to a map passed into {@link #containerEnv(Map)} are not reflected, as the builder makes a defensive copy. + * Use {@link #containerEnv(String, String)} to add variables to this builder instance. + *

*/ + @Nullable protected Map containerEnv; /** @@ -140,6 +150,7 @@ class Builder { * allows precise specification of time intervals. *

*/ + @Nullable protected Duration startupTimeout; /** @@ -169,18 +180,18 @@ protected Builder() { protected Builder(DoclingServeContainerConfig config) { this.image = config.image(); this.enableUi = config.enableUi(); - this.containerEnv = config.containerEnv(); + this.containerEnv = new HashMap<>(config.containerEnv()); this.startupTimeout = config.startupTimeout(); this.apiKey = config.apiKey(); } /** * Sets the name of the container image to use for the configuration. - * This value is required and must not be null or empty. + * This value is required and must not be null, empty, or blank. * * @param image the name of the container image * @return the builder instance for method chaining - * @throws IllegalArgumentException if the provided image is null or empty + * @throws IllegalArgumentException if the provided image is null, empty, or blank */ public Builder image(String image) { this.image = image; @@ -200,12 +211,22 @@ public Builder enableUi(boolean enableUi) { /** * Sets the environment variables to be passed to the container. + *

+ * The provided map is defensively copied, so later external mutations do not + * affect this builder. Passing {@code null} clears any previously configured + * environment variables, and the built configuration falls back to an empty map. + *

* - * @param containerEnv a map containing the environment variable names and their corresponding values + * @param containerEnv a map containing the environment variable names and their corresponding values, + * or {@code null} to clear the environment variables * @return the builder instance for method chaining */ - public Builder containerEnv(Map containerEnv) { - this.containerEnv = containerEnv; + public Builder containerEnv(@Nullable Map containerEnv) { + if (containerEnv != null) { + this.containerEnv = new HashMap<>(containerEnv); + } else { + this.containerEnv = null; + } return this; } @@ -214,12 +235,17 @@ public Builder containerEnv(Map containerEnv) { * This method allows setting a single key-value pair representing * an environment variable and its value to be passed to the container. * - * @param key the name of the environment variable - * @param value the value of the environment variable + * @param key the name of the environment variable; must not be null + * @param value the value of the environment variable; must not be null * @return the builder instance for method chaining * @throws NullPointerException if the key or value is null */ public Builder containerEnv(String key, String value) { + Objects.requireNonNull(key, "key must not be null"); + Objects.requireNonNull(value, "value must not be null"); + if (this.containerEnv == null) { + this.containerEnv = new HashMap<>(); + } this.containerEnv.put(key, value); return this; } @@ -238,11 +264,11 @@ public Builder startupTimeout(Duration startupTimeout) { /** * Sets the API key to be used for the container configuration. * - * @param apiKey the API key as a string; this value is used to authenticate - * or authorize the container's operations + * @param apiKey the API key as a string, used to authenticate or authorize the + * container's operations, or {@code null} to omit the API key * @return the builder instance for method chaining */ - public Builder apiKey(String apiKey) { + public Builder apiKey(@Nullable String apiKey) { this.apiKey = apiKey; return this; } diff --git a/docling-testcontainers/src/test/java/ai/docling/testcontainers/serve/config/DoclingServeContainerConfigTests.java b/docling-testcontainers/src/test/java/ai/docling/testcontainers/serve/config/DoclingServeContainerConfigTests.java new file mode 100644 index 00000000..637630cb --- /dev/null +++ b/docling-testcontainers/src/test/java/ai/docling/testcontainers/serve/config/DoclingServeContainerConfigTests.java @@ -0,0 +1,91 @@ +package ai.docling.testcontainers.serve.config; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; +import static org.assertj.core.api.Assertions.assertThatNullPointerException; + +import java.util.HashMap; +import java.util.Map; +import java.util.concurrent.atomic.AtomicReference; + +import org.junit.jupiter.api.Test; + +class DoclingServeContainerConfigTests { + + @Test + void toBuilderPreservesExistingContainerEnv() { + var original = DoclingServeContainerConfig.builder() + .image(DoclingServeContainerConfig.DOCLING_IMAGE) + .containerEnv(Map.of("FIRST", "1")) + .build(); + + var copy = original.toBuilder().build(); + + assertThat(copy.containerEnv()).containsExactlyInAnyOrderEntriesOf(Map.of("FIRST", "1")); + } + + @Test + void toBuilderAllowsAddingContainerEnvWithoutMutatingOriginal() { + var original = DoclingServeContainerConfig.builder() + .image(DoclingServeContainerConfig.DOCLING_IMAGE) + .containerEnv(Map.of("FIRST", "1")) + .build(); + + var updated = new AtomicReference(); + assertThatCode(() -> updated.set(original.toBuilder() + .containerEnv("SECOND", "2") + .build())) + .doesNotThrowAnyException(); + + assertThat(updated.get().containerEnv()) + .containsExactlyInAnyOrderEntriesOf(Map.of("FIRST", "1", "SECOND", "2")); + + // The original configuration must remain untouched by the builder copy. + assertThat(original.containerEnv()).containsExactlyInAnyOrderEntriesOf(Map.of("FIRST", "1")); + } + + @Test + void containerEnvMakesDefensiveCopyOfCallerMap() { + var source = new HashMap(); + source.put("FIRST", "1"); + + var config = DoclingServeContainerConfig.builder() + .image(DoclingServeContainerConfig.DOCLING_IMAGE) + .containerEnv(source) + .build(); + + // Mutating the caller-provided map after the call must not leak into the config. + source.put("SECOND", "2"); + source.remove("FIRST"); + + assertThat(config.containerEnv()).containsExactlyInAnyOrderEntriesOf(Map.of("FIRST", "1")); + } + + @Test + void containerEnvKeyValueRejectsNullKeyFailFast() { + var builder = DoclingServeContainerConfig.builder(); + + assertThatNullPointerException().isThrownBy(() -> builder.containerEnv(null, "value")); + } + + @Test + void containerEnvKeyValueRejectsNullValueFailFast() { + var builder = DoclingServeContainerConfig.builder(); + + assertThatNullPointerException().isThrownBy(() -> builder.containerEnv("key", null)); + } + + @Test + void containerEnvNullClearsToEmptyMapWithoutThrowing() { + var config = new AtomicReference(); + assertThatCode(() -> config.set(DoclingServeContainerConfig.builder() + .image(DoclingServeContainerConfig.DOCLING_IMAGE) + .containerEnv("FIRST", "1") + .containerEnv(null) + .build())) + .doesNotThrowAnyException(); + + assertThat(config.get().containerEnv()).isEmpty(); + } + +}