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?
*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.
*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)
*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.
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 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.
*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
>>>
>>>
>>
>