-
Notifications
You must be signed in to change notification settings - Fork 367
DATAJDBC-131 - Basic support for Maps #17
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
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.
There are also several places that have the isMap ternary operation. I don't know if this is the best way, or if it implies some other structure is needed to avoid sprinkling this logic everywhere. Thoughts?
@@ -193,14 +193,18 @@ public long count(Class<?> domainType) { | |||
} | |||
|
|||
@Override | |||
public <T> Iterable<T> findAllByProperty(Object rootId, JdbcPersistentProperty property) { | |||
public Iterable findAllByProperty(Object rootId, JdbcPersistentProperty 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'm not a fan of dropping the generic signatures. I sense we need to dig a little deeper to solve this one. Otherwise, things should be wrapped with the proper suppression annotations.
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.
yeah, I was half-hearted fiddling around with the generics. I put them back in, along with @SuppressWarnings
. I already created a separate issue for cleaning this up all over the code base. I was somewhat inconsistent in the past about this.
@@ -149,7 +149,7 @@ public void delete(Object rootId, PropertyPath propertyPath) { | |||
} | |||
|
|||
@Override | |||
public <T> void deleteAll(PropertyPath propertyPath) { | |||
public void deleteAll(PropertyPath propertyPath) { |
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 alters the signature from the interface, generating warnings.
Regarding the "isMap" issue. I for now extracted it in a separate method because eventually, it won't be just for I agree there, there might be a missing structure hiding in there somewhere but currently, I'm not able to identify it. |
Aggregate roots with properties of type java.util.Map get properly inserted, updated and deleted. Known limitations: - Naming strategy does not allow for multiple references via Set, Map or directly to the same entity. - The table for the referenced Entity contains the column for the map key. A workaround for that would be to manipulate the DbActions in the AggregateChange yourself.
Put generics back in and added @SupressWarnings where this caused warnings. Extracted the test “isMap” into a “isQualified” method on the property.
2d1f511
to
1579a3c
Compare
Resolved via 1291b5d. |
We now provide an annotation-based activation model for R2DBC repositories. Configuration classes can be annotated with @EnableR2dbcRepositories and configuration infrastructure will scan for bean definitions to implement declared R2DBC repositories. Original pull request: #17.
We now provide an abstract R2DBC configuration that registers beans required for R2DBC's DatabaseClient. Original pull request: #17.
Add missing header & package-info. Update nullability and add tests. Original pull request: #17.
Aggregate roots with properties of type java.util.Map get properly inserted, updated and deleted.
Known limitations:
A workaround for that would be to manipulate the DbActions in the AggregateChange yourself.