Fix null reader validation and typo in OptimizingAppendListenerDecorator - #116
Merged
Merged
Conversation
…og sizes Five independent review items. 1. Rename OptimizingApendListenerDecorator to OptimizingAppendListenerDecorator. The impl module *is* published -- its pom opts in with maven.deploy.skip=false, it is not in central-publishing's excludeArtifacts, it is in the BOM, and Maven Central carries it up to 0.9.1 -- so this is a real, if tiny, break rather than a free rename. Taken anyway: the class is 0.x, absent from the README, from CLAUDE.md's API sections and even from its own package-info's "Main Components", and it is applied by the store to every listener passed to subscribe(), so naming it from outside only gets a listener wrapped twice. A deprecated forwarder would enshrine the typo permanently in a package the ServiceLoader exists to hide, for no known caller. Javadoc now says it is internal. 2. Tighten EventStreamId.concretizes to match its javadoc rather than the reverse. The doc lists four conditions, the code checked three, so a wildcard-purpose stream concretized another wildcard-purpose stream in the same context. Unreachable through the library: canAppendTo reaches concretizes only after this.equals(other) has already returned true for that shape, and append() rejects the read-only target regardless. But concretizes is public API in the published api module, and "concretize" means supplying a value for a wildcard -- a wildcard supplies none. The doc describes what the method is for; the code was missing a clause. Behaviour of canAppendTo is unchanged. 3. Drop reader.toString() on values already typed String (EventStoreImpl.getBookmark, PostgresEventStorageImpl.bookmark/removeBookmark). Note these did not stringify a null into "null" -- they threw NPE -- so they were accidental null guards, and removing them alone would have left null readers undefined and backend-dependent: a HashMap null key in memory, a not-null violation on a Postgres place, a silent no-op on a remove and an empty Optional on a get. Replaced with one explicit Objects.requireNonNull at the store boundary, so every backend answers the same way, plus @throws on EventSource. The SPI contract is deliberately left alone: guarding one backend and not the others would recreate the divergence. 4. notify(BookmarkPlacedNotification) logged eventuallyConsistentSubscribers.size() in both debug lines while describing bookmark listeners; now bookmarkSubscribers.size(). 5. Make EventStreamId.DEFAULT_PURPOSE public. CLAUDE.md already points operators at it for raw SQL and interop, where the value is needed; publishing the constant is additive and binary-compatible, and lets an interop layer bind what the library binds. Verified: mvn clean install, api + impl module tests (150), and the TCK against the in-memory backends (426 tests, 0 failures). Docker was unavailable in this environment, so the PostgreSQL backends of the TCK were not re-run; the only Postgres change is dropping two no-op toString() calls on bound statement parameters. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L2PUkUZA3LpkDYFxKVRMRx
vanrogu
pushed a commit
that referenced
this pull request
Aug 3, 2026
Two conflicts, both where develop's review batch (#116) landed on lines this branch had also changed. 1. OptimizingApendListenerDecorator was renamed to OptimizingAppendListenerDecorator on develop while this branch added a delegate() accessor to it. Took the rename and kept the accessor, which notifyQuietly needs to log the listener the caller subscribed rather than the decorator wrapping it. The accessor is now placed after the constructor -- on this branch it had landed between the constructor's javadoc and the constructor, orphaning that javadoc. The instanceof in EventStoreImpl.notifyQuietly still named the old class. Git could not see that one: the rename and the reference are in different files, so it merged cleanly and would have failed to compile. 2. notify(BookmarkPlacedNotification) -- develop fixed the debug line to log bookmarkSubscribers.size() instead of eventuallyConsistentSubscribers.size(); this branch changed the line below it to route through notifyQuietly. Both kept. Verified after the merge: mvn clean install, api + impl + inmem module tests (155), and the TCK against both in-memory backends (428 tests, 0 failures). The one error is EventImportRoundTripTest, which hard-requires a Postgres container -- no Docker in this environment, and it fails identically on develop. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017mPz52fur5brEvq8sskF8Z
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR addresses null safety in bookmark operations and corrects a class name typo. It adds explicit null validation for reader parameters in bookmark methods, fixes a misspelled class name, and improves documentation around the DEFAULT_PURPOSE constant and stream concretization logic.
Key Changes
Null reader validation: Added
requireReader(String)method to explicitly validate that reader parameters are not null inplaceBookmark()andgetBookmark()methods. Previously, some paths were accidentally guarded by.toString()calls on already-typed String values. This makes the validation uniform across all backends and explicit in the code.Class name correction: Renamed
OptimizingApendListenerDecoratortoOptimizingAppendListenerDecorator(fixed typo: "Apend" → "Append"). Updated all references in:Stream concretization fix: Updated
EventStreamId.concretizes()to reject wildcard-purpose streams from concretizing other streams. Added check!this.isAnyPurpose()to prevent a wildcard stream from filling in another wildcard's purpose (which it cannot do). Added comprehensive test casetestWildcardPurposeConcretizesNothing().Documentation improvements:
EventStreamId.DEFAULT_PURPOSEpublic with detailed javadoc explaining its role as storage-level wire format for interop layers and raw SQL operationsEventStreamId.concretizes()javadoc to clarify that wildcards concretize nothingEventSourcebookmark method javadocs to document null-check behavior and NullPointerException contractOptimizingAppendListenerDecoratorexplaining it is not meant for direct useCode cleanup: Removed unnecessary
.toString()calls on String parameters in PostgreSQL bookmark implementation (reader parameter was already typed as String)Implementation Details
The null validation is centralized in a single
requireReader(String)method with comprehensive javadoc explaining why it exists: different backends previously handled null readers inconsistently (in-memory HashMap accepts null keys; PostgreSQL returns constraint violations or silent no-ops). The explicit check ensures uniform behavior across all backends and makes the contract clear at the API level.The wildcard concretization fix ensures the semantic contract stated in javadoc is enforced: "This stream has a specific purpose (not a wildcard)" is now validated in code, not just documentation.
https://claude.ai/code/session_01L2PUkUZA3LpkDYFxKVRMRx