Skip to content

fix: delegate MailData id generation to framework - #5

Merged
wisdommen merged 1 commit into
masterfrom
fix/maildata-id-generation
Jun 4, 2026
Merged

fix: delegate MailData id generation to framework#5
wisdommen merged 1 commit into
masterfrom
fix/maildata-id-generation

Conversation

@wisdommen

@wisdommen wisdommen commented Jun 4, 2026

Copy link
Copy Markdown
Member

Summary

  • Remove timestamp-based primary key generation from MailData constructor
  • Let the Framework insert-time UUID guard assign ids when id == null
  • Update MailDataTest default-id assertions to match delegated id generation

Why

System.currentTimeMillis() can collide during bulk/broadcast mail creation in the same millisecond, causing duplicate primary keys in relational storage or overwrite risk in JSON storage.

Test Plan

  • mvn -B -Dtest=MailDataTest test — BUILD SUCCESS, 57 tests, 0 failures, 0 errors
  • mvn -B package — BUILD SUCCESS, 459 tests, 0 failures, 0 errors, 11 skipped

Scope

  • UltiMail source/test only
  • No POM, workflow, schema, migration, release, or deploy changes

Summary by CodeRabbit

  • Tests

    • Updated test expectations for ID initialization behavior of mail items, verifying that new instances maintain null IDs before insertion.
  • Refactor

    • Modified ID initialization logic in the mail entity constructor to no longer auto-generate values on instantiation.

@coderabbitai

coderabbitai Bot commented Jun 4, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 3ee2d0b2-42c1-4267-875f-6cb6b3436a56

📥 Commits

Reviewing files that changed from the base of the PR and between 883db24 and 56fa359.

📒 Files selected for processing (2)
  • src/main/java/com/ultikits/plugins/mail/entity/MailData.java
  • src/test/java/com/ultikits/plugins/mail/entity/MailDataTest.java
💤 Files with no reviewable changes (1)
  • src/main/java/com/ultikits/plugins/mail/entity/MailData.java

📝 Walkthrough

Walkthrough

MailData's public no-arg constructor no longer auto-generates an id from a timestamp. The constructor continues to initialize sentTime and reset status flags. Corresponding tests are updated to assert that new instances have null IDs rather than auto-generated values.

Changes

ID initialization refactor

Layer / File(s) Summary
Remove auto-generated ID from constructor
src/main/java/com/ultikits/plugins/mail/entity/MailData.java, src/test/java/com/ultikits/plugins/mail/entity/MailDataTest.java
MailData constructor no longer assigns ID via timestamp. Tests updated to verify new instances have null IDs, confirming ID assignment is deferred to persistence layer.

Estimated Code Review Effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 A timestamp was living in a constructor's nest,
Until the day it left to let persistence do the rest,
Now MailData waits for ID to arrive,
When stored, it shall truly thrive!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix: delegate MailData id generation to framework' directly and clearly summarizes the main change: removing timestamp-based ID generation from MailData and delegating it to the framework.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/maildata-id-generation

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@wisdommen
wisdommen merged commit 3d229db into master Jun 4, 2026
7 checks passed
@wisdommen
wisdommen deleted the fix/maildata-id-generation branch June 4, 2026 02:09
wisdommen added a commit that referenced this pull request Aug 15, 2026
#8)

* test: assert MailData id is null and receivers correct on recall broadcast

Strengthens the existing verify(times(2)) into an ArgumentCaptor check, so
the test actually pins the behaviour #5 introduced (id generation delegated
to the framework) rather than only counting insert calls.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HzTkgBjRdaTcLrbvkoaFkp

* fix: declare api-version 621 to match the symbols this module needs

This module's bytecode references DataOperator.insert / update(T) with the
BaseDataEntity descriptors introduced in UltiTools-API 6.2.1, but plugin.yml
still declared 620. PluginManager.isUltiToolsVersionCompatible only reads
api-version -- the pom pin is provided scope and never reaches runtime -- so a
server on framework 6.2.0 admits this JAR and then throws NoSuchMethodError on
the first data access.

621 is measured, not estimated: a per-symbol comparison of the built JAR
against 6.2.0 and 6.2.1 reports missing symbols for the former and none for
the latter.

No release is cut for this on its own; the corrected floor rides out with this
module's next real release, which is MAJOR under the module versioning rules.

Refs UltiKits/UltiTools-Reborn#284

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HzTkgBjRdaTcLrbvkoaFkp

* docs: state the runtime minimum as 6.2.1 in README

Raising plugin.yml's api-version to 621 makes the README actively harmful:
it tells a server owner to install UltiTools-API 6.2.0, which the framework
compatibility check will now refuse. The documented minimum has to move with
the manifest floor.

Only statements of the runtime requirement are touched. Sentences that record
which API generation this module migrated to are history and stay at 6.2.0.

Refs UltiKits/UltiTools-Reborn#284

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HzTkgBjRdaTcLrbvkoaFkp

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant