Skip to content

feat: add Configuration.withOptions for copy-on-write derivation#158

Closed
OmarAlJarrah wants to merge 1 commit into
mainfrom
feat/withoptions-immutable-client-config
Closed

feat: add Configuration.withOptions for copy-on-write derivation#158
OmarAlJarrah wants to merge 1 commit into
mainfrom
feat/withoptions-immutable-client-config

Conversation

@OmarAlJarrah

Copy link
Copy Markdown
Member

Summary

There was no ergonomic way to derive a lightly-reconfigured client configuration. This adds a copy-on-write derivation API on the existing layered Configuration, which is the SDK's single client-config type, settling the open question of whether to introduce a unified cloneable config: it gains the clone-and-reconfigure entry points rather than spawning a parallel type.

Changes

  • Configuration.withOptions(Consumer<ConfigurationBuilder>) — applies a builder mutator to a copy prefilled from the receiver and returns a new immutable Configuration. The original is left unchanged.
  • Configuration.toBuilder() — returns the same prefilled ConfigurationBuilder for callers that prefer to thread it through other builder-folding code before build().
  • ConfigurationBuilder now implements Builder<Configuration> and has a constructor preloaded from an existing Configuration.

Immutability semantics (decision note)

Derivation is copy-on-write in the value sense: the override map is copied up front, so the original and the derived instance never alias the same mutable state. The env / system-property lookup seams are shared by reference because they are pure read functions and are never mutated. An empty mutator yields an independent instance equal in behaviour to the original. This is documented in the Configuration KDoc.

Tests

Added coverage in ConfigurationTest proving: derived copies pick up new and overridden keys; the original is never mutated (including the two-independent-derivations case); inherited env/property seams still resolve on the copy; and toBuilder() is independent of the source after the fact.

Gated build (scoped, run locally)

./gradlew :sdk-core:test :sdk-core:ktlintCheck :sdk-core:detekt --no-daemon   # BUILD SUCCESSFUL
./gradlew :sdk-core:apiDump --no-daemon                                       # regenerated sdk-core.api (committed)
./gradlew :sdk-core:apiCheck --no-daemon                                      # BUILD SUCCESSFUL

Closes #60

Introduce an ergonomic way to derive a lightly-reconfigured Configuration
without mutating the original. Configuration.withOptions(Consumer<Builder>)
applies a builder mutator to a copy and returns a new immutable instance;
Configuration.toBuilder() exposes the same prefilled builder for callers that
prefer to thread it through other builder-folding code.

This settles the open question of unified, cloneable client config: the
layered Configuration is the single client-config type, so it gains the
clone-and-reconfigure entry points rather than introducing a parallel one.
ConfigurationBuilder now implements Builder<Configuration> and gains a
constructor preloaded from an existing Configuration.

Derivation is copy-on-write in the value sense: the override map is copied so
the original and the derived instance never alias mutable state, while the
env/system-property lookup seams are shared by reference (they are pure read
functions and never mutated). The immutability semantics are documented in the
Configuration KDoc.

Closes #60
@OmarAlJarrah

Copy link
Copy Markdown
Member Author

This adds copy-on-write derivation to the runtime Configuration via withOptions(Consumer<ConfigurationBuilder>) and a prefilled-builder accessor, with the override map defensively copied and the env/props seams shared by reference. The implementation is correct and well-tested (add/override/inherit-seam/no-op/independence are all covered) and the api/sdk-core.api snapshot is regenerated. Good to merge after one naming nit.

Issues

Prefilled-builder accessor named toBuilder() instead of newBuilder()sdk-core/src/main/kotlin/org/dexpace/sdk/core/config/Configuration.kt:70. Every other immutable model in sdk-core (Request, Response, Headers, RequestConditions, RetrySettings, etc.) exposes its prefilled-builder method as newBuilder(), and the project conventions spell it out as "newBuilder() pre-filled". Naming this one toBuilder() makes Configuration the lone deviation in the public surface. It doesn't break anything, but it ships an inconsistent name into the stable api/*.api snapshot, where renaming it later would itself be a binary-compat change — cheaper to align now. Recommend renaming to newBuilder().

@OmarAlJarrah

Copy link
Copy Markdown
Member Author

Superseded by #161, which delivers the same copy-on-write derivation on Configuration and more:

  • withOptionsderive and toBuilder()newBuilder(), matching the prefilled-builder accessor every other model exposes.
  • Adds Configuration.builder() to complete the construction surface.
  • Constrains the derivation read seams (overrides / envSource / propsSource) with @get:JvmSynthetic so no Java-callable accessor leaks onto the public surface.
  • Broader tests: chained derivation, the null-mutator NPE contract, override-beats-inherited-env, source-replacement isolation, empty-env skip-to-sysprop inheritance, and derivation from an override-less base.

#60 is now tracked by #161. Closing in favour of that PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Generate withOptions(Consumer<Builder>) returning a new immutable client

1 participant