SLING-8116 implement OSGI Converter for default methods #8
Conversation
|
@rombert , @cziegeler any feedback is appreciated. |
|
Did not get much time to review it fully but I think that we should preserve backwards compatibility with the previous implementation. So changes in tests are a sign we should override the default converters behaviour and supply our own. |
|
The question then becomes, should we maintain backward compatibility with how the ValueMap works or the ValueMapDecorator? |
|
I'll review the changes to the test cases. My focus will be to confirm that the behaviour I'm testing will match the expectations of what is defined in the ValueMap and update the converter accordingly |
| @@ -52,7 +52,7 @@ | |||
|
|
|||
| <properties> | |||
| <site.jira.version.id>12314252</site.jira.version.id> | |||
| <sling.java.version>7</sling.java.version> | |||
| <sling.java.version>8</sling.java.version> | |||
There was a problem hiding this comment.
This is OK but I think should be called out in the commit message, to make it clear it was intentional.
|
|
||
| private static Converter converter; | ||
|
|
||
| private static synchronized Converter getConverter() { |
There was a problem hiding this comment.
I think we should be careful when adding syncrhonized static blocks in this class - it's bound to create contention under heavy load. Perhaps using another singleton pattern such as the enum-based one will get us lazy initialisation and also remove any contention.
There was a problem hiding this comment.
Not familiar with that pattern, I'll see what I can implement
| return Date.from(Instant.parse(date)); | ||
| } | ||
|
|
||
| public static Date toDate(Calendar cal) { |
There was a problem hiding this comment.
Does this method need to be public?
There was a problem hiding this comment.
Nope, I'll change that.
@JEBailey - by that do you mean the variation you spotted on the mailing list?
I would slightly lean towards not returning null values but TBH I don't have that much knowledge in ValueMap-land |
|
@rombert Yes, it appears that the ValueMap decorator was returning something different. I need to go through and double check all the changes. FWIW I'm a big fan of returning empty collections over a null value myself, but that doesn't appear to be the way that Sling has done things in the past. Although I couldn't predict the outcome of making that change to returning empty sets |
|
@rombert The only thing I changed in the tests were the results of the ValueMapDecorator. Which has logic that is inconsistent with every other ValueMap implementation. All other ValueMap implementations I've found assume that failure to convert one type to another returns a null. |
removed unused import from ValueMap, removed NotNull annotation from CompositeValueMap to match prior implementation. And verified that jre was set to version 8. Which was done to support default methods.
|
@rombert did I address your concerns? Or did I miss something? |
JEBailey
dismissed
rombert’s stale review
changes were made based on feedback
Implements default methods in the ValueMap interface for the get(key,class) and get(key, default) methods that will rely on the Converter class from the OSGi standard.
This will result in the following
ValueMapDecorator will now return null when converting an existing array to an unsupported Type as other ValueMap implementations do.
ValueMapDecorator will return a null when converting a missing property to an array, as other ValueMap implementations do.
ValueMaps relying on the default methods will now support the autoboxing to primitive arrays during conversion
ValueMaps relying on the default methods will return true for a conversion of a char or number that is not '0' to a Boolean
ValueMaps relying on the default methods will return false for any value that can not be converted to Boolean.TRUE
Will return false for a null property
package has been updated to jre8 to support default methods