2015-06-06 8:35 GMT+02:00 Martin Vojtek <voytoo_at_gmail.com>:
> Hi Eugen,
>
> thanks for your comments. Replies below.
>
> Martin
>
> On Sun, May 31, 2015 at 4:07 PM, Eugen Cepoi <cepoi.eugen_at_gmail.com>
> wrote:
>
>> 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.
>>
>>
> I am not sure, if providing default implementations is of much value. Not
> all options are trivial to implement, e.g. CASE_INSENSITIVE.
>
>
>>
>> *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);
>> }
>>
>>
> I see this can be simplified. Maybe we could remove method
> handlesNullValue and split remaining 4 methods into two separate interfaces.
>
Having stuff like handlesNullValue why not, until it has a default impl I
am fine.
I think it should not be split in separate interfaces.
> We need methods with explicit java.lang.reflect.Type to provide way how to
> get information about generic types at runtime.
>
I don't understand. We have generic info in the Adapter signature interface
Adapter<BoundType, ValueType>, it can be extracted at runtime. No need to
add methods that return the type handled by this adapter.
>
>
>>
>> *PropertyVisibilityStrategy*
>> Member does not provide access to annotations, handling things like
>> @JsonTransient requires access to those.
>>
>>
> Member provides access to annotations.
>
> //get annotations
> for (Member member : members) {
> if
> (member.getDeclaringClass().isAnnotationPresent(MyAnnotation.class)) {
> MyAnnotation myAnnotation =
> member.getDeclaringClass().getAnnotation(MyAnnotation.class);
> System.out.println("type annotation with value:
> "+myAnnotation.value());
> }
>
> if (member instanceof Field) {
> Field memberField = (Field)member;
> if (memberField.isAnnotationPresent(MyAnnotation.class)) {
> MyAnnotation myAnnotation =
> memberField.getAnnotation(MyAnnotation.class);
> System.out.println("field annotation with value:
> "+myAnnotation.value());
> }
> } else if (member instanceof Method) {
> Method memberMethod = (Method)member;
> if (memberMethod.isAnnotationPresent(MyAnnotation.class)) {
> MyAnnotation myAnnotation =
> memberMethod.getAnnotation(MyAnnotation.class);
> System.out.println("method annotation with value:
> "+myAnnotation.value());
> }
> }
>
> }
>
>
Yes sure with the cast we can. But I think it would be nicer from an usage
point of view to just define one method for fields and another for methods.
>
>> *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.
>>
>>
> Ok, I will replace REFLECTION with ANY.
> Yes, it is called with "renamed" name. I will state it in the spec and
> javadoc. I think that you can gen information about annotations from the
> Class. On the other hand, we could definitely remove the option of making
> ordering customizable (by removing getPropertyOrder method).
>
Sounds ok to me, if it is not 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
>>>>
>>>>
>>>
>>
>