-
Notifications
You must be signed in to change notification settings - Fork 6k
Add GenericConversionService with support for UUID and Strings #6141
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
acl/src/main/java/org/springframework/security/acls/jdbc/AclClassIdUtils.java
Outdated
Show resolved
Hide resolved
acl/src/main/java/org/springframework/security/acls/jdbc/AclClassIdUtils.java
Outdated
Show resolved
Hide resolved
acl/src/test/java/org/springframework/security/acls/jdbc/AclClassIdUtilsTest.java
Outdated
Show resolved
Hide resolved
acl/src/test/java/org/springframework/security/acls/jdbc/AclClassIdUtilsTest.java
Outdated
Show resolved
Hide resolved
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.
@nenaraab Thank you for addressing all these items. There are two that I think I miscommunicated on, so I've tried clarifying that in one more review.
Once you have this commit ready, would you squash your commits, please? Thanks again for the PR!
acl/src/main/java/org/springframework/security/acls/jdbc/AclClassIdUtils.java
Outdated
Show resolved
Hide resolved
acl/src/test/java/org/springframework/security/acls/jdbc/AclClassIdUtilsTest.java
Show resolved
Hide resolved
acl/src/main/java/org/springframework/security/acls/jdbc/AclClassIdUtils.java
Show resolved
Hide resolved
acl/src/test/java/org/springframework/security/acls/jdbc/AclClassIdUtilsTest.java
Show resolved
Hide resolved
Sorry, i was not sure how to correct a pull request best... If i would have squashed it, would it then look prettier, i.e. better readable for review? The additional String identifier was introduced by intention, to make sure that the String2String (and not only LongAsString2String) conversion works as expected. Thanks for your time, help and review :-) |
…l_object_identity.object_id_identity Co-Authored-By: nenaraab <[email protected]>
@nenaraab Squashing helps with backpatches mostly, but yes it also helps a bit with the review process. |
@jzheaux Before I'v started with the code change, i created the test to simulate the issues i've seen within my application when not specifying any ConversionService. Before of this pull request: this test case would have been green. @Test
public void shouldReturnStringWhenStringClassIdType() throws SQLException {
// given
String identifier = DEFAULT_IDENTIFIER_AS_STRING;
// when
Serializable newIdentifier = aclClassIdUtils.identifierFrom(identifier, resultSet);
...
} ...as “999” can be transformed to Long 999L. But the one below not: @Test
public void shouldReturnStringWhenStringClassIdType() throws SQLException {
// given
String identifier = "MY_STRING_IDENTIFIER";
// when
Serializable newIdentifier = aclClassIdUtils.identifierFrom(identifier, resultSet);
....
} It gives me an That's why i've decided to use a real "String" Identifier ... |
So, the commits are squashed... anything else to change :-) |
Thanks again, @nenaraab for the PR! This is now merged into |
Motivation
As of now only
acl_object_identity.object_id_identity
of typejava.util.Long
are supported. In case you use identifiers of typejava.lang.String
orjava.util.UUID
the application needs to provide a ConversionService. This commit comes with two default Converters to support Strings and UUIDs.related commit
pwheel@8efec02
related issue(s)