Hi Martin,
Thnx for your comments. Some of my additional comments below.
2015-02-27 13:18 GMT+02:00 Martin Grebac <martin.grebac_at_oracle.com>:
>
> Hi Oleg (and others),
> thank you for interest thorough review and feedback. Please see my
> comments below.
>
> On 26.02.15 11:59, Oleg Tsal-Tsalko wrote:
>
>> Hi guys,
>>
>> We (Kiev UA JUG) come up with list of comments/questions/suggestions reg
>> Runtime API proposed so far:
>>
>> 1. Having pluggable JsonbProviders, do we really want to leave an
>> option to also setup diff JsonProviders via
>> JsonBuilder#withProvider(...) method? Specific JsonbProvider could
>> decide what JsonProvider to use underneath by itself? Such a mix
>> brings a lot of confusion and introduces dependency to JSON-P api.
>> We would vote to remove JsonBuilder#withProvider(...) method. WDYT?
>>
>> As for dependency, JSON Binding will have JSON-P dependency anyway
> (direct or indirect), given we plan to support JSON-P artifacts within
> default mapping (see the default mapping examples from MartinV). This is
> similar to JAXP/JAXB alignment.
> The main reason I added this method is that JSON-P only allows
> ServiceLoader or DEFAULT_PROVIDER for selecting the provider, which
> inherently causes problems which you even describe below, and makes it hard
> or impossible to configure a specific JSON-P provider for use within JSON
> Binding ifthat is what the application requires.
> Question is, whether to impose this requirement on all JSON Binding
> implementations - by this requirement, the spec requires all JSON Binding
> implementations to be JSON-P implementation agnostic, which could limit
> some implementations, so I'd agree the requirement may be too strong. I'd
> like to hear some more voices in EG though.
>
> 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()
> 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...
>
> 1. From my point of view JsonConfig objects should be thread-safe and
>> immutable to be safelly passed around. From the other hand in
>> order to contruct JsonbConfig instances we can use implicit inner
>> class builder with comprehensive builder methods. See JsonbConfig
>> class in attach or on GitHub -
>> https://github.com/olegts/jsonb-spec/blob/simplified_
>> api/api/src/main/java/javax/json/bind/JsonbConfig.java
>> 2. JsonbConfig properties/methods naming could be improved to be more
>> readable and follow builder style. See JsonbConfig class in
>> attach. From client code it will looks like:
>> * JsonbConfig config =
>> jsonbConfig().withInputEncoding(UTF_16).
>> withPrettyOutput().build();
>>
>> I believe the full is: new JsonbConfig.JsonbConfigBuilder().
> withInputEncoding(StandardCharsets.UTF_16).withPrettyOutput().build();
>
> * instead
>> * JsonbConfig config = new
>> JsonbConfig().fromJsonEncoding(StandardCharsets.UTF_16.name
>> <http://standardcharsets.utf_16.name/>()).toJsonFormatting(true);
>>
>> 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)
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.
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?
>
> 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...
>
> 1. As has been already mentioned 'public' modifier could be omitted
>> in interfaces.
>>
>> This has been brought up number of times, I prefer to be explicit, for
> public health and safety I removed them :)
>
We are all healthy and happy now)) Thnx
>
> 1. Runtime "Custom providers with explicit provider impl" example
>> should be changed to explicitly instantiate provider instead of
>> builder itself I believe: Jsonb jsonb = new
>> CustomJsonbBuilder().build();in Runtime class line 111.
>> 2. TYPO: Json class line 222.
>>
>> Fixed.
> 1. JsonbBuilder methods could be more builder like with better names.
>> See JsonbBuilder class in attach or on GitHub -
>> https://github.com/olegts/jsonb-spec/blob/simplified_
>> api/api/src/main/java/javax/json/bind/JsonbBuilder.java
>>
>> From client code it will looks like:
>>
>> Jsonb jsonb = newJsonbWith(config);
>>
>> Jsonb jsonb =
>> jsonBuilderFrom("jug.kiev.jsonb.impl.
>> CustomJsonbProvider").build();
>>
>> Jsonb jsonb = newJsonb();
>>
>> instead
>>
>> Jsonb jsonb = JsonbBuilder.create(config);
>>
>> Jsonb jsonb =
>> JsonbBuilder.newBuilder("foo.bar.ProviderImpl").build();
>>
>> Jsonb jsonb = JsonbBuilder.create();
>>
>>
>> Hope some of our comments/suggestions will be useful...
>>
> You example is not fully complete, and to me it still is more verboseand
> not that much consistent with what other apis are providing, such as JAX-RS
> client or JSON-P e.g. so I'm still inclined to the latter.
>
> MartiNG
>
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?
Thank you,
Oleg