Fix DELETE query in derived delete operations#2128
Fix DELETE query in derived delete operations#2128Huiyeongkim wants to merge 1 commit intospring-projects:mainfrom
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
Huiyeongkim
force-pushed
the
issue-2123
branch
from
88285f7 to
1f70e88
Compare
| String name; | ||
| Set<GrandChildElement> content = new HashSet<>(); | ||
| @Id private Long id; | ||
| @Id @Column("CHILD_ID") private Long id; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@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.
@chanhyeong
Thanks for pointing that out. I'll try adding the test and update the PR accordingly
There was a problem hiding this comment.
Sure! I just wrote my opinion.
There was a problem hiding this comment.
@chanhyeong
Thank you! I’ve added the test as discussed.
Huiyeongkim
force-pushed
the
issue-2123
branch
2 times, most recently
from
f970f7b to
174defd
Compare
schauder
left a comment
schauder
left a comment
There was a problem hiding this comment.
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.
| */ | ||
| @IntegrationTest | ||
| @EnabledOnDatabase(DatabaseType.HSQL) | ||
| class JdbcRepositoryWithCollectionsCustomIdIntegrationTests { |
There was a problem hiding this comment.
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.
@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.
@schauder
As suggested, I have updated JdbcRepositoryWithCollectionsChainHsqlIntegrationTests.java.
No new integration test class was added.
schauder
added
status: waiting-for-feedback
- 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>
Huiyeongkim
force-pushed
the
issue-2123
branch
from
174defd to
ab7bf64
Compare
5 participants
Fixes errors when deleting child entities with non-standard ID column names.
Fixes #2123