Issue/refactor path extension #1526
Conversation
There was a problem hiding this comment.
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.
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.
This property can be checked directly from getLeafEntity(). A path should conceptually not have an identifier property.
There was a problem hiding this comment.
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.
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.
We should honor quoting settings of the underlying table and property.
There was a problem hiding this comment.
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.
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.
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.
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
schauder force-pushed the issue/refactor-path-extension branch from 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.
schauder force-pushed the issue/refactor-path-extension branch from 86b090d to cb890c5 Compare 
| After heavy reworking we resolved this by merging d00e928 |
AggregatePathreplacesPersistentPropertyPathExtension.It gets created and cached by the
RelationalMappingContext, which should be more efficient and certainly looks nicer.Closes #1525