Skip to content

Close repository configuration knobs from being overridden - #1672

Merged
alexander-yevsyukov merged 14 commits into
masterfrom
encapsulate-abstract-entity-repository
Aug 27, 2026
Merged

Close repository configuration knobs from being overridden#1672
alexander-yevsyukov merged 14 commits into
masterfrom
encapsulate-abstract-entity-repository

Conversation

@alexander-yevsyukov

@alexander-yevsyukov alexander-yevsyukov commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

What changed

Removed the open modifier from five protected repository configuration
functions, and reworked the two aggregate test fixtures that overrode them.

Closed in AbstractEntityRepository:

  • recordStateHistory()
  • stopRecordingStateHistory()
  • stateHistoryEnabled()

Closed in SignalDispatchingRepository:

  • eventHistoryDepth()
  • setEventHistoryDepth()

Why

Each of these does exactly one thing: read or write a private flag —
recordingEnabled and historyDepth respectively. None is an extension
point, and leaving them open created a real hazard: a subclass overriding
a getter decouples it from what the paired setter writes, silently turning
the setter into a no-op. Closing each getter together with its setter seals
the flag with its two accessors.

A derived repository that needs to act around these calls declares its own
function that calls them, rather than overriding.

Test fixtures

Two fixtures overrode eventHistoryDepth() / setEventHistoryDepth() for
no reason other than to widen their visibility to public for tests — which
re-published a framework-internal knob into the public API of every test
repository derived from them. They now wrap under distinct names, matching
the pattern the same files already used for the double-dispatch guard
(enableGuard(), guardEnabled()):

/** Exposes the event-history depth to the tests. */
@VisibleForTesting
public int getEventHistoryDepth() {
    return eventHistoryDepth();
}

/** Exposes the event-history depth setter to the tests. */
@VisibleForTesting
public void doSetEventHistoryDepth(int depth) {
    setEventHistoryDepth(depth);
}

The state-history trio needed no test changes at all — every existing user
(StateHistoryTestRepository, ProjectionStateHistorySpec,
JournalTestPmRepo) already wrapped rather than overrode.

Follow-on fixes from review

Two further commits address findings raised while preparing this PR, both in
code that predates the encapsulation work:

  • @Volatile on the double-dispatch guard flags. guardEnabled and
    historyDepth are read by dispatch workers and may change at runtime, but
    lacked the visibility guarantee that AbstractEntityRepository.recordingEnabled
    already had. Closing the accessors is what makes this a one-place fix — while
    the getters were open, a subclass could bypass the field entirely.
  • Restored the reason comment on the class-level @Suppress("DEPRECATION")
    in AggregatePart, which was dropped when the suppression was widened from
    method to class scope.

Notes for the reviewer

  • afterStore() and stateHistory() now read the flag through
    stateHistoryEnabled() rather than touching recordingEnabled directly.
    Behavior-neutral now that the function is final.
  • doSetEventHistoryDepth carries a do prefix deliberately: a plain
    setEventHistoryDepth(int) paired with getEventHistoryDepth() would make
    Kotlin synthesize a mutable eventHistoryDepth property over the JavaBean
    pair, which is not what these two explicit test hooks are.
  • Compatibility: removing open is source- and binary-breaking for any
    downstream subclass that overrode these members. The whole surface was
    introduced on the unreleased 2.0.0-SNAPSHOT line, so this is acceptable,
    but it deserves a release note — the migration (stop overriding, call from
    the constructor instead) is not self-evident.
  • Deliberately left for a follow-up: useDoubleDispatchGuard() and
    doubleDispatchGuardEnabled() are still open, carrying a milder form of
    the same hazard this PR removes elsewhere. The fixtures already wrap them,
    so closing them would touch only the two modifiers.

Verification

  • ./gradlew build dokkaGenerate — BUILD SUCCESSFUL
  • ./gradlew :server:test --rerun — 2032 tests, 2032 passed, 0 failed
  • Reviewers: spine-code-review APPROVE, review-docs APPROVE,
    kotlin-engineer APPROVE WITH CHANGES (its must-fix is addressed above)
  • Version gate: 2.0.0-SNAPSHOT.5232.0.0-SNAPSHOT.530

🤖 Generated with Claude Code

alexander-yevsyukov and others added 10 commits August 26, 2026 16:54
The state history and event history depth accessors only read or write
a private flag. Overriding them decouples the query from what the setter
writes, so a subclass could silently turn the setter into a no-op.

Closed in `AbstractEntityRepository`:
 - `recordStateHistory()`
 - `stopRecordingStateHistory()`
 - `stateHistoryEnabled()`

Closed in `SignalDispatchingRepository`:
 - `eventHistoryDepth()`
 - `setEventHistoryDepth()`

A derived repository that needs to act around these calls declares its
own function that calls them, as the test fixtures already did for the
double-dispatch guard. The two aggregate test fixtures that overrode the
event history depth accessors merely to widen their visibility now wrap
them the same way.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The class-level `@Suppress("DEPRECATION")` lost the comment explaining
why it is there. Coding standards require the suppression of a deprecated
API to name the sanctioning reason and the replacement to migrate to.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`guardEnabled` and `historyDepth` are read by dispatch workers on the
dispatch path, and both may be changed at runtime. Without `@Volatile`
a worker thread is not guaranteed to observe the change, which is the
posture `AbstractEntityRepository.recordingEnabled` already takes.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Place the reason as `//` lines above the annotation, as
`DefaultRepository` does, instead of a trailing block comment.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings August 26, 2026 17:29

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR primarily tightens encapsulation of repository “configuration knobs” by preventing subclasses from overriding internal getters/setters that only proxy private flags, and updates test fixtures/tests to use explicit test-only wrappers instead of overrides. It also includes a broad set of build/dependency/report updates (Gradle/TestKit/POM/dependency coordinates) that appear orthogonal to the repository encapsulation goal.

Changes:

  • Make state-history and event-history depth accessors non-overridable (open removed) and route internal reads through final accessors where applicable.
  • Update aggregate test fixtures and tests to use getEventHistoryDepth() / doSetEventHistoryDepth(...) wrappers instead of overriding protected members.
  • Update build/dependency artifacts and generated dependency reports (plus associated build tooling changes).

Reviewed changes

Copilot reviewed 32 out of 41 changed files in this pull request and generated no comments.

Show a summary per file
File Description
version.gradle.kts Bumps published snapshot version.
server/src/testFixtures/java/io/spine/server/aggregate/given/repo/ProjectAggregateRepository.java Replaces overrides with explicit test-only wrappers for event-history depth.
server/src/testFixtures/java/io/spine/server/aggregate/given/aggregate/AbstractAggregateTestRepository.java Replaces overrides with explicit test-only wrappers for event-history depth.
server/src/test/kotlin/io/spine/server/aggregate/DoubleDispatchGuardSpec.kt Migrates tests to new wrapper methods.
server/src/test/java/io/spine/server/aggregate/AggregateRepositoryTest.java Migrates tests to new wrapper methods.
server/src/main/kotlin/io/spine/server/entity/SignalDispatchingRepository.kt Adds volatility to guard/depth flags; seals event-history depth accessors.
server/src/main/kotlin/io/spine/server/entity/AbstractEntityRepository.kt Seals state-history toggle/query methods; routes checks via accessor.
server/src/main/kotlin/io/spine/server/aggregate/AggregatePart.kt Restores suppression reason and scopes DEPRECATION suppression at class level.
docs/dependencies/pom.xml Updates generated dependencies POM versions/coordinates.
docs/dependencies/dependencies.md Updates generated dependency/license report output.
build.gradle.kts Updates buildscript dependency forcing and artifacts used.
gradle/wrapper/gradle-wrapper.properties Skipped (config-managed path in this repo’s rules).
.gitignore Skipped (config-managed path in this repo’s rules).
.github/workflows/gradle-wrapper-validation.yml Skipped (config-managed path in this repo’s rules).
buildSrc/build.gradle.kts Skipped (config-managed path in this repo’s rules).
buildSrc/src/main/kotlin/io/spine/gradle/report/pom/ResolvedVersions.kt Skipped (config-managed path in this repo’s rules).
buildSrc/src/main/kotlin/io/spine/gradle/report/pom/PomXmlWriter.kt Skipped (config-managed path in this repo’s rules).
buildSrc/src/main/kotlin/io/spine/gradle/report/pom/PomGenerator.kt Skipped (config-managed path in this repo’s rules).
buildSrc/src/main/kotlin/io/spine/gradle/report/pom/DependencyWriter.kt Skipped (config-managed path in this repo’s rules).
buildSrc/src/test/kotlin/io/spine/gradle/report/pom/PomGeneratorIgTest.kt Skipped (config-managed path in this repo’s rules).
buildSrc/src/test/kotlin/io/spine/gradle/report/pom/DependencyWriterSpec.kt Skipped (config-managed path in this repo’s rules).
buildSrc/src/main/kotlin/io/spine/dependency/local/Validation.kt Skipped (config-managed path in this repo’s rules).
buildSrc/src/main/kotlin/io/spine/dependency/local/ToolBase.kt Skipped (config-managed path in this repo’s rules).
buildSrc/src/main/kotlin/io/spine/dependency/local/ProtoTap.kt Skipped (config-managed path in this repo’s rules).
buildSrc/src/main/kotlin/io/spine/dependency/local/CoreJvmCompiler.kt Skipped (config-managed path in this repo’s rules).
buildSrc/src/main/kotlin/io/spine/dependency/local/CoreJvm.kt Skipped (config-managed path in this repo’s rules).
buildSrc/src/main/kotlin/io/spine/dependency/local/Compiler.kt Skipped (config-managed path in this repo’s rules).
buildSrc/src/main/kotlin/io/spine/dependency/local/Base.kt Skipped (config-managed path in this repo’s rules).
buildSrc/src/main/kotlin/io/spine/dependency/lib/PalantirJavaFormat.kt Skipped (config-managed path in this repo’s rules).
buildSrc/src/main/kotlin/io/spine/dependency/lib/Log4j2.kt Skipped (config-managed path in this repo’s rules).
buildSrc/src/main/kotlin/io/spine/dependency/lib/JacksonV2.kt Skipped (config-managed path in this repo’s rules).
buildSrc/src/main/kotlin/io/spine/dependency/lib/Jackson.kt Skipped (config-managed path in this repo’s rules).
buildSrc/src/main/kotlin/io/spine/dependency/boms/Boms.kt Skipped (config-managed path in this repo’s rules).
.agents/tasks/archive/state-history-for-all-entities.md Skipped (config-managed path in this repo’s rules).
.agents/tasks/archive/reduce-public-internal-api.md Skipped (config-managed path in this repo’s rules).
.agents/tasks/archive/event-history-for-process-managers.md Skipped (config-managed path in this repo’s rules).
.agents/tasks/archive/de-event-sourcing-plan.md Skipped (config-managed path in this repo’s rules).
.agents/tasks/archive/de-event-sourcing-followups.md Skipped (config-managed path in this repo’s rules).
.agents/tasks/archive/de-event-sourcing-brief.md Skipped (config-managed path in this repo’s rules).

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot AI review requested due to automatic review settings August 26, 2026 17:33
@alexander-yevsyukov alexander-yevsyukov self-assigned this Aug 26, 2026
@alexander-yevsyukov alexander-yevsyukov moved this to 🏗 In progress in v2.0 Aug 26, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 32 out of 41 changed files in this pull request and generated no new comments.

@alexander-yevsyukov alexander-yevsyukov moved this from 🏗 In progress to In Review in v2.0 Aug 26, 2026
@codecov

codecov Bot commented Aug 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.87%. Comparing base (0779b5f) to head (2df13c1).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1672   +/-   ##
=======================================
  Coverage   87.87%   87.87%           
=======================================
  Files        1067     1067           
  Lines       22466    22466           
  Branches     1090     1090           
=======================================
  Hits        19741    19741           
  Misses       2346     2346           
  Partials      379      379           
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@armiol

armiol commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

@claude regarding @Volatile: check whether Spine-based apps really need this. I have doubts, since not a lot of work is being done in a multi-threaded environment.
Therefore, to me this is a clear over-engineering.

@armiol

armiol commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

@alexander-yevsyukov apparently, @claude knows I am mostly a Codex user, and ignores me. See my comment above :)

Copilot AI review requested due to automatic review settings August 26, 2026 20:34
@alexander-yevsyukov

alexander-yevsyukov commented Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Dropped the @Volatile change — reverted in a49988f. guardEnabled and historyDepth are back to plain private var. Dropped @Volatile on stateHistoryEnabled (previously recordingEnabled) too.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 32 out of 41 changed files in this pull request and generated no new comments.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

server/src/main/kotlin/io/spine/server/entity/SignalDispatchingRepository.kt:269

  • historyDepth is read/written via these accessors and can be mutated at runtime (e.g., in tests). Without a memory-visibility guarantee, dispatch threads may observe stale values. Consider making the accessors synchronized (or marking the backing field historyDepth as @Volatile).

Copilot AI review requested due to automatic review settings August 26, 2026 20:40

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 32 out of 41 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

server/src/main/kotlin/io/spine/server/entity/SignalDispatchingRepository.kt:272

  • eventHistoryDepth() / setEventHistoryDepth() read/write historyDepth, which is used by dispatch-time logic (e.g., enabling the double-dispatch guard). If the depth can be adjusted at runtime, the current implementation provides no cross-thread visibility guarantees.

Either make the backing field @Volatile or synchronize these accessors so updates are reliably observed by worker threads.

@alexander-yevsyukov
alexander-yevsyukov merged commit f4f473d into master Aug 27, 2026
11 checks passed
@alexander-yevsyukov
alexander-yevsyukov deleted the encapsulate-abstract-entity-repository branch August 27, 2026 13:02
@github-project-automation github-project-automation Bot moved this from In Review to ✅ Done in v2.0 Aug 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants