Repository navigation
Conversation
Refactored nonce generator
|
santanusinha
left a comment
santanusinha
left a comment
There was a problem hiding this comment.
Thanks for the PR — the direction of introducing IdGeneratorV2 as a parallel, non-breaking (from a runtime perspective) API alongside IdGenerator is good. The new formatter/decorator separation is a cleaner design than the old Base36IdFormatter-wrapping-a-formatter approach. That said, there are a few concerns worth addressing before merging.
Breaking Changes
1. IdValidationConstraint.isValid(Id) → isValid(InternalId) is a binary-breaking change
Any downstream library or application implementing IdValidationConstraint will fail to compile against this version. This is not just an internal refactor — IdValidationConstraint is a public API. If this is intentional, it needs a prominent callout in the PR description and ideally a major version bump.
2. IdGenerator.initialize(int, List, Map) → initialize(List, Map)
Callers that previously passed the node ID into initialize() will now get a compile error. The PR description calls this out, but I'd suggest keeping the old overload as @Deprecated rather than removing it — especially since the IdGenerator.initialize(int) overload still exists (which is confusing: one overload still takes node, the other doesn't).
3. GenerationResult field renaming
Removing nonceInfo and adding exponent, time, and internalId breaks any caller using GenerationResult directly. If this class is considered internal (package-private), it is fine — but it is currently public.
Design / Correctness
4. NodeUtils node re-setting is silently allowed, but IdGeneratorBase.setNodeId throws on re-set
// NodeUtils — silently overwrites
public static void setNode(int nodeId) { node = nodeId; }
// IdGeneratorBase — throws on second call
if (this.nodeId > 0) { throw new RuntimeException("Node ID already set"); }These two guards are inconsistent. A caller who calls NodeUtils.setNode() twice (e.g. in test teardown/setup) will silently succeed, but IdGeneratorBase.setNodeId() will then throw on the second initialize() call. This is exactly why the test files now need IdGenerator.cleanUp() / IdGeneratorV2.cleanUp() in @BeforeEach.
Consider making NodeUtils also throw (or log a warning) on re-set, consistent with IdGeneratorBase; or relax IdGeneratorBase to allow re-set, consistent with NodeUtils. The current state mixes both.
5. IdGeneratorType.LOOKUP_MAP relies on object identity for IdFormatter
private static final Map<IdFormatter, Map<List<IdDecorator>, Integer>> LOOKUP_MAP =
Arrays.stream(values())
.collect(Collectors.groupingBy(type -> type.idFormatter, ...));IdFormatter is an interface with no equals()/hashCode() override — the map key is the specific singleton instances held in IdFormatters. This works as long as callers always use IdFormatters.original() / IdFormatters.randomNonce() (the cached singletons), but will silently return an empty Optional if anyone passes a freshly constructed formatter. The IdGeneratorRequestBuilder.includeRandomNonce() method does use the singleton, so it's fine today — but this is fragile. A comment or a guard would help.
6. IdParsersV2 DEFAULT_PATTERN ambiguity with V2 format
private static final Pattern DEFAULT_PATTERN = Pattern.compile("([A-Za-z]*)([\\d]{22})(.*)");A V2 ID like PREFIX0122digitpayload... could match this pattern if the generator-type digits happen to be all digits too. The parsing strategy first checks DEFAULT_PATTERN and bails out if group(3) is empty — but for a V2 ID with no suffix, group(3) contains just the 2-digit type code and is not empty, so it falls through to PATTERN. This seems intentional, but the logic is subtle. A comment explaining the two-step disambiguation would significantly help future maintainers.
7. Dual registration burden on callers
The Javadoc on IdGeneratorV2 already mentions this, which is good:
"If both generators are used in the same service, ensure constraints and domains are registered on each independently."
However, ServiceDiscoveryBundle.start() initializes both generators with Collections.emptyMap() domain constraints. A real user who upgrades will need to remember to register the same constraints on both generators. Consider whether a shared domain/constraint registry (or at least a factory method that initializes both from the same config) would reduce this footgun.
Minor / Style
8. Missing license headers on new files
Several new files are missing the Apache 2.0 license header that all other files in the project carry:
InternalId.javaIdGenerationInternalRequest.javaIdProvider.javaNonceGenerators.javaNonceUtils.javaIdDecorators.javaFormattedId.javaIdGenerationRequestV2.javaNodeUtils.java
9. Stale @SneakyThrows in benchmark methods
In IdGeneratorPerfTest, the benchmark methods annotated @SneakyThrows no longer throw any checked exceptions. The annotation can be removed.
10. Unused import in IdGeneratorPerfV2Test and IdParsersV2Test
IdGeneratorBase is imported in IdParsersV2Test but not used.
11. IdGenerationRequest still has an idFormatter field, but Domain no longer has one
After removing idFormatter from Domain, the IdGenerationRequest (the old public-facing request class) still carries idFormatter. This creates confusion: is IdGenerationRequest still used? It appears to no longer be used anywhere after the refactor — if so, it should be deprecated or removed.
Tests
The new test suite for IdGeneratorV2Test is comprehensive and covers happy paths, retries, constraints, suffix/prefix validation, and round-trip parse/generate. Well done. Two suggestions:
- Add a cross-generator test: generate with
IdGenerator, parse withIdGeneratorV2.parse()and vice versa — thetestOriginalIdParseSuccessAfterGenerationtest already does the first direction, but the reverse (generate with V2, parse with legacyIdGenerator) is not covered and would document the interop contract. IdGeneratorV2.cleanUp()should resetNodeUtils: CurrentlycleanUp()resets the base generator butNodeUtils.noderemains set. AftercleanUp(), callingIdGeneratorV2.initialize()without first callingNodeUtils.setNode()will silently use whatever the old node value was. This is especially surprising in tests wherecleanUp()is called in@AfterEachbut the node re-setup happens in@BeforeEach. This works today because@BeforeEachalways callsNodeUtils.setNode()explicitly, but it is a latent footgun.
- IdValidationConstraint: restore isValid(Id) as the abstract method (binary-compat break flagged in review); isValid(InternalId) is now a default bridging via toInternalId()/toId() helpers - GenerationResult: document as internal-only (not public API contract) - IdFormatter: add id() so IdGeneratorType.LOOKUP_MAP keys off a stable identifier instead of formatter object identity, which only worked for the cached IdFormatters singletons - IdParsersV2: comment DEFAULT_PATTERN's two-step V1/V2 disambiguation - NodeUtils: add reset(), called from IdGenerator/IdGeneratorV2 cleanUp() so a stale node id isn't silently reused by a later initialize() that forgets to call setNode() again - add missing Apache license headers on 9 files - drop stale @SneakyThrows from perf benchmarks, unused import in IdParsersV2Test, deprecate unused IdGenerationRequest - add IdGeneratorV2Test covering the (asymmetric) V2->legacy parse interop contract Refs review comments on #109 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Removed InternalId method in interface
Keep V1 and V2 generators on the same process-level node while allowing idempotent startup calls. Reset remains the explicit lifecycle boundary.
Resolve equivalent formatter instances without adding an internal lookup identifier to the public IdFormatter contract.
fix(id): address Santanu's review feedback on #109
|
Keep the shared node reserved until all initialized generators stop. Snapshot request lists so builder reuse cannot change encoded formats. Reject partial IDs and invalid suffixes; join test workers before cleanup. V2 requests no longer expose setConstraints; use the builder instead. NodeUtils.reset rejects resets while a generator remains initialized.
|
4 participants
Summary
Introduces
IdGeneratorV2— a next-generation ID generator that runs parallel to the existingIdGeneratorwith an extended feature set, while keeping the coreIdGeneratorBaseshared.What's new
IdGeneratorV2IdGenerationRequestV2builder with.includeRandomNonce()and.includeBase36()optionsIdDecorator) for post-format transformations (e.g. base36 encoding)IdParsersV2for parsing V2-format IDs back toIdobjectsNew formatter:
RandomNonceIdFormatter{yyMMddHHmmssSSS}{nodeId:4d}{randomNonce:3d}— 22 chars totalparse()New decorator:
Base36IdDecoratorIdGeneratorTypeenumIdParsersV2to auto-detect format and reverse the decorator chain on parseInfrastructure
NodeUtils: thread-local + default node ID management, shared across both generatorsNonceUtils: extracted retry count and retry event listener fromNonceGeneratorInternalId: internal ID representation decoupled from the publicIdtypeIdGeneratorBaserefactored to be formatter-agnostic viaIdProviderfunctionBreaking changes (intentional)
IdGenerator.initialize(int, List, Map)→initialize(List, Map)(node now viaNodeUtils)IdGenerator.generate(IdGenerationRequest)now returnsOptional<InternalId>Domain.builder()no longer acceptsidFormatter— formatter is now per-requestNotes
IdGeneratorandIdGeneratorV2maintain independent constraint/domain registries. Register constraints on each generator separately if both are used in the same service.