-
Notifications
You must be signed in to change notification settings - Fork 367
Fix DELETE query in derived delete operations #2128
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
88285f7 to
1f70e88
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know how maintainers will think about this, but I was planning to maintain the existing codes, and create new test cases and classes to verify fixed.
The changes of test I made were just for reporting.
@injae-kim
injae-kim
Aug 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but I was planning to maintain the existing codes
Hi @chanhyeong! I'm helping @Huiyeongkim's first contribution by https://medium.com/opensource-contributors.
Can you explain more details about existing codes that you plan to edit?
We simply want to know how you think to fix this issue 🙇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@injae-kim
I planned adding some classes like CustomIdChildElement and test cases for them.
PR author explained 'Add test case for custom ID column name', but there are no added cases.
However, as I commented before, I'm careful because I'm not a maintainer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chanhyeong
Thanks for pointing that out. I'll try adding the test and update the PR accordingly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! I just wrote my opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chanhyeong
Thank you! I’ve added the test as discussed.
f970f7b to
174defd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR causes JdbcRepositoryEmbeddedWithCollectionIntegrationTests.deleteBy() to fail.
Please make sure to run all tests, if possible against all supported databases.
You do this by running
./mvnw clean verify
or
./mvnw clean verify -Pall-dbs
The later requires Docker to run on your machine and also the acceptance of some db licenses, which you can accept by running ci/accept-third-party-license.sh. Of course, you should review any license you are accepting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather not have another full integration test class, but go with the approach you used in the reproducer of just tweaking the existing JdbcRepositoryWithCollectionsChainHsqlIntegrationTests.java
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@schauder
Thanks for pointing that out!
I’ll adjust the test case in JdbcRepositoryWithCollectionsChainHsqlIntegrationTests instead of adding a new class,
and re-run ./mvnw clean verify to make sure everything passes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@schauder
As suggested, I have updated JdbcRepositoryWithCollectionsChainHsqlIntegrationTests.java.
No new integration test class was added.
- Support @column annotation on ID fields when deleting collection entities - Fallback to parent path IDs for embedded collections - Add test for custom ID column DELETE operations Fixes DATAJDBC-2123 Signed-off-by: Huiyeongkim <huiyeong9619@naver.com>
174defd to
ab7bf64
Compare
Uh oh!
There was an error while loading. Please reload this page.
Fixes errors when deleting child entities with non-standard ID column names.
Fixes #2123