-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
DATAJPA-1822 - Ensure left outer join for nested optional attributes #436
DATAJPA-1822 - Ensure left outer join for nested optional attributes #436
Conversation
@pblanchardie Please sign the Contributor License Agreement! Click here to manually synchronize the status of this Pull Request. See the FAQ for frequently asked questions. |
@pblanchardie Thank you for signing the Contributor License Agreement! |
2deabac
to
6146ab4
Compare
84f947c
to
3017539
Compare
Hi @schauder, I would appreciate some feedback and don't hesitate to ask for clarification. |
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.
Two integration tests fail with EclipseLink:
Please make sure to run all tests with ./mvnw clean verify
[ERROR] Failures:
[ERROR] EclipseLinkQueryUtilsIntegrationTests>QueryUtilsIntegrationTests.createsLeftJoinForNonOptionalToOneWithNestedOptional:147
Expecting empty but was: [org.eclipse.persistence.internal.jpa.querydef.JoinImpl@56bb4183]
[ERROR] EclipseLinkQueryUtilsIntegrationTests>QueryUtilsIntegrationTests.createsLeftJoinForOptionalToOneWithNestedNonOptional:131
Expecting empty but was: [org.eclipse.persistence.internal.jpa.querydef.JoinImpl@7b1248b8]
27e7c21
to
4bc11ac
Compare
Temporarily fixed by skipping these tests for EclipseLink. This is caused by QueryUtils.java#L699 producing an inner join. The existing workaround using |
4bc11ac
to
0860d7f
Compare
Added a workaround for EclipseLink (inspired by the existing workaround using |
Formatting. Preferring iterative code of streams. Adding `@author` tags for previous changes. Original pull request: #436
Formatting. Preferring iterative code of streams. Adding `@author` tags for previous changes. Original pull request: #436
Formatting. Preferring iterative code of streams. Adding `@author` tags for previous changes. Original pull request: #436
Thanks for bringing this to the attention of the EclipseLink team. |
Thanks, this is merged, polished and back ported. |
Formatting. Preferring iterative code of streams. Adding `@author` tags for previous changes. Original pull request: #436
Formatting. Preferring iterative code of streams. Adding `@author` tags for previous changes. Original pull request: #436
Formatting. Preferring iterative code of streams. Adding `@author` tags for previous changes. Original pull request: #436
Made test methods public for JUnit 4. Original pull request: #436
Suggested fix for DATAJPA-1822.
It is similar to DATAJPA-1418 (#294).
Reading the git history of
toExpressionRecursively
shows that multiples attempts have been made to deal withLEFT OUTER JOIN
andPath.get
depending on several conditions.The
isForSelection
parameter, when set totrue
bytoJpaOrder
, partly solves the problem.The new
hasRequiredOuterJoin
parameter keeps track of previous outer joins to avoid further inner joins on the same path.toExpressionRecursively
has been rewritten to be more readable and create or reuse joins properly.