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?
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.
2. 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?
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
2. [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)
3. [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.
4. [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)
5. [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.
6. [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?
7. [TYPO] [JSB-4.8-2] @JsonbJavaTypeAdapter has been renamed
Also is there any discussion going on in private reg JsonbAdapter API,
cause I feel like it's hot topic?
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
>>>
>>
>>
>