2015-03-16 22:16 GMT+01:00 Oleg Tsal-Tsalko <oleg.tsalko_at_gmail.com>:
> Hi Martin,
>
> Just wanted to remind you couple of changes you kind of agreed with but
> haven't committed yet (here I'm referring master branch):
>
> 1)
>
>> Is it ever possible that JsonbConfig#getProperty()/setProperty()
>> methods will throw JsonbConfigException is they are simply calling
>> Map#get()/put() methods?Thus JsonbConfigException looks redundant
>> for now at all and methods signatures + javadocs should be fixed?
>>
>> Is it ever possible that JsonbConfig#getProperty()/setProperty()
>> methods will throw JsonbConfigException is they are simply calling
>> Map#get()/put() methods?Thus JsonbConfigException looks redundant
>> for now at all and methods signatures + javadocs should be fixed?
>> Thanks, somehow this got dropped - makes sense.
>
>
> 2)
>
>> However if we really want to leave JsonbConfig object mutable in runtime
>>> to quickly change some configuration then we probably need to explicitly
>>> highlight in specification that JsonbConfig is not thread safe and could be
>>> modified in runtime.
>>
>> Agree.
>
>
> 3) In Runtime example we still have:
>
> // Use an explicit implementation of JsonbProvider
> Jsonb jsonb = new CustomJsonbBuilder().build();
>
> where should be smith like below I believe:
>
> // Use an explicit implementation of JsonbProvider
> Jsonb jsonb = JsonbBuilder.newBuilder(new CustomJsonbProvider()).build();
>
>
I don't have the code under the eyes, but from memory, the provider would
just create an instance of the builder. In that case I think it is just
better to directly create the builder. The indirection is unecessary.
> And couple of more questions:
>
> 4) Why decision has been made to throw NPEs upon retrieval of null values
> across API instead of IllegalArgumentExceptions? I don't remember it has
> been discussed in public?
> 5) Why default JSON-B provider is mentioned as org.eclipse.persistence.json.bind.JsonBindingProvider
> or it's just a stub for now?
>
I guess it is the default impl. Though this code should never be reached as
if the default impl. org.eclipse.persistence.json.bind.JsonBindingProvider
is in the classpath it should be returned by the code above. In that case
maybe this code can be removed.
>
> Thank you,
> Oleg
>
> 2015-03-02 12:13 GMT+00:00 Martin Grebac <martin.grebac_at_oracle.com>:
>
>> On 28.02.15 18:05, Oleg Tsal-Tsalko wrote:
>>
>>>
>>> 1. Don't we want to add fallback option to
>>> JsonbProvider#provide(String className) method to load class
>>> directly using ClassLoader if no class found using
>>> ServiceLoader
>>> similar to default JsonbProvider#provide() method?
>>>
>>> Are you looking to solve a specific usecase? We already have a
>>> specific provider lookup using provider name (which should support
>>> also jdk9 module mode without changes in future):
>>> public static JsonbProvider provider(final String providerName) {
>>>
>>>
>>> I meant changing existing implementation of
>>>
>>> public static JsonbProvider provider(final String providerName)
>>>
>>> to class load provider class directly as fall back option if no such
>>> provider found using ServiceLoader
>>> as in
>>>
>>> public static JsonbProvider provider()
>>>
>>> I see, understand now. I consider throwing an exception here better,
>> because the user is explicitly requesting specific provider. So the
>> implementation should not choose something different and should let the
>> user know the expected provider could not be found/instantiated.
>>
>>
>>> 1. It was already discussed couple of times however why not
>>> support
>>> JVM params to specify JSON-B Provider to be used in runtime
>>> similar to specifying TransformerFactory impl in JAXP for
>>> example?
>>> It might be usefull in some cases and will preserve
>>> consistency
>>> with other specs. Imagine you have couple of provider libs in
>>> classpath and would like in runtime to specify which one
>>> to use
>>> without recompiling/rebuilding project. How can we cover
>>> this use
>>> case with specific JsonbConfig as some of you suggested?
>>>
>>> Are you missing this option in JSON-P as well? Looking at
>>> consistency, we should be looking at multiple places.
>>>
>>>
>>> Didn't use JSON-P in practice so far. Was referring to JAXP
>>> specification as an example...
>>>
>> I'm not opposing here, I'm trying to figure out whether there really is
>> the need, or the above methods are enough. JSON-P e.g. does not define any
>> such overriding mechanism, does not even define the possibility to request
>> specific provider. JSON-P is with us for some time and so far I haven't
>> seen any requests for overriding.
>>
>> The problem is this implementation is missing a method for
>>> generic, provider specific properties, which will make the
>>> immutability and thread safety hard to achieve. We need an
>>> extension mechanism to support provider specific features.
>>> Also, my motivation was to avoid creating multiple instances of
>>> JsonbConfig in every simple case even if e.g. one just wants to
>>> quickly set a pretty printing on existing configuration to e.g.
>>> log some messages.
>>>
>>>
>>> We can just add one more method to JsonbConfigBuilder then:
>>>
>>> JsonbConfigBuilder withSpecificProperty(String propertyName,
>>> Object propertyValue)
>>>
>>> Right. With this you have JCB itself immutable, but not the
>> configuration as such. Object (say some collection) is still mutable.
>>
>> However if we really want to leave JsonbConfig object mutable in runtime
>>> to quickly change some configuration then we probably need to explicitly
>>> highlight in specification that JsonbConfig is not thread safe and could be
>>> modified in runtime.
>>>
>> Agree.
>>
>> I saw you've added:
>>>
>>> Map<String, Object> getAsMap()
>>>
>>> probably with intention to use this method inside JsonbBuilder to pass
>>> unmodifiable Map of props into Jsonb object contractor,
>>> however I would rather use JsonbConfig container itself to pass into
>>> Jsonb object during construction. WDYT?
>>>
>> Right, though it has the same problem as described above. Without deep
>> clone one still has to face mutability. This method allows a clone to be
>> done within the method calls before Jsonb objects are constructed.
>>
>> 1. As has been already proposed lets provide API examples as
>>> unit
>>> tests which should pass using mocks or dummy implementation
>>> instead of main method classes that are failing.
>>>
>>> At this early stage, compiling the examples should be enough, as
>>> mentioned they are more to guide the discussion.
>>>
>>>
>>> Having unit test style examples carefully structured will make examples
>>> more readable and clear with later ease to make them actually runnable.
>>> It's not big effort to do anyway...
>>>
>> In general I understand the test obsession - I'd myself be obsessed as
>> well :) - this being a regular implementation project. Though here actually
>> *not* having the examples in the form of tests makes sense, because they
>> are not expected to be so, and besides writing them in such a way is an
>> unnecessary complication. There will be hundreds times more tests within RI
>> implementation or TCK required than just these few within examples, so I
>> don't want anyone to think these are normatives or anything.
>>
>> Also one item been missed:
>>>
>>> Is it ever possible that JsonbConfig#getProperty()/setProperty()
>>> methods will throw JsonbConfigException is they are simply calling
>>> Map#get()/put() methods?Thus JsonbConfigException looks redundant
>>> for now at all and methods signatures + javadocs should be fixed?
>>>
>>> Thanks, somehow this got dropped - makes sense.
>>
>> MartiNG
>>
>>
>