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();
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?
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
>
>