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.
We need methods with explicit java.lang.reflect.Type to provide way how to
get information about generic types at runtime.
>
> *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());
}
}
}
> *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).
>
> 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
>>>
>>>
>>
>