-
Notifications
You must be signed in to change notification settings - Fork 365
Consider embedded properties in the QBE queries #2100
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off the commits |
7045c5c
to
3d8fa6e
Compare
…rties in QBE Signed-off-by: mipo256 <mikhailpolivakha@gmail.com>
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 goes into the right direction. There's quite some noise and parameter and variable names are quite lengthy. Long names add complexity and from my perspective, they do not improve readability or understandability by explaining different domains.
|
||
Object actualPropertyValue = entityPropertiesAccessor.getProperty(property); | ||
|
||
if (property.isEmbedded() && actualPropertyValue != null) { |
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.
Ideally, we don't distinguish based on whether the property is an embedded one but rather whether it is an entity. At some point it should be possible to express criteria based across objects that are part of the aggregate (like it is done in JPA, we consider embeddables and references).
return Query.query(criteria); | ||
} | ||
|
||
private <T> @NotNull List<Criteria> buildCriteriaRecursive( // |
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.
Care to remove all those lingering @NotNull
annotations?
* @return query | ||
*/ | ||
private <T> Query getMappedExample(Example<T> example, RelationalPersistentEntity<?> entity) { | ||
private <T> Query getMappedExample(Example<T> example, RelationalPersistentEntity<?> persistentEntity) { |
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.
What's the reason behind the renaming?
@@ -22,23 +22,31 @@ | |||
import java.util.List; | |||
import java.util.Optional; | |||
|
|||
import org.jetbrains.annotations.NotNull; |
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.
We don't want to have Jetbrains annotations here.
) { | ||
final List<Criteria> criteriaBasedOnProperties = new ArrayList<>(); | ||
|
||
persistentEntity.doWithProperties((PropertyHandler<RelationalPersistentProperty>) property -> { |
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.
Nit: Use a for-loop instead of callback-style to reduce stack frames.
* @param criteriaBasedOnProperties collection of {@link Criteria} objects to potentially enrich. | ||
* @param currentPropertyPath the dot-separated path of the passed {@code property}. | ||
*/ | ||
private void processEmbeddedRecursively( |
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.
Looks like a duplication of the general code that collects criteria for example matching. Should be consolidated.
Closes #2099
Closes #1986
This PR enhances the
RelationalExampleMapper
by making it consider embedded properties and specifiers for them.As it turned out during resolution of #1986, the embedded properties were kind of ignored in QBE.
Current algorithm works more a less the same way, it just traverses the
Example
recursively from direct properties defined in the rootprobe
, all the way down to the deeply embedded leafs.Tests are included in the PR.
CC: @mp911de @schauder