More comments, these mails are quite long :)
On Mon, Jun 1, 2015 at 2:55 AM, Romain MB <rmannibucau_at_tomitribe.com> wrote:
> 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?
>
>
its a typo, will fix it.
> > 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).
>
>
+1
> Side note: having setters and not "builder methods" (withXX) would
> make it easy to integrate with any common IoC config IMO.
>
>
Yes, I can see your point. On the other hand, builder methods are
convenient for developers.
> > 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).
>
>
+1
> >> 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?
>
>
do you prefer applyFrom/applyTo?
> >> 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).
>
>
we can't get rid of methods with Type because of generics, but we can split
the interface to two different interfaces. After that it will be simple to
implement adapter for simple cases without generics.
+1 for from/to parameters for JsonbTypeAdapter. In that case adapter
methods should have default implementation throwing some Exception.
> >>
> >> 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.
>
>
It was expected to be casted, but we could split it into two methods for
fields/methods. I think we should skip synthetic fields.
> >>
> >> 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.
>
>
we could remove the method
> >>
> >> 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
> >>>>
> >>>
> >>
> >
>