Skip to content

fix(metadata): update cascade options for comic metadata relationship to include removal and orphan handling#3023

Open
balazs-szucs wants to merge 2 commits intobooklore-app:developfrom
balazs-szucs:possible-hibernate-fix
Open

fix(metadata): update cascade options for comic metadata relationship to include removal and orphan handling#3023
balazs-szucs wants to merge 2 commits intobooklore-app:developfrom
balazs-szucs:possible-hibernate-fix

Conversation

@balazs-szucs
Copy link
Collaborator

📝 Description

Possible fix for: #3000

Linked Issue: Fixes #

Required. Every PR must reference an approved issue. If no issue exists, open one and wait for maintainer approval before submitting a PR. Unsolicited PRs without a linked issue will be closed.

🏷️ Type of Change

  • Bug fix
  • New feature
  • Enhancement to existing feature
  • Refactor (no behavior change)
  • Breaking change (existing functionality affected)
  • Documentation update

🔧 Changes

🧪 Testing (MANDATORY)

PRs without this section filled out will be closed. "Tests pass" or "Tested locally" is not sufficient. You must provide specifics.

Manual testing steps you performed:

Regression testing:

Edge cases covered:

Test output:

Backend test output (./gradlew test)
PASTE OUTPUT HERE
Frontend test output (ng test)
PASTE OUTPUT HERE

📸 Screen Recording / Screenshots (MANDATORY)

Every PR must include a screen recording or screenshots showing the change working end-to-end in a running local instance (both backend and frontend). This means you must have actually built, run, and tested the code yourself. PRs without visual proof will be closed without review.


✅ Pre-Submission Checklist

All boxes must be checked before requesting review. Incomplete PRs will be closed without review. No exceptions.

  • This PR is linked to an approved issue
  • Code follows project style guidelines and conventions
  • Branch is up to date with develop (merge conflicts resolved)
  • I ran the full stack locally (backend + frontend + database) and verified the change works
  • Automated tests added or updated to cover changes (backend and frontend)
  • All tests pass locally and output is pasted above
  • Screen recording or screenshots are attached above proving the change works
  • PR is a single focused change (one bug fix OR one feature, not multiple unrelated changes)
  • PR is reasonably scoped (PRs over 1000+ changed lines will be closed, split into smaller PRs)
  • No unsolicited refactors, cleanups, or "improvements" are bundled in
  • Flyway migration versioning is correct (if schema was modified)
  • Documentation PR submitted to booklore-docs (if user-facing changes)

🤖 AI-Assisted Contributions

If any part of this PR was generated or assisted by AI tools (Copilot, Claude, ChatGPT, etc.), all items below are mandatory. You are fully responsible for every line you submit. "The AI wrote it" is not an excuse, and AI-generated PRs that clearly haven't been reviewed are the #1 reason PRs get closed.

  • I have read and understand every line of this PR and can explain any part of it during review
  • I personally ran the code and verified it works (not just trusted the AI's output)
  • PR is scoped to a single logical change, not a dump of everything the AI suggested
  • Tests validate actual behavior, not just coverage (AI-generated tests often assert nothing meaningful)
  • No dead code, placeholder comments, TODOs, or unused scaffolding left behind by AI
  • I did not submit refactors, style changes, or "improvements" the AI suggested beyond the scope of the issue

💬 Additional Context (optional)

@balazs-szucs
Copy link
Collaborator Author

balazs-szucs commented Feb 26, 2026

Not to repeat myself, but eventually we have to get to #2670 because hibernate situation is not maintainable. We need consistency...

If you indicate when I should get the PR ready, I'll do it.

@balazs-szucs balazs-szucs marked this pull request as ready for review February 26, 2026 15:09
@balazs-szucs
Copy link
Collaborator Author

As for this, it's a pure guess.

Copy link
Contributor

Copilot AI left a comment

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 addresses issue #3000 where deleting books with comic metadata failed with a Hibernate TransientPropertyValueException. The root cause was that the cascade configuration on the BookMetadataEntity to ComicMetadataEntity relationship only included PERSIST and MERGE operations, not REMOVE. When Hibernate attempted to delete a book and its metadata, it couldn't properly cascade the deletion to the associated comic metadata, leading to inconsistent entity states during the flush operation.

Changes:

  • Updated the cascade options for the comicMetadata field in BookMetadataEntity to include CascadeType.REMOVE and added orphanRemoval = true, ensuring comic metadata is properly deleted when the parent book metadata is deleted.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

private BookEntity book;

@OneToOne(cascade = {CascadeType.PERSIST, CascadeType.MERGE}, fetch = FetchType.LAZY)
@OneToOne(cascade = {CascadeType.PERSIST, CascadeType.MERGE, CascadeType.REMOVE}, orphanRemoval = true, fetch = FetchType.LAZY)
Copy link
Collaborator

Choose a reason for hiding this comment

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

hey, thanks for looking into this! the change itself looks reasonable, CascadeType.REMOVE + orphanRemoval = true makes sense here since BookEntity already cascades ALL + orphanRemoval down to BookMetadataEntity (line 32 of BookEntity), but that cascade was stopping at BookMetadataEntity and not reaching the ComicMetadataEntity child. so deleting a book would leave orphaned comic metadata rows behind, which matches the error in #3000.

one thing though, the PR description says "possible fix" so it sounds like this is a guess. were you able to actually reproduce the issue and test the fix? the issue reporter mentioned it only happens on specific entries in their instance, so it'd be good to know if you confirmed the cascade chain works end-to-end (delete a book that has comic metadata and verify the comic_metadata row gets cleaned up too). if you haven't been able to reproduce it, totally understandable given the setup, just want to make sure we're not merging blind.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nope. I wasn't, essentially I looked "What's different here" that may cause problems and made educated guess there.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tested this, it worked, but (for me) it worked before also perfectly so that does not mean anything

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Though, we can try to give to the people a test container see where it goes? What do you think about that 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, makes sense to have the folks who reported the bug test the container first. They'd be the best ones to verify the fix.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, I think there are some useful tips for you at: https://github.com/booklore-app/booklore/blob/develop/CONTRIBUTING.md

But feel free to ask, if something not clear.

Copy link

@DanielBrierton DanielBrierton Feb 26, 2026

Choose a reason for hiding this comment

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

Got a chance to build the image there and test it out. Unfortunately it hasn’t resolved the issue. The error I’m now getting is

Batch update returned unexpected row count from update 0 (expected row count 1 but was 0) [delete from comic_metadata where book_id=?] for entity [org.booklore.model.entity.ComicMetadataEntity with id '5737']

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This gave me an idea. 1 sec!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can you retry with this commit included: 383de22

Choose a reason for hiding this comment

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

Sorry, I missed your response yesterday.

Looks like @acx10 might have fixed this in #3067 already, but I've yet to try it myself. I'll try out their fix first since it's merged, and if that doesn't work I'll come back and try with that commit. Thanks!

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.

4 participants