Skip to content

refactor(reindex): convert ReindexEntry to Immutables value object (#34934)#35391

Merged
fabrizzio-dotCMS merged 4 commits intomainfrom
issue-34934-reindexentry-immutable
Apr 21, 2026
Merged

refactor(reindex): convert ReindexEntry to Immutables value object (#34934)#35391
fabrizzio-dotCMS merged 4 commits intomainfrom
issue-34934-reindexentry-immutable

Conversation

@fabrizzio-dotCMS
Copy link
Copy Markdown
Member

@fabrizzio-dotCMS fabrizzio-dotCMS commented Apr 20, 2026

Summary

  • Converts ReindexEntry from a mutable POJO to a @Value.Immutable abstract class
  • Equals/hashCode match original semantics (identToIndex, priority, delete, serverId) via @Value.Auxiliary on id, lastResult, and timeEntered
  • Updates both creation sites in ReindexQueueFactory to use ImmutableReindexEntry.builder()...build()
  • serverId was never set anywhere in production code — now correctly documented as @Nullable
  • isReindex() and errorCount() preserved as concrete methods on the abstract class
  • toString() preserved with identical format to the original

Notes

  • mock(ReindexEntry.class) in ReindexQueueAPITest continues to work — Mockito supports abstract classes
  • ImmutableReindexEntry is generated by the Immutables annotation processor at compile time

Test plan

  • ./mvnw install -pl :dotcms-core -DskipTests — verifies annotation processor generates ImmutableReindexEntry
  • ./mvnw verify -pl :dotcms-integration -Dcoreit.test.skip=false -Dit.test=ReindexQueueAPITest
  • ./mvnw verify -pl :dotcms-integration -Dcoreit.test.skip=false -Dit.test=ReindexAPITest

Refs: #34934

🤖 Generated with Claude Code

This PR fixes: #34934

…34934)

Replaces the mutable POJO with a @Value.Immutable abstract class using
@value.Style(init = "set*") to preserve the existing builder API surface.
Equals/hashCode match the original semantics (identToIndex, priority, delete,
serverId) via @Value.Auxiliary on id, lastResult, and timeEntered.
Updates both creation sites in ReindexQueueFactory to use ImmutableReindexEntry.builder().

Refs: #34934

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 20, 2026

Claude finished @fabrizzio-dotCMS's task in 2m 26s —— View job


PR Review

  • Gather context
  • Read changed files
  • Analyze code
  • Post review

Overall the conversion is clean and the Immutables setup is correct. A few issues worth addressing:


1. Date leaks mutability through an immutable object

ReindexEntry.java:62getTimeEntered() returns java.util.Date, which is mutable. Any caller can mutate the returned Date and corrupt the "immutable" value. Either return java.time.Instant or copy defensively in the getter:

@Override
public Date getTimeEntered() {
    Date d = super.getTimeEntered();
    return d == null ? null : new Date(d.getTime());
}

Immutables does not guard mutable types automatically. Fix this →


2. hashCode semantics do NOT match the original

The PR description says "Equals/hashCode match original semantics" — this is incorrect for hashCode. In the original class:

result = prime * result + (int) (priority ^ (priority >>> 32));

priority is an int. Java masks int shifts to 5 bits, so priority >>> 32 is priority >>> 0 = priority. This means priority ^ priority = 0priority was never contributing to hashCode in the original. Immutables will fix that bug (correctly including priority), but the description is misleading. This should be either acknowledged or the description corrected.


3. mapToReindexEntry silently drops DB columns

ReindexQueueFactory.java:356–363 — the query in loadUpLocalQueue is select * (includes serverid, time_entered), but mapToReindexEntry only maps id, ident_to_index, priority, and dist_action. Queue entries will always have null serverId and timeEntered.

This was true before the PR, but is now visible because consumers might call getTimeEntered() expecting data from the DB. The query should either be narrowed to the columns actually mapped, or the builder should map serverid and time_entered. Fix this →


4. Test uses ordinal() where dbValue() is required

ReindexQueueAPITest.java:96, 102 — both lines use Priority.REINDEX.ordinal() (= 3) instead of Priority.REINDEX.dbValue() (= 300). The test passes today only by coincidence — markAsFailed increments 1 + priority when errorCount < MAX_FAILURE_ATTEMPTS, so starting at priority 2 yields priority 3 which happens to equal REINDEX.ordinal(). Any change to REINDEX_MAX_FAILURE_ATTEMPTS or Priority ordering would silently break the assertion. This is a pre-existing bug that became visible via ImmutableReindexEntry.builder().from(currentEntry).priority(...) — worth fixing in this PR. Fix this →


5. Minor: static factory on abstract class couples it to generated implementation

ReindexEntry.java:91–93 — The builder() static factory delegates directly to ImmutableReindexEntry.builder(). This is a convenience but it means the abstract class now depends on its generated subclass by name, which can cause compile errors during incremental builds before annotation processing runs. Most Immutables codebases use ImmutableReindexEntry.builder() directly at call sites. Not a blocker, but worth knowing.


Clean otherwise — the @Value.Auxiliary choices, @Value.Default on isDelete(), and null-safe @Nullable usage are all correct.

fabrizzio-dotCMS and others added 3 commits April 20, 2026 14:53
…yle(init = "set*")

@value.Style(init = "set*") generated setIsDelete() for isDelete(), not
setDelete() — causing CI compilation failure. Remove the style annotation
and adopt Immutables default fluent naming (id/identToIndex/isDelete…).
Remove dead toString() override (Immutables generates its own).
Update ReindexQueueFactory call sites and ReindexQueueAPITest accordingly.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
getFailedReindexRecords() selects failed entries without dist_action —
those records are never deletes, so false is the correct default.
Using @Value.Default preserves the .isDelete(boolean) setter in the
builder so mapToReindexEntry() can still set true for DELETE actions.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…) factory

- Add @OverRide toString() that preserves the original log format:
  IndexJournal [id=..., identToIndex=..., priority=..., delete=..., serverId=...]
- Replace direct ImmutableReindexEntry.builder() calls in ReindexQueueFactory
  with ReindexEntry.builder() to keep the generated class encapsulated

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@fabrizzio-dotCMS fabrizzio-dotCMS added this pull request to the merge queue Apr 21, 2026
Merged via the queue into main with commit eafb30d Apr 21, 2026
49 checks passed
@fabrizzio-dotCMS fabrizzio-dotCMS deleted the issue-34934-reindexentry-immutable branch April 21, 2026 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI: Safe To Rollback Area : Backend PR changes Java/Maven backend code

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

[TASK] Migrate ContentletIndexAPIImpl

2 participants