Hi
mainly agree with Eugen but added few comments inline.
Few more comments:
- JsonbProperty has nillable so it should get transient IMO (or
nillable should be removed)
- JsonbCreator misses parameter (like ConstructorProperties)
- Can't date and number format annotations be merged? seems it is not
ambiguous for me and simplifies the API.
Romain Manni-Bucau
@rmannibucau
http://www.tomitribe.com
http://rmannibucau.wordpress.com
https://github.com/rmannibucau
2015-05-31 21:55 GMT+02:00 Eugen Cepoi <cepoi.eugen_at_gmail.com>:
> 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?).
>
> 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".
not sure for timestamp option, using a plain long would make it quite
easy as well
>
> JsonbJavaTypeAdapter
> Why not only JsonbTypeAdapter.
>
> 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.
>
> 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.
since it is part of the API directly I think it is not a big deal.
Side note: why JSONB_STRICT_IJSON, doesn't seem to match the convention?
> 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).
>
JsonbConfig#withAdapters should merge adapters (you can still use
setProperty if you want to erase previous ones or we can even add a
real setter).
Side note: having setters and not "builder methods" (withXX) would
make it easy to integrate with any common IoC config IMO.
> 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
>>
Using strings have the advantage to make it config friendly so I guess
we would need both.
About having defaults or not I think it shouldn't be in the spec since
it is up to the implementations (even if some code canbe trivial).
>> 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 using Java8 naming: apply?
>> 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);
>> }
>>
+1, even wonder if splitting both sides wouldn't makes sense. I'm
thinking to a rest service (server only) where you don't care of read
side. So would be @JsonbJavaTypeAdapter(reader = MyReader.class,
writer = MyWriter.class) (or from/to or any other naming convention).
>>
>> PropertyVisibilityStrategy
>> Member does not provide access to annotations, handling things like
>> @JsonTransient requires access to those.
>>
javax.json.bind.config.PropertyVisibilityStrategy#isVisible: agree we
should have annotations but type as well. Is it expected to be casted?
Can't we add a bit like in CDI an abstraction for that? Would be
Property { type, annotations, declaringClass, name, member }. Wonder
if synthetic makes sense or if we skip them before calling the method.
>>
>> 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.
>>
+1
If we go with a "Property" API we should replace the Class here by
something close to be able to change metadata easily just wrapping a
strategy.
>>
>> 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
>>>>
>>>
>>
>