(catching up on the discussions after vacation)
I agree with Romain here,
MartiNG
On 10.07.15 19:35, Romain MB wrote:
> Hi
>
> about global formatters:
>
> I think as well it is a must have, ie being able to register custom
> adapter on the overall config instance. However I wouldnt make it
> specific to date/number. Since we can register an adapter for a type
> globally I think we are good and we avoid to make the API too specific
> and full of entry points.
>
> Romain
>
> 2015-07-10 10:16 GMT-07:00 Martin Vojtek <voytoo_at_gmail.com>:
>> Hi Oleg,
>>
>> comments below.
>>
>> Thank you,
>> Martin
>>
>> On Sun, Jul 5, 2015 at 8:23 PM, Oleg Tsal-Tsalko <oleg.tsalko_at_gmail.com>
>> wrote:
>>> Hi Martin,
>>>
>>> Thnx for your reply and that you incorporated our feedback in spec and
>>> code examples.
>>> Couple more comments/questions from my side including main Olena's
>>> proposal as I haven't seen any replies on her email:
>>>
>>> [by @Olena] It really makes sense to let “Custom date format”/“Custom
>>> number format” to be specified globally via JsonbConfig. WDYT?
>>
>> From my point of view it makes sense. What do others think about it?
>>
>>> Agree with all your comments in your reply and changes you've done except:
>>>
>>> Why have we decided not to leave FIELDS DEFINITION ORDER DRIVEN
>>> PropertyOrderStrategy? Is it difficult to achieve from impl point of view? I
>>> think it's default behavior expected by users IMHO.
>> Because it is not guaranteed across different vms. It was renamed to random.
>> Random could be implemented as the reflection order ...
>>
>>> Reg last example in DefaultMappingGenerics#toJson_generics() =>
>>> jsonb.toJson(expected) will yield same result as
>>> jsonb.toJson(expected,DefaultMappingGenerics.class.getField("listOfOptionalStringField").getGenericType()),
>>> isn't it?
>>
>> I don't think so. It should illustrate that if we don't know the type ... in
>> this case we are quaranteed to know it is List of Objects, and the
>> implementation is not able to infer type from the first item (first item is
>> null), so it will inevitably be treated as List of Objects. So the result
>> will be [null, {},{}].
>>
>>> SPEC REVIEW:
>>>
>>> [POLISHING] "The default behavior can be customized annotating a given
>>> field (or JavaBean property)" => Probably could be stated better to cover
>>> type level annotations
>> Sure, will improve that.
>>
>>> [JSB-4.3.1-2] Why have we decided to use diff annotations (@JsonbNillable
>>> and @JsonbProperty) to customize NULL handling on type level and on property
>>> level? It will be more clear to use @JsonbNillable annotation everywhere.
>>> (And even if we do decide to use @JsonbProperty to specify nillable fields,
>>> lets update javadoc for @JsonbProperty to reflect NULL handling purpose)
>>
>> There has been long discussion in EG mailing list about this. At the
>> beginning the nill behavior was associated only with @JsonbNillable. Sure,
>> will improve the javadoc. In general, feel free to attach patches to mailing
>> list.
>>> [POLISHING] [JSB-4.5-1] Class can contain at most one mapped property or
>>> field that is annotated with javax.json.bind.annotation.JsonbValue,
>>> otherwise JsonbException MUST be thrown.
>>
>> Ok, will change that.
>>
>>> [4.5] I would explicitly specify restriction on method signature
>>> @JsonbValue annotation can be applied on otherwise JsonbException MUST be
>>> thrown (e.g. public method without parameters)
>>
>> Makes sense.
>>> [TYPO] [4.6] "JsonbCreator annotation can be used to annotation custom
>>> constructor or static void factory method" => JsonbCreator annotation can be
>>> used to annotate custom constructor or static factory method. It should be
>>> at most one @JsonbCreator annotation otherwise JsonbException MUST be
>>> thrown.
>>
>> Makes sense.
>>> [4.6] I would add a NOTE that factory method annotated with @JsonbCreator
>>> should return instance of particular class this annotation is used for,
>>> otherwise JsonbException MUST be thrown. Also I would add a NOTE on how JSON
>>> fields will be mapped on parameters of constructor/factory method annotated
>>> with @JsonbCreator?
>>
>> Makes sense.
>>> [TYPO] [JSB-4.8-2] @JsonbJavaTypeAdapter has been renamed
>>
>> Will fix that.
>>
>>> Also is there any discussion going on in private reg JsonbAdapter API,
>>> cause I feel like it's hot topic?
>>>
>> No, there is no discussion in private going on. There is public EG and user
>> mailing list.
>>
>>
>>> Thank you,
>>> Oleg
>>>
>>> 2015-06-21 13:41 GMT+03:00 Olena Syrota <sirotae_at_gmail.com>:
>>>> Hi guys,
>>>>
>>>> More comments from JUG UA.
>>>>
>>>> 1. What do you think, is it worth to specify default behavior and
>>>> customization for unmarshalling nulls to primitives value? Default behavior
>>>> will be to ignore nulls as it is agreed for null Java fields in 3.14.1.
>>>> Customization may be fail on null for primitive value.
>>>>
>>>> 2. [3.5.1. Marshalling] 1. Proposition to setup default format for
>>>> Date|Calendar|GregorianCalendar marshalling instead of run-time desision on
>>>> ISO_DATE|ISO_DATE_TIME format regarding of time presence in value. 2.
>>>> Proposition to consider customization of marshalling date format on
>>>> Builder-level (additionally to current cistomization for field)
>>>>
>>>> 3. [3.5.1. Unmarshalling] 1. Proposition to setup default format for
>>>> Date|Calendar|GregorianCalendar unmarshaling instread of chosing
>>>> corresponding format at run-time. 2. Proposition to consider customization
>>>> on Builder-level (additionally to current customization for field).
>>>>
>>>> Thank you
>>>> Olena
>>>>
>>>> 2015-06-20 18:49 GMT+03:00 Martin Vojtek <voytoo_at_gmail.com>:
>>>>> Hi Oleg,
>>>>>
>>>>> it was quite a big list :)
>>>>>
>>>>> comments below.
>>>>>
>>>>> Thank you,
>>>>> MartinV
>>>>>
>>>>> On Tue, Jun 2, 2015 at 11:30 PM, Oleg Tsal-Tsalko
>>>>> <oleg.tsalko_at_gmail.com> wrote:
>>>>>> Hi guys,
>>>>>>
>>>>>> It's been for a while since our last email, so we have quite a few
>>>>>> outstanding comments/questions reg JSON-B spec.
>>>>>>
>>>>>> I decided to provide majority of them in one email so it might be
>>>>>> easier for you to comment on them since all of you pretty much in context of
>>>>>> recent changes in spec including 'Customizing Mapping'. Also I grouped our
>>>>>> comments/questions by topics:
>>>>>>
>>>>>> Forgotten stuff:
>>>>>>
>>>>>> 1) [TYPO] Runtime "Use an explicit implementation of JsonbProvider"
>>>>>> example should be changed to explicitly instantiate provider instead of
>>>>>> builder itself I believe.
>>>>>>
>>>>>> DefaultMappingGenerics:
>>>>>>
>>>>>> 2) There are still no examples to show use cases for
>>>>>> Jsonb#toJson(Object, Type) method to deal with particular special cases
>>>>>> where otherwise it won't work
>>>>>
>>>>> I think this is the last example in toJson method -
>>>>> List<java.util.Optional<String>>.
>>>>>
>>>>>> 3) [TYPO]
>>>>>> assertEquals("{\"boundedSet\":[3],\"superList\":[{\"radius\":2.5}]}",
>>>>>> jsonb.toJson(boundedGenericClass));
>>>>>>
>>>>>> missing area property of Shape super class I guess
>>>>>
>>>>> Fixed.
>>>>>
>>>>>> 4) In DefaultMappingGenerics examples we shouldn't use explicit asserts
>>>>>> for LinkedHashMap default implementation of Map as we agreed to relax this
>>>>>> constraint in spec as I remember
>>>>>
>>>>> Fixed
>>>>>
>>>>>> 5) In DefaultMappingGenerics examples for incompatible types it will be
>>>>>> ClassCastException later on instead of JsonbException
>>>>>>
>>>>>> DefaultMappingDates:
>>>>>
>>>>> Fixed
>>>>>
>>>>>> 6) No negative test case examples to throw JsonbException if JSON
>>>>>> date/time is not in supported ISO format
>>>>>
>>>>> Fixed
>>>>>
>>>>>> SPEC:
>>>>>>
>>>>>> 7) Formally in 3.4.3 chapter it's not mentioned about ser/deser of
>>>>>> Optional's as JsonValues, despite behavior is the same as for object
>>>>>> instance properties
>>>>>
>>>>> Fixed
>>>>>
>>>>>> 8) [TYPO] in JSB-3.6-1 java.math.Integer—Long—BigDecimal
>>>>>
>>>>> Fixed
>>>>>
>>>>>> 9) In 'Scope and Field access strategy' 3.7.1 section nothing said
>>>>>> about getters/setters without actual field as we have examples for it
>>>>>
>>>>> Fixed
>>>>>
>>>>>> 10) In 'Null Java field' 3.14.1 section it is not explicitly stated
>>>>>> about unmarshalling of null values from JSON and there is no such case in
>>>>>> examples by the way
>>>>>
>>>>> Fixed
>>>>>
>>>>>> 11) [TYPO] javax.json.bin. in several places in spec
>>>>>
>>>>> Fixed
>>>>>
>>>>>> 12) @JsonbPropertyOrder annotation usage is not mentioned in spec
>>>>>
>>>>> Fixed
>>>>>
>>>>>> 13) There is no word about @JsonbTransient annotation in spec
>>>>>
>>>>> Fixed
>>>>>
>>>>>> CustomizingMapping:
>>>>>>
>>>>>> 14) Why @Retention is missed on @JsonbTransient annotation?
>>>>>
>>>>> Fixed
>>>>>
>>>>>> 15) It is stated that @JsonbTransient is mutually exclusive with other
>>>>>> annotations. What gonna happen if user specify contradictive annotation on
>>>>>> field lets say?
>>>>>
>>>>> Fixed
>>>>>
>>>>>> 16) I suspect in CustomMapping#toJson_nillable() examples it should be
>>>>>> 'null' instead of 'nill' values?
>>>>>
>>>>> Fixed
>>>>>
>>>>>> 17) I think JsonConfig#withSkippedNullValues should really be
>>>>>> #withNullValues and there is no mentioning of default value in Javadoc which
>>>>>> should be 'true' in this case because we are skipping null values by
>>>>>> default?
>>>>>
>>>>> Makes sense. Fixed.
>>>>>
>>>>>> 18) There is no example of @JsonbProperty usage on Parameter level and
>>>>>> I'm not sure it's required IMO. Only one valid usage I can think of is
>>>>>> together with @JsonbCreator methods/constructors which hasn't been agreed
>>>>>> yet.
>>>>>
>>>>> Yes, it is meant of @JsonbCreator methods/constructors. We try to change
>>>>> the process from discussion -> implementation to implementation proposal ->
>>>>> discussion to help discussion about the topics.
>>>>>
>>>>>> 19) Do we really feel strong need in @JsonbCreator functionality? If
>>>>>> yes, it should probably be integrated in main default mapping spec chapters
>>>>>> rather then simply in customization mapping chapter, cause it influence
>>>>>> constraints for POJOs being deserialized. WDYT?
>>>>>
>>>>> Default mapping is about no JSON Binding annotations present in the java
>>>>> model. I think JsonbCreator could solve some problems in certain situations.
>>>>> Personally can live without this functionality in version 1.0.
>>>>>
>>>>>> 20) There are no examples of @JsonbValue usage which might be ambiguous
>>>>>> IMO. Will be good to hear some good use cases justifying it? Also same
>>>>>> effect could be achieved using TypeAdapters.
>>>>>
>>>>> If there are some ambiguities, these should be fixed in SPEC. The
>>>>> examples are just illustrative to support discussion about the topics. Not
>>>>> providing examples is a good way how to force people to read SPEC IMO :)
>>>>> Please could you specify what ambiguities do you see?
>>>>>
>>>>>> 21) [TYPO] public identifiers from
>>>>>> PropertyNamingStartegy/PropertyVisibilityStrategy interfaces should be
>>>>>> removed to be consistent
>>>>>
>>>>> Fixed
>>>>>
>>>>>> 22) There are no examples of JsonbVisibilityStrategy usage and again I
>>>>>> don't feel it's really needed. I would rather relax constraints for public
>>>>>> class members in first place. However even if we do want to have it,
>>>>>> probably it's better to define some meaningfull default strategies here if
>>>>>> you can think of any?
>>>>>
>>>>> There was long discussion about these constraints and I am happy not to
>>>>> open it for some time :)
>>>>>
>>>>>> 23) BinaryDataStrategy could be an enum?
>>>>>
>>>>> I have switched from enums to Strings. The reason is that enum is not an
>>>>> interface and extending enum for another option could break some (bad
>>>>> written) existing code and it is quite easy to live with strings. It will be
>>>>> easier to add another option in the future.
>>>>>
>>>>>> 24) In PropertyOrderStrategy I would rename REFLECTION constant to
>>>>>> FIELDS and update corresponding javadocs to not rely on some particular
>>>>>> reflection API method as stated in javadocs currently
>>>>>
>>>>> There was discussion in EG and I plan to rename REFLECTION to ANY.
>>>>>
>>>>>> 25) Don't we want to introduce smh like JSONB_STRICT_DESERIALIZATION
>>>>>> config param to handle missing JSON properties differently than default skip
>>>>>> strategy and corresponding @JsonbStrictDeserialization annotation on TYPE
>>>>>> level?
>>>>>
>>>>> According to RFC 7493 https://tools.ietf.org/html/rfc7493 tt is
>>>>> recommended to skip missing properties. Strict deserialization could be
>>>>> addressed by some preprocessor and there was no strong agreement in EG to
>>>>> support strict deserialization. Maybe it could be add in some later JSON
>>>>> Binding version.
>>>>>
>>>>>> 26) JSONB_EVENT_HANDLER could be removed in JsonbConfig class
>>>>>
>>>>> Fixed
>>>>>
>>>>>>
>>>>>> Thank you,
>>>>>> Oleg
>>>>>
--
Martin Grebac, SW Engineering Manager
Oracle Czech, Prague