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:
>
> 1. [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:
>
> 1. 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 ...
>
> 1. 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:
>
> 1. [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.
>
> 1. [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.
>
> 1. [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.
>
> 1. [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.
>
> 1. [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.
>
> 1. [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.
>
> 1. [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
>>>>
>>>
>>>
>>
>