Replies below.
Martin
On Sun, May 31, 2015 at 9:55 PM, Eugen Cepoi <cepoi.eugen_at_gmail.com> wrote:
> Some other comments.
>
> *JsonbCreator*
> The javadoc could be improved to make it clear what the contract is.
> Only one constructor or static factory method can be annotated with it,
> the marked ctr/method will be used to create instances of that object. A
> note about ctr/method with args would be nice too (are impls supposed to
> work with it or not?).
>
>
Sure, I will improve it. Impls are supposed to work with ctr/method with
args. Args could be annotated with e.g. JsonbProperty.
> *JsonbDateFormat*
> Maybe add an option to ser the property as a timestamp in millis?
> If you don't, then there shouldn't be a default value for "value".
>
>
+1 for timestamp in millis
> *JsonbJavaTypeAdapter*
> Why not only JsonbTypeAdapter.
>
>
+1
> *JsonbValue*
> Will this single value be "unwrapped" if it is a class that would
> serialized as a json object?
> It might be worth adding an example in the javadoc.
>
>
Yes, I will add an example to javadoc. One for String result of such single
value and one for mappable class of such single value. If the result is
String, it will be unwrapped.
If the result is some mappable class, it will be serialized as any other
mappable class (it will be in braces). So wrapped. It will be clear from
the examples.
> *JsonbConfig*
> It is maybe not necessary to prefix all the config constants with JSONB.
> Though it has the advantage to avoid namespace conflicts with static
> imports.
> WithAdapters overrides the previous result, this should be documented or
> be merged with previous value (as a user it wouldn't be obivous to me, I'd
> tend to think that it merges the adapters).
>
>
I don't have strong opinion on the JSONB prefix. I think it should be
merged with the previous value. Will add some javadoc.
> And again providing some default impls with the spec would be nice
> (adapters, configs etc).
>
> Eugen
>
>
> 2015-05-31 16:07 GMT+02:00 Eugen Cepoi <cepoi.eugen_at_gmail.com>:
>
>> 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
>>>>
>>>>
>>>
>>
>