You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sling.apache.org by Stefan Seifert <ss...@pro-vision.de> on 2016/12/19 11:40:38 UTC
[api] type conversion in ValueMapDecorator
SLING-6416 revealed a problem in the type conversion logic of the ValueMapDecorator [1] of the Sling API not supporting BigDecimal conversions.
as the TODO indicates (present there for nearly 8 years) the current conversion logic is not very smart (and not very efficient), and supports less conversions than the JCR value map implementation. in jcr.resource a set of internal converters takes care of the conversion (NumberConverter, CalendarConverter, BooleanConverter etc. [2]).
the contract of the ValueMap interface does not define which conversions should be supported exactly, but one might expect that a map enhanced with ValueMapDecorator should behave at least for the conversion rules the same way as the JCR value map.
in this case we should move these converter implementation to the sling API and use them in both ValueMapDecorator and jcr.resource implementation.
WDYT?
stefan
[1] https://github.com/apache/sling/blob/trunk/bundles/api/src/main/java/org/apache/sling/api/wrappers/ValueMapDecorator.java#L64
[2] https://github.com/apache/sling/tree/trunk/bundles/jcr/resource/src/main/java/org/apache/sling/jcr/resource/internal/helper
Re: [api] type conversion in ValueMapDecorator
Posted by Konrad Windszus <ko...@gmx.de>.
Very reasonable indeed. It would probably be good to extend the javadoc of the ValueMap at the same time to make clearer what kind of conversions are supposed to work.
Konrad
> On 19 Dec 2016, at 12:40, Stefan Seifert <ss...@pro-vision.de> wrote:
>
> SLING-6416 revealed a problem in the type conversion logic of the ValueMapDecorator [1] of the Sling API not supporting BigDecimal conversions.
>
> as the TODO indicates (present there for nearly 8 years) the current conversion logic is not very smart (and not very efficient), and supports less conversions than the JCR value map implementation. in jcr.resource a set of internal converters takes care of the conversion (NumberConverter, CalendarConverter, BooleanConverter etc. [2]).
>
> the contract of the ValueMap interface does not define which conversions should be supported exactly, but one might expect that a map enhanced with ValueMapDecorator should behave at least for the conversion rules the same way as the JCR value map.
>
> in this case we should move these converter implementation to the sling API and use them in both ValueMapDecorator and jcr.resource implementation.
>
> WDYT?
>
> stefan
>
> [1] https://github.com/apache/sling/blob/trunk/bundles/api/src/main/java/org/apache/sling/api/wrappers/ValueMapDecorator.java#L64
> [2] https://github.com/apache/sling/tree/trunk/bundles/jcr/resource/src/main/java/org/apache/sling/jcr/resource/internal/helper
>
>
RE: [api] type conversion in ValueMapDecorator
Posted by Stefan Seifert <ss...@pro-vision.de>.
>* Bloat: Please consider to not bloat the conversion functionality and
>carefully consider which conversion really makes sense and which is just
>nice-to-have-because-JCR-has-it.
i've created at ticket about which conversion rules we speak SLING-6420 and outlinded them in a wiki page https://cwiki.apache.org/confluence/x/OQkIB
the main features missing in ValueMapDecorator are conversion rules for Date, Calendar, BigDecimal, byte, short. some conversion rules from jcr.resource are questionable (e.g. convert boolean true -> 1 -> Date).
stefan
RE: [api] type conversion in ValueMapDecorator
Posted by Stefan Seifert <ss...@pro-vision.de>.
>* API: I suggest to break this out of the Sling API bundle into a separate
>bundle. One of the problems I see we have with the API bundle is that is
>becoming more and more a collection of API with independent lifecycle.
>Particularly the Resource API has its own life warranting its own bundle.
>But this type conversion is another one. (And yes I know we also have a
>packaging problem in the API bundle, which might have to be fixed)
yes, i see this point
problem: ValueMapDecorator *is* part of the API. if we move the conversion logic to a new bundle Sling API needs to depend on it.
unless we deprecate ValueMapDecorator in the API and move it somewhere else. currently API has not dependencies except javax.servlet and slf4j-api.
stefan
Re: [api] type conversion in ValueMapDecorator
Posted by Felix Meschberger <fm...@adobe.com>.
Hi
Granted, consolidation probably makes sense, but there are caveats:
* Bloat: Please consider to not bloat the conversion functionality and carefully consider which conversion really makes sense and which is just nice-to-have-because-JCR-has-it.
* API: I suggest to break this out of the Sling API bundle into a separate bundle. One of the problems I see we have with the API bundle is that is becoming more and more a collection of API with independent lifecycle. Particularly the Resource API has its own life warranting its own bundle. But this type conversion is another one. (And yes I know we also have a packaging problem in the API bundle, which might have to be fixed)
Regards
Felix
> Am 19.12.2016 um 12:59 schrieb Julian Sedding <js...@gmail.com>:
>
> +1 It sounds sensible to move the logic to Sling API and re-use it in
> the JCR value map.
>
> Regards
> Julian
>
> On Mon, Dec 19, 2016 at 12:40 PM, Stefan Seifert <ss...@pro-vision.de> wrote:
>> SLING-6416 revealed a problem in the type conversion logic of the ValueMapDecorator [1] of the Sling API not supporting BigDecimal conversions.
>>
>> as the TODO indicates (present there for nearly 8 years) the current conversion logic is not very smart (and not very efficient), and supports less conversions than the JCR value map implementation. in jcr.resource a set of internal converters takes care of the conversion (NumberConverter, CalendarConverter, BooleanConverter etc. [2]).
>>
>> the contract of the ValueMap interface does not define which conversions should be supported exactly, but one might expect that a map enhanced with ValueMapDecorator should behave at least for the conversion rules the same way as the JCR value map.
>>
>> in this case we should move these converter implementation to the sling API and use them in both ValueMapDecorator and jcr.resource implementation.
>>
>> WDYT?
>>
>> stefan
>>
>> [1] https://github.com/apache/sling/blob/trunk/bundles/api/src/main/java/org/apache/sling/api/wrappers/ValueMapDecorator.java#L64
>> [2] https://github.com/apache/sling/tree/trunk/bundles/jcr/resource/src/main/java/org/apache/sling/jcr/resource/internal/helper
>>
>>
Re: [api] type conversion in ValueMapDecorator
Posted by Julian Sedding <js...@gmail.com>.
+1 It sounds sensible to move the logic to Sling API and re-use it in
the JCR value map.
Regards
Julian
On Mon, Dec 19, 2016 at 12:40 PM, Stefan Seifert <ss...@pro-vision.de> wrote:
> SLING-6416 revealed a problem in the type conversion logic of the ValueMapDecorator [1] of the Sling API not supporting BigDecimal conversions.
>
> as the TODO indicates (present there for nearly 8 years) the current conversion logic is not very smart (and not very efficient), and supports less conversions than the JCR value map implementation. in jcr.resource a set of internal converters takes care of the conversion (NumberConverter, CalendarConverter, BooleanConverter etc. [2]).
>
> the contract of the ValueMap interface does not define which conversions should be supported exactly, but one might expect that a map enhanced with ValueMapDecorator should behave at least for the conversion rules the same way as the JCR value map.
>
> in this case we should move these converter implementation to the sling API and use them in both ValueMapDecorator and jcr.resource implementation.
>
> WDYT?
>
> stefan
>
> [1] https://github.com/apache/sling/blob/trunk/bundles/api/src/main/java/org/apache/sling/api/wrappers/ValueMapDecorator.java#L64
> [2] https://github.com/apache/sling/tree/trunk/bundles/jcr/resource/src/main/java/org/apache/sling/jcr/resource/internal/helper
>
>
Re: [api] type conversion in ValueMapDecorator
Posted by Bertrand Delacretaz <bd...@apache.org>.
On Tue, Dec 20, 2016 at 3:38 PM, Stefan Seifert <ss...@pro-vision.de> wrote:
> ...- instead we just add some more conversion rules for Byte, Short, BigInteger, Calendar,
> Date which are currently missing in internal implementation of the ValueMapDecorator...
works for me!
> ...with this we have no code reuse...
I suppose one could still embed that class in a different bundle...not
full clean reusability but not impossible either.
-Bertrand
Re: [api] type conversion in ValueMapDecorator
Posted by Carsten Ziegeler <cz...@apache.org>.
Stefan Seifert wrote
>
>> Sounds good, I think we should also improve the decorator to check
>> whether it is decorating a ValueMap (as opposed to a map) and then
>> potentially use the methods of the wrapped ValueMap for conversion as well
>
> good idea, i've created a separate ticket SLING-6421 for this. in this case i suppose we should just delegate all methods to the decorated ValueMap to avoid mixing probably different conversion concepts?
>
Yes, sounds good to me
Thanks
Carsten
--
Carsten Ziegeler
Adobe Research Switzerland
cziegeler@apache.org
RE: [api] type conversion in ValueMapDecorator
Posted by Stefan Seifert <ss...@pro-vision.de>.
>Sounds good, I think we should also improve the decorator to check
>whether it is decorating a ValueMap (as opposed to a map) and then
>potentially use the methods of the wrapped ValueMap for conversion as well
good idea, i've created a separate ticket SLING-6421 for this. in this case i suppose we should just delegate all methods to the decorated ValueMap to avoid mixing probably different conversion concepts?
stefan
Re: [api] type conversion in ValueMapDecorator
Posted by Carsten Ziegeler <cz...@apache.org>.
Sounds good, I think we should also improve the decorator to check
whether it is decorating a ValueMap (as opposed to a map) and then
potentially use the methods of the wrapped ValueMap for conversion as well
Carsten
Stefan Seifert wrote
> thinking about the arguments from different directions i came up with a more simple proposal which is perhaps the best compromise:
>
> - we do not touch the conversion implementation in the jcr.resource implementations which has a lot of good conversion rules, but also some questionable we do not want to apply all.
> - we do not add and conversion support in the Sling API
> - we do not use osgi conversion service
> - instead we just add some more conversion rules for Byte, Short, BigInteger, Calendar, Date which are currently missing in internal implementation of the ValueMapDecorator, and apply some optimiziations to the implementation. this is outlined in SLING-6420.
>
> with this we have no code reuse, but code reuse would lead to either
> - potential overengineering adding new services interfaces and implementations for conversion
> - having to deal with the questionable rules from jcr.resource in the generic impl as well
> - introducing dependencies to the Sling API
> - bloating things
>
> if no one objects I would follow this way in SLING-6420, which would also help solve the initial mock problem SLING-6416.
>
> Stefan
>
>
>> -----Original Message-----
>> From: Stefan Seifert [mailto:sseifert@pro-vision.de]
>> Sent: Monday, December 19, 2016 12:41 PM
>> To: dev@sling.apache.org
>> Subject: [api] type conversion in ValueMapDecorator
>>
>> SLING-6416 revealed a problem in the type conversion logic of the
>> ValueMapDecorator [1] of the Sling API not supporting BigDecimal
>> conversions.
>>
>> as the TODO indicates (present there for nearly 8 years) the current
>> conversion logic is not very smart (and not very efficient), and supports
>> less conversions than the JCR value map implementation. in jcr.resource a
>> set of internal converters takes care of the conversion (NumberConverter,
>> CalendarConverter, BooleanConverter etc. [2]).
>>
>> the contract of the ValueMap interface does not define which conversions
>> should be supported exactly, but one might expect that a map enhanced with
>> ValueMapDecorator should behave at least for the conversion rules the same
>> way as the JCR value map.
>>
>> in this case we should move these converter implementation to the sling API
>> and use them in both ValueMapDecorator and jcr.resource implementation.
>>
>> WDYT?
>>
>> stefan
>>
>> [1]
>> https://github.com/apache/sling/blob/trunk/bundles/api/src/main/java/org/ap
>> ache/sling/api/wrappers/ValueMapDecorator.java#L64
>> [2]
>> https://github.com/apache/sling/tree/trunk/bundles/jcr/resource/src/main/ja
>> va/org/apache/sling/jcr/resource/internal/helper
>>
>>
>
>
>
--
Carsten Ziegeler
Adobe Research Switzerland
cziegeler@apache.org
RE: [api] type conversion in ValueMapDecorator
Posted by Stefan Seifert <ss...@pro-vision.de>.
thinking about the arguments from different directions i came up with a more simple proposal which is perhaps the best compromise:
- we do not touch the conversion implementation in the jcr.resource implementations which has a lot of good conversion rules, but also some questionable we do not want to apply all.
- we do not add and conversion support in the Sling API
- we do not use osgi conversion service
- instead we just add some more conversion rules for Byte, Short, BigInteger, Calendar, Date which are currently missing in internal implementation of the ValueMapDecorator, and apply some optimiziations to the implementation. this is outlined in SLING-6420.
with this we have no code reuse, but code reuse would lead to either
- potential overengineering adding new services interfaces and implementations for conversion
- having to deal with the questionable rules from jcr.resource in the generic impl as well
- introducing dependencies to the Sling API
- bloating things
if no one objects I would follow this way in SLING-6420, which would also help solve the initial mock problem SLING-6416.
Stefan
>-----Original Message-----
>From: Stefan Seifert [mailto:sseifert@pro-vision.de]
>Sent: Monday, December 19, 2016 12:41 PM
>To: dev@sling.apache.org
>Subject: [api] type conversion in ValueMapDecorator
>
>SLING-6416 revealed a problem in the type conversion logic of the
>ValueMapDecorator [1] of the Sling API not supporting BigDecimal
>conversions.
>
>as the TODO indicates (present there for nearly 8 years) the current
>conversion logic is not very smart (and not very efficient), and supports
>less conversions than the JCR value map implementation. in jcr.resource a
>set of internal converters takes care of the conversion (NumberConverter,
>CalendarConverter, BooleanConverter etc. [2]).
>
>the contract of the ValueMap interface does not define which conversions
>should be supported exactly, but one might expect that a map enhanced with
>ValueMapDecorator should behave at least for the conversion rules the same
>way as the JCR value map.
>
>in this case we should move these converter implementation to the sling API
>and use them in both ValueMapDecorator and jcr.resource implementation.
>
>WDYT?
>
>stefan
>
>[1]
>https://github.com/apache/sling/blob/trunk/bundles/api/src/main/java/org/ap
>ache/sling/api/wrappers/ValueMapDecorator.java#L64
>[2]
>https://github.com/apache/sling/tree/trunk/bundles/jcr/resource/src/main/ja
>va/org/apache/sling/jcr/resource/internal/helper
>
>
Re: [api] type conversion in ValueMapDecorator
Posted by Bertrand Delacretaz <bd...@apache.org>.
On Tue, Dec 20, 2016 at 12:32 PM, Stefan Seifert <ss...@pro-vision.de> wrote:
> ...then we will effectively put the conversion logic into the Sling API bundle itself?
> this would be an easy solution....
But that's a "sacred" bundle that we'd like to touch as little as
possible, and we'd have to release it every time we need a change in
that conversion code...so IMO this only works if that conversion code
is really stable and has excellent test coverage. Which might be the
case of the current code that you want to move, I haven't checked.
-Bertrand
Re: [api] type conversion in ValueMapDecorator
Posted by Carsten Ziegeler <cz...@apache.org>.
Stefan Seifert wrote
>
>> Whatever we do, we should rather build utility (static) methods for the
>> conversion or have a static way of getting the converter.
>> Creating an OSGi service out of this is imho overkill.
>
> then we will effectively put the conversion logic into the Sling API bundle itself?
> this would be an easy solution.
>
Yes, I think that's the best option, otherwise you create unnecessary
dependencies for a simple util
Carsten
--
Carsten Ziegeler
Adobe Research Switzerland
cziegeler@apache.org
RE: [api] type conversion in ValueMapDecorator
Posted by Stefan Seifert <ss...@pro-vision.de>.
>Whatever we do, we should rather build utility (static) methods for the
>conversion or have a static way of getting the converter.
>Creating an OSGi service out of this is imho overkill.
then we will effectively put the conversion logic into the Sling API bundle itself?
this would be an easy solution.
stefan
Re: [api] type conversion in ValueMapDecorator
Posted by Carsten Ziegeler <cz...@apache.org>.
Stefan Seifert wrote
>
>>> ...so, what about defining only an "conversion interface" in the Sling
>> API, limit it per
>>> javadocs to support only a fixes list of types and conversions, and start
>> with providing
>>> an own implementation in a new bundle based on what is in jcr.resource
>> today...
>>
>> +1 on having our own API while the OSGi one stabilizes, and maybe we
>> can already use the Felix implementation that you mention underneath?
>>
>> Temporarily forking that Felix code in Sling is ok and would make it
>> easier to control updates to the parts that we use, without depending
>> on Felix releases of that module.
>
> in my pov our conversion API should be much simpler than the OSGi one, which in the current draft supports a lot of extra functionality like custom rules, copying etc. we should limit the scope of the interface to what is needed for valuemaps. so our use case is not exactly the same as the OSGi use case which has a much broader scope.
>
> for the impl it would be easier to start with what we have in jcr.resource today, but we could also fork the code and embed it in our own impl bundle until it is released if we think this makes more sense in the long run. forking and syncing with further updates in felix makes additional effort. independently of the way we choose we need a good set of unit test coverage of the valuemap-related conversions we want to support, so it's easy to switch the implementation later.
>
Whatever we do, we should rather build utility (static) methods for the
conversion or have a static way of getting the converter.
Creating an OSGi service out of this is imho overkill.
Carsten
--
Carsten Ziegeler
Adobe Research Switzerland
cziegeler@apache.org
RE: [api] type conversion in ValueMapDecorator
Posted by Stefan Seifert <ss...@pro-vision.de>.
>> ...so, what about defining only an "conversion interface" in the Sling
>API, limit it per
>> javadocs to support only a fixes list of types and conversions, and start
>with providing
>> an own implementation in a new bundle based on what is in jcr.resource
>today...
>
>+1 on having our own API while the OSGi one stabilizes, and maybe we
>can already use the Felix implementation that you mention underneath?
>
>Temporarily forking that Felix code in Sling is ok and would make it
>easier to control updates to the parts that we use, without depending
>on Felix releases of that module.
in my pov our conversion API should be much simpler than the OSGi one, which in the current draft supports a lot of extra functionality like custom rules, copying etc. we should limit the scope of the interface to what is needed for valuemaps. so our use case is not exactly the same as the OSGi use case which has a much broader scope.
for the impl it would be easier to start with what we have in jcr.resource today, but we could also fork the code and embed it in our own impl bundle until it is released if we think this makes more sense in the long run. forking and syncing with further updates in felix makes additional effort. independently of the way we choose we need a good set of unit test coverage of the valuemap-related conversions we want to support, so it's easy to switch the implementation later.
stefan
Re: [api] type conversion in ValueMapDecorator
Posted by Bertrand Delacretaz <bd...@apache.org>.
Hi,
On Tue, Dec 20, 2016 at 11:50 AM, Stefan Seifert <ss...@pro-vision.de> wrote:
> ...so, what about defining only an "conversion interface" in the Sling API, limit it per
> javadocs to support only a fixes list of types and conversions, and start with providing
> an own implementation in a new bundle based on what is in jcr.resource today...
+1 on having our own API while the OSGi one stabilizes, and maybe we
can already use the Felix implementation that you mention underneath?
Temporarily forking that Felix code in Sling is ok and would make it
easier to control updates to the parts that we use, without depending
on Felix releases of that module.
-Bertrand
RE: [api] type conversion in ValueMapDecorator
Posted by Stefan Seifert <ss...@pro-vision.de>.
ok, others agree as well it is a good idea to enhance the conversion support in ValueMapDecorator.
now i'm wondering how we can share the conversion logic for the JCR data types between Sling API and jcr.resource. one solution would be create a new package in sling API, move the converters there and perhaps refactor the API a bit to support a single point of entry which is able to convert an "Object" to "anything" (singleton or array of commonly used data types like primitives, date, calendar, bigdecimal etc.).
but this feels very similar to the OSGi object conversion service (RFP-169, [1]), which also has a first implementation in felix [2]. this has much more features. but I think we definitely do not want a dependency of the Sling API to this conversion service, especially as the spec is not finalized and nor release exists yet.
so, what about defining only an "conversion interface" in the Sling API, limit it per javadocs to support only a fixes list of types and conversions, and start with providing an own implementation in a new bundle based on what is in jcr.resource today. we may replace it later with an impl based on OSGi conversion service.
stefan
[1] https://github.com/osgi/design/raw/master/rfps/rfp-0169-Object-Conversion.pdf
[2] https://github.com/apache/felix/tree/trunk/converter
>-----Original Message-----
>From: Stefan Seifert [mailto:sseifert@pro-vision.de]
>Sent: Monday, December 19, 2016 12:41 PM
>To: dev@sling.apache.org
>Subject: [api] type conversion in ValueMapDecorator
>
>SLING-6416 revealed a problem in the type conversion logic of the
>ValueMapDecorator [1] of the Sling API not supporting BigDecimal
>conversions.
>
>as the TODO indicates (present there for nearly 8 years) the current
>conversion logic is not very smart (and not very efficient), and supports
>less conversions than the JCR value map implementation. in jcr.resource a
>set of internal converters takes care of the conversion (NumberConverter,
>CalendarConverter, BooleanConverter etc. [2]).
>
>the contract of the ValueMap interface does not define which conversions
>should be supported exactly, but one might expect that a map enhanced with
>ValueMapDecorator should behave at least for the conversion rules the same
>way as the JCR value map.
>
>in this case we should move these converter implementation to the sling API
>and use them in both ValueMapDecorator and jcr.resource implementation.
>
>WDYT?
>
>stefan
>
>[1]
>https://github.com/apache/sling/blob/trunk/bundles/api/src/main/java/org/ap
>ache/sling/api/wrappers/ValueMapDecorator.java#L64
>[2]
>https://github.com/apache/sling/tree/trunk/bundles/jcr/resource/src/main/ja
>va/org/apache/sling/jcr/resource/internal/helper
>
>