Builder KV and OS Configuration from JSON

[scottf]

Addressing #1579

[scottf]

@claude review as if the first time. Do not care about wildcard imports

[synadia-claude-reviewer]

Claude finished @scottf's task in 4m 40s —— View job

---

PR Review: Builder KV and OS Configuration from JSON

• Gather context and read changed files
• Review KeyValueConfiguration.java changes
• Review ObjectStoreConfiguration.java changes
• Review ApiConstants.java changes
• Review test changes
• Post final review

---

Summary

This PR adds instance(String json) and instanceViaStreamConfig(String json) factory methods to KeyValueConfiguration and ObjectStoreConfiguration, enabling deserialization from two distinct JSON formats: the user-facing KV/OS builder domain (simple bucket names, KV-specific field names) vs. the raw backing stream config. The design is sound and the documentation is clear about the distinction. There is one critical defect that will cause test failures, and a few secondary concerns.

---

⚠️ Duration Fields Are Nanoseconds — Worth Documenting

readNanos(v, TTL) and readNanos(v, LIMIT_MARKER_TTL) expect the JSON values to be nanosecond integers (consistent with JsonValueUtils.instance(Duration d) which calls d.toNanos()). This is the right choice for internal consistency, but it is non-obvious to users writing JSON by hand. The instance(String) Javadoc explains the format is distinct from toJson(), but doesn't mention the nanosecond convention for durations. A note like // duration fields are nanoseconds alongside the relevant readNanos calls, or a sentence in the Javadoc, would help.

---

🔍 Test Coverage Gaps in validate()

Neither validate() method asserts metadata, even though instance() deserializes it. In the KV case, mirror and sources are also deserialized but not asserted in validate(). These paths compile and appear logically correct from reading the code, but they go unexercised. If the JSON fixture files include those fields, the tests only verify that parsing doesn't throw — not that the values round-trip correctly.

---

✅ What's Good

• Design is clean: The two-method pattern (instance for builder JSON, instanceViaStreamConfig for raw stream JSON) is a clear and principled API split.
• Javadoc is thorough: Both methods are well-documented, the distinction between the two entry points is explicit, and the "use at your own risk" caveat on instanceViaStreamConfig is appropriate.
• Validation still runs: instance() routes through the builder's build() method, so all existing KV/OS validation (name format, replica limits, history limits, etc.) is preserved. The assertThrows(IllegalArgumentException.class, () -> instance("{}")) test confirms this.
• New constants in ApiConstants: LIMIT_MARKER_TTL, MAX_BUCKET_SIZE, MAX_HISTORY_PER_KEY, MAX_VALUE_SIZE, TTL are all alphabetically ordered, aligned, and correctly commented.
• Null-safety: StorageType.get(null) returns null → Builder.storageType(null) defaults to StorageType.File. readNanos returning null → ttl(null) → Duration.ZERO. These are handled correctly downstream.
• instanceViaStreamConfig test improvement: Replacing the old new KeyValueConfiguration(StreamConfiguration.instance(jvSc)) (which used a package-private constructor) with the new public API is a good cleanup.

---

| Branch: issue-1579

[scottf]

@claude re-review

[synadia-claude-reviewer]

Claude finished @scottf's task in 3m 8s —— View job

---

PR Re-Review: Builder KV and OS Configuration from JSON

• Gather context
• Read KeyValueConfiguration.java changes
• Read ObjectStoreConfiguration.java changes
• Read ApiConstants.java changes
• Read test files and JSON fixtures
• Post final review

---

Summary

The design and implementation are clean and correct. There is one failing test caused by missing fixture files. Everything else looks good.

---

🔴 Critical: Missing Test Fixture Files — testInstanceMirrorAndSources Will Fail

KeyValueConfigurationTests.testInstanceMirrorAndSources() (added in the final commit) calls:

validateSources(KeyValueConfiguration.instance(dataAsString("KeyValueConfigSources.json")));
validateMirror(KeyValueConfiguration.instance(dataAsString("KeyValueConfigMirror.json")));

Neither KeyValueConfigSources.json nor KeyValueConfigMirror.json exists under src/test/resources/data/. ResourceUtils.dataAsString calls classLoader.getResource(fileName).getFile() — when the resource is not found, .getResource() returns null and .getFile() throws a NullPointerException wrapped in a RuntimeException. The test will fail.

The two missing files should be something like:

KeyValueConfigSources.json:

{
  "name": "bucketName",
  "sources": [
    { "name": "KV_sourceBucket" }
  ]
}

KeyValueConfigMirror.json:

{
  "name": "bucketName",
  "mirror": { "name": "KV_mirrorBucket" }
}

Note: source/mirror names can be either prefixed (KV_) or unprefixed — build() auto-adds the prefix when missing via hasPrefix() / toStreamName(). The validateSources / validateMirror assertions check the post-build name ("KV_sourceBucket" / "KV_mirrorBucket"), so either form in the JSON will satisfy the test. Using the already-prefixed names in the JSON makes the fixture self-documenting about what the backing stream actually stores.

Fix this →

---

✅ Everything Else is Correct

instance() implementation — KeyValueConfiguration (KeyValueConfiguration.java:135–155): Reads each KV-domain field, routes through Builder.build() so all existing validation runs. Field-by-field:

• readBoolean(v, COMPRESSION) returns primitive false (not Boolean) when absent — no unboxing NPE.
• readNanos(v, TTL) → Duration.ofNanos(long) or null; ttl(null) downstream handles null correctly.
• readNanos(v, LIMIT_MARKER_TTL) → limitMarker(Duration) calls validateDurationNotRequiredGtOrEqSeconds, which accepts null → clears.
• Source.optionalListOf(null) (when key absent) → empty list, not null → sources(emptyList) safely clears.
• Mirror.optionalInstance(null) → null → builder mirror field stays null → build() falls through to subject-based path.

instanceViaStreamConfig() (KeyValueConfiguration.java:170–172, ObjectStoreConfiguration.java:93–95): Clean one-liner delegation to StreamConfiguration.instance(json). The "at your own risk" caveat in the Javadoc is appropriate.

ObjectStoreConfiguration.instance() (ObjectStoreConfiguration.java:65–79): Symmetric to the KV version, correct for OS's simpler field set (no mirror, sources, republish, limitMarkerTtl).

JSON fixtures vs assertions — all values in KeyValueConfig.json and ObjectStoreConfig.json correctly match what validate() asserts:

• "ttl": 777000000 → Duration.ofNanos(777_000_000) = Duration.ofMillis(777) ✓
• "limit_marker_ttl": 8888000000 → Duration.ofNanos(8_888_000_000) = Duration.ofMillis(8888) ✓
• "max_value_size": 666 (not 66666666) → getMaximumValueSize() == 666 ✓

ApiConstants additions (ApiConstants.java:113,120,127,135,238): LIMIT_MARKER_TTL, MAX_BUCKET_SIZE, MAX_HISTORY_PER_KEY, MAX_VALUE_SIZE, TTL — all correctly positioned alphabetically and correctly commented.

testConstruction() improvements: Now exercises metadata (previously untested via the JSON path), instance(), instanceViaStreamConfig(), and the invalid-JSON / missing-name error cases. The assertThrows(IllegalArgumentException.class, () -> instance("{}")) confirms builder validation still runs. Good coverage.

testInstanceMirrorAndSources() (once the fixture files exist): Correctly covers mirror and sources independently, and exercises both instance() and instanceViaStreamConfig() for each — good symmetry.

---

Verdict

One test will fail due to the two missing JSON files. Everything else — design, implementation, documentation, and existing tests — is solid. Add the two fixture files and this is ready.
| Branch: issue-1579