-
Notifications
You must be signed in to change notification settings - Fork 346
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
Issue/refactor path extension #1526
Conversation
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 like the direction of the changes. They start reflecting the underlying metamodel. I added a few initial comments. The column and table handling of paths is coupled to tight effectively preventing certain use-cases when using embedded property holders. Also, there are some aspects that require further refinement in the sense of methods allowing traversal.
Commonly used functionality should go into utilities so that with the known requirements (e.g. composite primary keys) we do not need to break our API but rather keep a clear separation between concerns.
* @since 3.2 | ||
* @author Jens Schauder | ||
*/ | ||
public class AggregatePath { |
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.
Let's extract AggregatePath
as interface with the default implementation being package-private, similar to PersistentPropertyPath
.
...relational/src/main/java/org/springframework/data/relational/core/mapping/AggregatePath.java
Outdated
Show resolved
Hide resolved
...relational/src/main/java/org/springframework/data/relational/core/mapping/AggregatePath.java
Outdated
Show resolved
Hide resolved
/** | ||
* @return {@literal true} if this path represents an entity which has an Id attribute. | ||
*/ | ||
public boolean hasIdProperty() { |
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 property can be checked directly from getLeafEntity()
. A path should conceptually not have an identifier 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.
I don't really follow. What makes this different than the other boolean properties?
* | ||
* @return A path that starts just as this path but is shorter. Guaranteed to be not {@literal null}. | ||
*/ | ||
public AggregatePath getIdDefiningParentPath() { |
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.
It would be neat to refactor this method to a path-traversal method to walk the path hierarchy with a Predicate
. The concrete functionality here would fit into a utility as paths should not be tied to identifiers.
...relational/src/main/java/org/springframework/data/relational/core/mapping/AggregatePath.java
Outdated
Show resolved
Hide resolved
|
||
if (path.getLength() == 1) { | ||
Assert.notNull(prefix, "Prefix mus not be null"); | ||
return StringUtils.hasText(prefix) ? SqlIdentifier.quoted(prefix) : 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.
We should honor quoting settings of the underlying table and 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.
I disagree. This is for aliases only. They are only used internally in SQL statements.
* | ||
* @return a path. Guaranteed to be not {@literal null}. | ||
*/ | ||
private AggregatePath getTableOwningAncestor() { |
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 method is another indication of the need to traverse the AggregatePath
hierarchy with having an utility for querying the model.
|
||
Assert.state(path != null, "Path is null"); | ||
|
||
return assembleColumnName(path.getLeafProperty().getColumnName()); |
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'm not sure how this remains applicable for properties that reference an embedded holder (embedded properties object, composite Id class) as these map to multiple inner columns. (e.g. person.address
where address
is class Address {String street; String zip;}
.
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.
The path includes those embedded entities. Therefore the relevant paths would be address/street
and address/zip
.
assembleColumnName
takes care of prefixing embedded property names appropriately.
...relational/src/main/java/org/springframework/data/relational/core/mapping/AggregatePath.java
Outdated
Show resolved
Hide resolved
AggregatePath replaces PersistentPropertyPathExtension. It gets created and cached by the RelationalMappingContext, which should be more efficient and certainly looks nicer. Closes #1525
45c55c3
to
86b090d
Compare
Using isRoot() instead of path == null. Rename extendBy to append. Move getRequiredLeafEntity closer to getLeafEntity. Replace matches with equals.
This forces us to make some methods public, that have been private before.
86b090d
to
cb890c5
Compare
After heavy reworking we resolved this by merging d00e928 |
AggregatePath
replacesPersistentPropertyPathExtension
.It gets created and cached by the
RelationalMappingContext
, which should be more efficient and certainly looks nicer.Closes #1525