Skip to content

refactor(metadata): make bookId and authorId/categoryId/shelfId final and adjust constructors#968

Open
balazs-szucs wants to merge 2 commits intogrimmory-tools:developfrom
balazs-szucs:final-constructor
Open

refactor(metadata): make bookId and authorId/categoryId/shelfId final and adjust constructors#968
balazs-szucs wants to merge 2 commits intogrimmory-tools:developfrom
balazs-szucs:final-constructor

Conversation

@balazs-szucs
Copy link
Copy Markdown
Member

@balazs-szucs balazs-szucs commented Apr 28, 2026

Description

Linked Issue: Fixes #

Changes

This pull request refactors the key classes used as composite primary keys in the entity model to improve immutability and encapsulation. The main changes are making the fields final, restricting the no-args constructor's access, and removing unnecessary setters.

Entity key class improvements:

  • Made bookId and the related key fields (authorId, categoryId, shelfId) final in BookMetadataAuthorKey, BookMetadataCategoryKey, and BookShelfKey to ensure immutability.
  • Changed the Lombok @NoArgsConstructor to have access = AccessLevel.PROTECTED, force = true in all three key classes, restricting instantiation and ensuring compatibility with JPA.
  • Removed the @Setter annotation from all key classes, preventing modification of fields after construction.

Summary by CodeRabbit

  • Refactor
    • Strengthened internal data integrity through enhanced immutability constraints on core entity structures.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 28, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: 030a2764-a9c1-417e-8c0e-e35e6ca96382

📥 Commits

Reviewing files that changed from the base of the PR and between f359e49 and 2d08624.

📒 Files selected for processing (3)
  • backend/src/main/java/org/booklore/model/entity/BookMetadataAuthorKey.java
  • backend/src/main/java/org/booklore/model/entity/BookMetadataCategoryKey.java
  • backend/src/main/java/org/booklore/model/entity/BookShelfKey.java
📜 Recent review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
  • GitHub Check: Test Suite / Backend Tests
  • GitHub Check: Test Suite / Frontend Tests
  • GitHub Check: Analyze (javascript-typescript)
  • GitHub Check: Analyze (java-kotlin)
  • GitHub Check: Frontend Lint Threshold Check
🧰 Additional context used
📓 Path-based instructions (1)
backend/src/**/*.java

📄 CodeRabbit inference engine (AGENTS.md)

backend/src/**/*.java: Use 4-space indentation and match surrounding Java style in backend code
Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce @Autowired field injection in backend code
Use MapStruct for entity/DTO mapping in backend code
Keep JPA entities on the *Entity suffix in backend code

Files:

  • backend/src/main/java/org/booklore/model/entity/BookMetadataAuthorKey.java
  • backend/src/main/java/org/booklore/model/entity/BookShelfKey.java
  • backend/src/main/java/org/booklore/model/entity/BookMetadataCategoryKey.java
🧠 Learnings (4)
📓 Common learnings
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 173
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:188-199
Timestamp: 2026-03-24T18:46:47.249Z
Learning: In grimmory-tools/grimmory (HardcoverSyncService.java), when a `hardcoverBookId` is stored in book metadata but is malformed (non-numeric), the preferred behavior is to return null and skip the sync rather than falling back to ISBN-based lookup. The reasoning is that an explicitly set bookId represents deliberate user intent, and silently resolving via ISBN could sync progress to a different book than intended.
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-22T01:56:39.495Z
Learning: Applies to backend/src/**/*.java : Keep JPA entities on the *Entity suffix in backend code
Learnt from: CR
Repo: grimmory-tools/grimmory PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-22T01:56:39.495Z
Learning: Applies to backend/src/**/*.java : Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce Autowired field injection in backend code
📚 Learning: 2026-04-10T08:15:37.436Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 449
File: booklore-api/src/main/java/org/booklore/service/book/BookDownloadService.java:139-145
Timestamp: 2026-04-10T08:15:37.436Z
Learning: When using Spring `ContentDisposition.builder(...).filename(name, StandardCharsets.UTF_8).build()` (i.e., explicitly providing UTF-8), the resulting header value should include both the quoted `filename="=?UTF-8?..."` and the RFC 5987 `filename*=` parameters. In this case, any extra ASCII fallback computation (e.g., deriving an ASCII `fallbackFilename` via `NON_ASCII_PATTERN` and calling `.filename(fallbackFilename)`) is likely redundant—prefer calling only `.filename(fallbackName?, StandardCharsets.UTF_8)` as appropriate and let Spring handle the UTF-8 header parameters. Verify by comparing the emitted header for `filename` and `filename*` before deciding to keep an ASCII fallback.

Applied to files:

  • backend/src/main/java/org/booklore/model/entity/BookMetadataAuthorKey.java
  • backend/src/main/java/org/booklore/model/entity/BookShelfKey.java
  • backend/src/main/java/org/booklore/model/entity/BookMetadataCategoryKey.java
📚 Learning: 2026-04-14T12:43:08.698Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 502
File: booklore-api/src/main/java/org/booklore/service/reader/ChapterCacheService.java:0-0
Timestamp: 2026-04-14T12:43:08.698Z
Learning: For this codebase (booklore-api), target Java 25 with `--enable-preview`, so `_` is intentionally used as an unnamed/ignored variable (e.g., lambda parameter or pattern variable) per Java’s preview feature JEP 456. Do not flag `_` in those contexts as an invalid/reserved identifier; only flag it if it’s used in a non-supported position (e.g., where an unnamed variable is not applicable for the Java preview rules).

Applied to files:

  • backend/src/main/java/org/booklore/model/entity/BookMetadataAuthorKey.java
  • backend/src/main/java/org/booklore/model/entity/BookShelfKey.java
  • backend/src/main/java/org/booklore/model/entity/BookMetadataCategoryKey.java
📚 Learning: 2026-04-04T15:36:56.558Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 372
File: booklore-api/src/main/java/org/booklore/app/service/AppBookService.java:357-359
Timestamp: 2026-04-04T15:36:56.558Z
Learning: In `booklore-api/src/main/java/org/booklore/app/service/AppBookService.java`, `shelfId` and `magicShelfId` are mutually exclusive navigation contexts in `getFilterOptions`. A request will never supply both at the same time, so the `else if (shelfId != null)` branch after the `magicBookIds != null` check is intentional and correct — there is no missing shelf-∩-magicShelf intersection case. Do not flag this pattern as a bug.

Applied to files:

  • backend/src/main/java/org/booklore/model/entity/BookShelfKey.java
🔇 Additional comments (3)
backend/src/main/java/org/booklore/model/entity/BookMetadataAuthorKey.java (1)

5-21: Looks good.

The final fields, protected no-args constructor with force = true, and existing equals/hashCode all line up with the intended immutable key refactor.

backend/src/main/java/org/booklore/model/entity/BookMetadataCategoryKey.java (1)

5-21: Looks good.

This keeps the composite key immutable while still giving JPA a protected no-args constructor path.

backend/src/main/java/org/booklore/model/entity/BookShelfKey.java (1)

5-21: Looks good.

The immutable key shape is consistent here as well, and the generated no-args constructor keeps the class usable by the persistence layer.


📝 Walkthrough

Walkthrough

Three composite key entity classes are refactored to enforce immutability by making ID fields final, removing setter generation, and adjusting Lombok's no-args constructor to be protected with force = true to maintain JPA compatibility.

Changes

Cohort / File(s) Summary
Composite Key Entity Immutability
backend/src/main/java/org/booklore/model/entity/BookMetadataAuthorKey.java, backend/src/main/java/org/booklore/model/entity/BookMetadataCategoryKey.java, backend/src/main/java/org/booklore/model/entity/BookShelfKey.java
Made bookId and secondary ID fields (authorId, categoryId, shelfId) final to enforce immutability. Removed @Setter annotation and adjusted @NoArgsConstructor to @NoArgsConstructor(access = AccessLevel.PROTECTED, force = true) to support JPA binding while preserving immutable field semantics. Added AccessLevel import to each class.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested labels

backend, chore

Suggested reviewers

  • imajes
  • zachyale

Poem

🐰 Hops with delight

Three keys now stand firm, no longer swayed,
Final fields locked in, immutable parade,
Protected constructors whisper JPA's truth,
Setters removed like shedding autumn's ruth,
Safer and stronger, the entities bloom! 🌱

🚥 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
Title check ✅ Passed The title follows conventional commit format with 'refactor(metadata):' prefix and clearly describes the main changes to the key classes.
Description check ✅ Passed The description follows the template structure with all required sections completed and provides clear details about the refactoring changes.
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 unit tests (beta)
  • Create PR with unit tests
✨ Simplify code
  • Create PR with simplified code

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant