Hey Martin,
I didn't review all the code yet, but here are my first remarks.
*General*
Instead of having all options as strings, I think it would be better to
provide them as implementations.
The advantages of doing so:
- The spec provides stronger guarantees on the default behaviour
- This avoids having the same code being rewritten over and over by
implementations
- Makes the spec more useful
This is something that can be done once and for all.
*Adapter*
The methods in Adapter could be called adapt or adaptFrom/adaptTo.
Why not use type parameters for Adapter, this is much nicer. The end user
will have to do a cast inside his ser/de methods,
impl ~5 methods just to provide some dummy ser/de? IMO that's way too much.
Something like:
interface Adapter<BoundType, ValueType> {
ValueType adaptTo(BoundyType obj);
BoundType adaptFrom(ValueType json);
}
*PropertyVisibilityStrategy*
Member does not provide access to annotations, handling things like
@JsonTransient requires access to those.
*PropertyOrderStrategy*
Reflection gives perhaps to much details/constraints. ANY could perhaps be
better = we don't provide any ordering guarantee, don't rely on it.
Is it called with the "renamed" name or the originally resolved one?
Only the class might not be enough information. How would one match the
name to some property if he needs an annotation from it or whatever else?
But anyway, except of providing a consistent ordering I don't think there
is much value in making ordering customizable.
I'll have a look at the rest later.
Eugen
2015-05-30 13:12 GMT+02:00 Martin Vojtek <voytoo_at_gmail.com>:
> Hi Experts,
>
> I have pushed initial version of custom mapping api specification.
> Javadocs will be improved asap.
>
> I am looking forward to receiving your feedback.
>
> Thanks,
>
> MartinV
>
> On Wed, May 13, 2015 at 6:21 PM, Martin Grebac <martin.grebac_at_oracle.com>
> wrote:
>
>> On 13.05.15 14:53, Martin Vojtek wrote:
>>
>> On Wed, May 13, 2015 at 1:34 PM, Eugen Cepoi <cepoi.eugen_at_gmail.com>
>> wrote:
>>
>>>
>>>
>>> 2015-05-13 8:40 GMT+02:00 Martin Vojtek <voytoo_at_gmail.com>:
>>>
>>>> Hi Experts,
>>>>
>>>> we should get some conclusion to this discussion.
>>>>
>>>> I have the following notes:
>>>>
>>>> 1. Nillable, JsonbProperty, JsonbTransient
>>>>
>>>> I don't see any problem adding nillable attribute to JsonbProperty
>>>> annotation. The situation with JsonbTransient is different, because
>>>> transient field is effectively not a JsonbProperty. I think JsonbTransient
>>>> should stay as is.
>>>>
>>>>
>>> Adding target Property to JsonbProperty makes sense.
>>>>
>>>> 2. Property Naming
>>>>
>>>
>>>> I see concept of Mappers equivalent to JsonbAdapter (analogy to
>>>> XmlAdapter). It means that JsonbAdapter should see the context to be able
>>>> to do complex mapping between type and JSON fragment.
>>>>
>>>
>>> What is the context to you? From memory there is no context in
>>> XmlAdapter. Also I think it could make sense to allow users to register
>>> Adapters directly to JsonB (using the builder or config) vs only via
>>> annotations.
>>>
>>
>> Right now I don't know exactly what information JsonbAdapter needs, but
>> the idea behind context is that JsonbAdapter should be able to override as
>> much as possible behavior of default mapping. One option is to give access
>> to JsonbConfig. It allows to change behavior when some property is set. The
>> other option is to have access to information what has already been
>> processed. For example if there is dependency between objects in JSON.
>> Another interesting option is possibility to skip some part of json, or to
>> change some behavior during processing of the JSON. But maybe there should
>> be no context and to keep the concept of JsonbAdapter simple in version 1.0.
>>
>>
>>>
>>>> Composition of Mappings looks like an advanced feature and maybe it
>>>> should be postponed to future spec version.
>>>>
>>>
>>> By composition of mappings do you think of allowing users to provide
>>> an Adapter/Mapper for some type that uses another registered
>>> adapters/mappers?
>>>
>>> For example I provide an PersonsAdapter<List<Person>> but inside I
>>> don't want to deal with the mapping of each entry and want to delegate to
>>> the default adapter of person.
>>>
>>>
>>>> WDYT?
>>>>
>>> IMO we should keep it simple, unless there's obvious reason to not
>> do so. I'd like to see clearer need for the context and more complex
>> usecases here.
>>
>> 3. Property Order
>>>>
>>>> Case insensitive mapping is special, because it is not 1:1 mapping. To
>>>> make PropertyOrderStrategy simple, it should support only 1:1 mapping. In
>>>> that case PropertyOrderPolicy should not implement PropertyOrderStrategy. I
>>>> am not sure if there is a need to introduce Accessor class. In this case
>>>> simple String -> String mapping (accessor name -> JSON key) should work.
>>>> Implementations could provide some internal mechanism to call this function
>>>> only once per property.
>>>>
>>>
>>> I guess you are talking about naming and mean PropertyNamingStrategy
>>> and PropertyNamingPolicy.
>>>
>>> You are right, I was thining about different things at once and at
>> the end I have messed it into one point ...
>>
>>
>>> Even if I like the names "strategy" and "policy" (who doesn't, this
>>> sounds pretty cool :)) I find them confusing when used together.
>>> I think both mean the same thing :)
>>>
>>>
>> I'm not that good in inventing names :) If you have better names, it is
>> still possible to change that.
>>
>>
>>> To me there are two things with some common parts:
>>> - Resolve names (using Java Bean convetion/Annotations/Other)
>>> - Translate the string to another string
>>> (lower/upper/dashed/underscore/blabla)
>>>
>>>
>> +1
>>
>>
>>> The first one could deal directly with "properties" where the second
>>> one takes the resolved name and translates it.
>>> So here we have the choice of:
>>> - Have two separate concepts. One interface PropertyNameResolver
>>> that deals with properties or some abstraction. And a
>>> PropertyNamingStrategy/Policy interface that takes a string and returns
>>> another string (and provides the default impls as static attributes of the
>>> interface).
>>> - Or we consider that the name resolution is relatively fixed and
>>> handled inside the spec impl, thus we expose only the naming
>>> strategy/policy. Though I think this too is probably even more fixed...
>>> - Or we expose only the name resolution API
>>>
>>> No matter the choice, I'd be for providing default impls for all those
>>> inside the spec. There aren't many ways to implement a name resolution from
>>> java bean or annotations, neither to translate camel case to underscore...
>>>
>>
>> Agree with default impls + two separate concepts.
>>
>> My first comment applies here, too. If we're thinking the most of the
>> need is going to be covered by the spec implementation, then is there
>> really a need for the flexibility? What are the usecases which this would
>> be covering?
>>
>> MartiNG
>>
>>
>