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.
> - 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.
>
> >> 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.
>
> >>
> >> 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.
>
> >>
> >> 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
> >>>>
> >>>
> >>
> >
>