DATAJDBC-359 - Support for arbitrary chains of entities with or without Id. #150
Conversation
Introduced a `ReadingContext` in the `EntityRowMapper` to avoid passing the `ResultSet` and the `path` all over the place.
Added a dependency test to Spring Data Relational and fixed the test failure by moving the PersistentPropertyPathExtension to core.mapping.
mp911de left a comment
mp911de left a comment There was a problem hiding this comment.
Looks good from a functional perspective. I added comments regarding code organization and readability.
| ? populateProperties(result, resultSet) // | ||
| : result; | ||
| } | ||
| RelationalPersistentProperty idProperty = entity.getIdProperty(); |
There was a problem hiding this comment.
It would make sense to move all this mapping to a MappingR2dbcConverter to keep EntityRowMapper slim (see https://github.com/spring-projects/spring-data-r2dbc/blob/master/src/main/java/org/springframework/data/r2dbc/function/convert/EntityRowMapper.java#L29). Doing so would align object mapping with the other Spring Data modules.
| | ||
| if (a instanceof DbAction.WithGeneratedId) { | ||
| | ||
| Assert.notNull(persistentEntity, |
There was a problem hiding this comment.
This code is deeply nested. How about extracting it to a class/method that makes the concept explicit?
| | ||
| template.delete(chain4, NoIdChain4.class); | ||
| | ||
| String countSelect = "SELECT COUNT(*) FROM %s"; |
There was a problem hiding this comment.
Why String.format(…) instead of having directly SELECT COUNT(*) CHAIN0? Also, the query result is not used/asserted.
| | ||
| String countSelect = "SELECT COUNT(*) FROM %s"; | ||
| jdbcTemplate.queryForObject(String.format(countSelect, "CHAIN0"),emptyMap(), Long.class); | ||
| jdbcTemplate.queryForObject(String.format(countSelect, "CHAIN0"), emptyMap(), Long.class); |
There was a problem hiding this comment.
Why String.format(…) instead of having directly SELECT COUNT(*) CHAIN0? Also, the query result is not used/asserted.
There was a problem hiding this comment.
Probably a refactoring left over.
Fixed.
| @Test // DATAJDBC-359 | ||
| public void chainedEntitiesWithoutId() throws SQLException { | ||
| | ||
| ResultSet rs = mockResultSet(asList("four", // |
There was a problem hiding this comment.
How about some mock/builder API? From just looking at the code, it's next to impossible to understand what's happening here. Check out Cassandra's RowMockUtil.
| @Test // DATAJDBC-359 | ||
| public void saveAndLoadLongChainWithoutIds() { | ||
| | ||
| NoIdChain4 chain4 = new NoIdChain4(); |
There was a problem hiding this comment.
How about some more semantic naming (inner, middle, outer). Probably, also the numbering is confusing because I'd expect to start with zero with the outermost.
There was a problem hiding this comment.
The names are actually really useful when thinking about the generated SQL, since it is kind of recursive.
Maybe it makes it easier when one thinks as Chain4 as shorthand for Chain containing 4 elements.
I added a comment to that extend.
| @Test // DATAJDBC-359 | ||
| public void idDefiningPath() { | ||
| | ||
| SoftAssertions.assertSoftly(softly -> { |
There was a problem hiding this comment.
Let's statically import assertSoftly. These deeply indented blocks make it hard to read.
| @Test // DATAJDBC-359 | ||
| public void deletingLongChainNoIdWithBackreferenceNotReferencingTheRoot() { | ||
| | ||
| assertThat(createSqlGenerator(IdIdNoIdChain.class) |
There was a problem hiding this comment.
Nit: Let's reformat this for better readability.
| */ | ||
| public static JdbcIdentifierBuilder forBackReferences(PersistentPropertyPath<RelationalPersistentProperty> path, | ||
| @Nullable Object value) { | ||
| public static JdbcIdentifierBuilder forBackReferences(PersistentPropertyPathExtension path, @Nullable Object value) { |
There was a problem hiding this comment.
We probably should keep this API id-column-count agnostic. JdbcIdentifierBuilder wasn't introduced with DATAJDBC-359 but that's a potential candidate for likely breaking changes as soon as we start supporting composite keys (Id-class or flat).
There was a problem hiding this comment.
As discussed I'll introduce that with an upcoming change.
- reduced deep nesting. - simplified code. - added assertions to test where missing. - commented on the choice of names for Chain4 and ChainNoId4. - added a little DSL for tests to make them more readable. - formatting. - made the EntityRowMapper a facade for the converter. - moved classes to avoid dependency cycles.
Deprecate remaining DataAccessStrategy types in jdbc.core and create replacements in jdbc.core.convert. Migrate using code to replacement types. Simplify warnings and if flows. Add since version to deprecations. Move MyBatisDataAccessStrategyUnitTests from core to mybatis package.
…ut Id. Introduced a `ReadingContext` in the `EntityRowMapper` to avoid passing the `ResultSet` and the `path` all over the place. Added a dependency test to Spring Data Relational and fixed the test failure by moving the PersistentPropertyPathExtension to core.mapping. Original pull request: #150.
Deprecate remaining DataAccessStrategy types in jdbc.core and create replacements in jdbc.core.convert. Migrate using code to replacement types. Simplify warnings and if flows. Add since version to deprecations. Move MyBatisDataAccessStrategyUnitTests from core to mybatis package. Original pull request: #150.
| That's merged and polished now. |
No description provided.