Le 1 juin 2015 02:56, "Eugen Cepoi" <cepoi.eugen_at_gmail.com> a écrit :
>
>
>
> 2015-06-01 2:55 GMT+02:00 Romain MB <rmannibucau_at_tomitribe.com>:
>>
>> 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)
>
>
> I was hesitant about this one too, but we already have JsonbProperty. It
could be used on the ctr parameters.
>
Hmm then metadata (nillable) should be there to avoid potential conflicts.
We could of course validate it but not needing it would make the API better.
>>
>> - 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
>
>
> Yes that's what I meant, allow to use the long representation.
>
>>
>>
>> >
>> > 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).
>
>
> But what value is there in implementing those inside the implementations
of the spec?
> Assuming we would all end up with very similar code.
>
Who knows? Plus better to not put logic in the API to let it be switchable.
>>
>>
>> >> 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).
>
>
> In theory yes in practice I find that separating those is not necessary
(but yes it gives more flexibility). People can throw an
UnsupportedOperationException from the method they don't want to implement
and voila.
>
Yes and no. Using java 8 defaults can help surely.
Wdyt?
>>
>>
>> >>
>> >> 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.
>
>
> This would be one option. But it is a bit like the TypeToken story, we
end up with clones all around...
> The advantage of a property class I see is to merge the field/get/set
into a single property and then use this property all around.
> The other solution could be to have separate methods using field/method.
>
Not real clones here, we are quite more specific and having an abstraction
will avoid to impl twice the same logic.
>>
>>
>> >>
>> >> 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
>> >>>>
>> >>>
>> >>
>> >
>
>