2015-01-31 14:34 GMT+01:00 Przemyslaw Bielicki <pbielicki_at_gmail.com>:
> Hi,
>
> Thanks a lot for sharing this Martin - so far we have something to begin
> with, which is great.
>
> @Eugene, I'm supporting most of your comments. Here are my comments and
> questions.
>
> @Eugene: "If yes, it looks too restrictive and how will this integrate
> with Jersey (JsonRootElement... :( )?" - do you have better idea than
> JsonbContext?
>
What looks to me restrictive and unecessary is to explicitly tell what
classes can be ser/de as root class - this is not specfic to JsonbContext,
but also to Jsonb. I would just allow any class to be root.
Concerning JsonbContext more in general please see the code snippet in my
previous mail.
>
> *JsonbContext.Builder*
>
> Arrays should be avoided in public APIs - getClasses should return
> unmodifiable Collection/List<Class<?>>. On the other hand setClasses should
> stay as it is now.
>
>
+1 for it in general, but I would prefer to see this classes filtering
being dropped.
>
>
> *Jsonb*
> I like shortcut methods, they save a lot of code for end users. We should
> also try to build as fluent API as possible (not sure than binding API is a
> good candidate though)
>
>
Agreed on saving code, but Jsonb also encourages bad practices where
instances are not reused.
>
>
> *JsonbMarshaller*
> Me too, I don't really get the idea of callback.
>
>
>
> *Thread safety*
> It would be great to support immutability everywhere in JSONB API - then
> thread safety is for free. Disadvantage is that JsonbContext(s) should be
> cached somehow, to avoid expensive object creation and annotation scanning.
>
Agree. I am talking about around the code snippet. If people are supposed
to reuse Jsonb instances, then it can contain the cache - otherwise it
would (I guess) be inside the impls of Un/Marshaller. This is very natural
in the end, most libs do it this way. A main entry point that is tied to
some config, has the cache for the components related to this config and is
reused by the user.
> Do we want to delegate thread-safety question for the implementations? IMO
> we should do it as early as in API.
>
>
>
>
> *JSON-P*
> As you said Martin, JSON-P support would be a natural step, just after
> replacing File with Streams :)
>
> *Overall impression*
>
> I think File is really useless in the API and should be replaced with
> Input/OutputStream, as it is more generic and more used (e.g. in Servlet
> there is no File).
>
>
I would be in favor for both.
File is maybe not very useful, but can save a bit of code to the user, so
why not (what about URL) :)
> I agree with Eugene that we should limit the number of possibilities of
> creating API objects (but keep shortcuts as they are extremely useful if
> users do not need any customizations) - Builder seems flexible enough. With
> Builder we can automatically guarantee immutability.
> If user wants to have flexible marshalling / unmarshalling we could
> overload marshall/unmasrhall methods with settings Map. We would not loose
> immutability and we could keep flexibility.
>
>
>
Let's not forget about abilities to customize JSONB default behavior using
> system properties.
>
System properties - you might find enemies here :p
>
> *Project structure*
>
> It would be nice to have a parent pom.xml in the jsonb directory. We could
> build all the projects with "one click".
> In the examples I would focus on extensive unit testing (using main()
> method for testing is not really modern ;)
>
> It would be nice if the build fails on checkstyle violations - it's rather
> for you Martin to enforce quality.
> The same for code coverage - tests should be everywhere and target
> coverage should not be less than 80%.
>
> Have a great weekend (what's left of it)!
> Przemyslaw
>
> On Fri, Jan 30, 2015 at 10:34 PM, Eugen Cepoi <cepoi.eugen_at_gmail.com>
> wrote:
>
>> Martin, here are some (opinionated :p) comments...
>>
>> *Jsonb*
>>
>> createContext(final Class<?> ... classes) => Does that mean that the user
>> will have to explicitly specify what will be the root classes being
>> ser/de?? If yes, it looks too restrictive and how will this integrate with
>> Jersey (JsonRootElement... :( )?
>>
>> Missing unmarshal/marshal methods taking input/outputstream and bytes.
>>
>> I am not sure that having those shortcut methods is a good idea. Using
>> them wouldn't take advantage of any caching thus would yield poor
>> performances. At least it should be explained in the javadoc (but people
>> tend to not read it...)
>>
>> *JsonbContext*
>>
>> We can read at the beginning of the javadoc *"information necessary to
>> implement the JAXB binding framework operations:"*, I guess it is a typo?
>>
>> *JsonbMarshaller*
>>
>> I don't get the part about the callbacks. Are they supposed to be defined
>> on the object being ser. or by the impl. of the Marshaller?
>> What use cases do you have in mind? For deser. I would see validation or
>> instantiation, but here...
>>
>> *"Invoked by JsonbMarshaller after it has created an instance of this
>> object."* I guess it is an instance of the JsonbMarshaller?
>>
>> *JsonbUnmarshaller*
>>
>> I would remove all the untyped unmarshal methods that return Object. If
>> people want the deser. to be fully driven by the input, they can use
>> unmarshal(json, Object.class). I think in most cases they would deser to a
>> node structure (jsonp) or map/list.
>>
>> *"An unmarshal method never returns null."*, I would consider an empty
>> input as null rather than malformed. Some libs also allow root json values
>> different from object and array.
>>
>>
>> *Overall impression*
>>
>> - Is sticking to JAXB design a goal of this jsr? I would prefer not... some
>> parts look to me over complicated for no apparent reason.
>>
>> - Users that want to provide some custom ser/de for a specific type (the
>> usual custom Serializer) would they use a kind of Adapter like in JAXB? I
>> think it can be easier to use but less efficient than directly using a
>> streaming API in the custom code.
>>
>> - I have the impression that JsonbContext/Builder, Un/Marhsaller and
>> Jsonb overlap and could be redesigned a bit (see below).
>>
>> - I was expecting to see jsonp reader/writers instead of std java io
>> classes in Un/Marhsaller.
>>
>> - Do we want the Un/Marshaller to be mutable (I see those setProperty
>> methods)?
>>
>> - Do you have an example of what would be the recommended single way to
>> use this API?
>>
>> Jsonb instance could
>> - use the JsonbProvider as a factory to obtain instances of
>> marshaller/unmarshaller configured with some typed config object or a plain
>> map
>> - obtain a low level reader/writer instance from jsonp for current std
>> reader/writer
>> - do some other stuff where having an spi does not provide any win
>> - reuse the marshallers (or let it be the responsability of
>> JsonbProvider)
>> - then call marshaller.marshal(object, jsonpWriter)
>>
>> // or just new Jsonb() with default config
>> Jsonb jsonb = new Jsonb.Builder()
>> .setClasses(Book.class, Author.class, Language.class)
>> .setProperty(JsonbMarshaller.JSON_BIND_FORMATTED_OUTPUT, true)
>> .use(SomeJsonbProvider()) // optional - by default would load one using
>> ServiceLoader
>> .use(JsonpProvider()) // used to obtain jsonp read/write instances
>> .build();
>>
>> // also accepting jsonp reader/writer
>> jsonb.marshal(...);
>> jsonb.unmarshal(...);
>>
>> Unmarshall/Marshall could take only jsonp reader/writer and no std java
>> reader/writer. It is discutable, but if the goal is to provide databinding
>> over the already existing "std parsing API" then it should be the way to go
>> IMO.
>>
>> People are getting used with APIs that provide a single entry point + a
>> builder to configure it and the underlying components.
>> Providing multiple ways to use this API could be error prone (In the
>> javadoc and examples I see 3 ways to achieve near the same thing).
>>
>>
>> Maybe I am missing the point and you already have in mind use cases that
>> could take advantage of the current design?
>>
>>
>> Eugen
>>
>> 2015-01-30 19:52 GMT+01:00 Eugen Cepoi <cepoi.eugen_at_gmail.com>:
>>
>>> Hey Martin,
>>>
>>> Do you expect us to post our comments only on the ML or perhaps is there
>>> a way to comment directly the code "à la github" on java.net?
>>> BTW I see ssh/user_name in your URL, are we supposed to have a write
>>> access to the repo and can maybe contribute some code?
>>>
>>> Eugen
>>>
>>> 2015-01-30 18:02 GMT+01:00 Martin Grebac <martin.grebac_at_oracle.com>:
>>>
>>>> Hi experts,
>>>> welcome again. We're well ahead in 2015, and ready to run full speed
>>>> on the JSON Binding API. As you may have already noticed, I've put together
>>>> an initial list of individual API/spec [1] items which should lead us to
>>>> our first quickly approaching milestone - Early Draft. I expect this to be
>>>> a fairly live list, updated regularly based on progress and status.
>>>>
>>>> As a first step, I'd like to submit for review proposal for runtime
>>>> api. One thing to state first, this initial step does not contain 2 obvious
>>>> items: full generic types and JSON-P support. I'll open these in coming
>>>> subsequent proposals as I believe they warrant separate discussion.
>>>>
>>>> To review, please checkout the remote branch:
>>>>
>>>> git clone ssh://<user_name>@git.java.net/jsonb-spec~git jsonb
>>>> cd jsonb
>>>> git checkout runtime_api_init
>>>>
>>>> Folder description:
>>>> api - api code
>>>> examples - code examples, not expected to run
>>>> spec - spec document source code in tex, spec/spec.pdf is current
>>>> snapshot of the document
>>>>
>>>> Build with Maven (3.2, JDK8), built API javadoc is located in
>>>> api/target/apidocs folder.
>>>>
>>>> Looking forward to your feedback - please send your comments by Friday
>>>> next week (Feb06).
>>>>
>>>> Thank you,
>>>> MartiNG
>>>>
>>>> [1] https://java.net/jira/secure/RapidBoard.jspa?rapidView=27&
>>>> view=planning.nodetail&versions=visible
>>>>
>>>>
>>>
>>
>