Hey Martin,
2015-02-02 14:23 GMT+01:00 Martin Grebac <martin.grebac_at_oracle.com>:
> Hi,
> thanks all for all the comments, let me try to address at least some of
> them ;O)
>
> Just a general comment - as I mentioned I intentionally left out many
> areas in the initial proposal, such as JSON-P, io streams, ... because I
> did not consider them important to deal with wrt the initial api skeleton.
>
> On 30.01.15 22:34, Eugen Cepoi wrote:
>
>> Martin, here are some (opinionated :p) comments...*
>> *
>>
> Opinions are needed, let's work to reach some consensus in our opinions :).
>
> *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... :( )?
>>
> I'd like to hear more opinions here, the question here is whether the
> usecase is strong enough such that it is not going to be too big
> requirement for all binding implementations to support it. Also, in javadoc
> the api allows empty set.
>
It is not hard to impl., my concern is about making some rare use case the
default, most people just want to ser/de what they pass in - versus
explicitly tell what will be the classes being ser/de. I understand this
feature as: if a class is not in this list and people try to ser it then an
exception is thrown.
>
> Missing unmarshal/marshal methods taking input/outputstream and bytes.
>>
> Intentionally left out, the reason is similar to the above, to evaluate
> whether streams are really needed once there are readers/writers and JSON
> being inherently a UTF text source/output so one does not have to deal with
> encodings. I recall I've also seen binding implementations which left io
> streams out.
> So, is there really a strong need to keep streams?
>
>
In genson I left it out and then added it as I figured that people tended
to copy the inputstream to a string and then pass the string...instead of
just wrapping it in a reader. But it is more of a remark that it could be
nice to have, especially if we already have File.
> 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...)
>>
> Whether or not I like shortcut methods, there are usecases where I think
> they make sense, where e.g. writing concise and easy to understand code is
> more important than achieving optimal performance.
Agreed, note that the difference in performance is important if you do
introspection each time.
> Such as tests, e.g., or minor use of the api within an application. Wrt
> javadoc comment I'd swear I had the javadoc comment in there, but it seems
> it disappeared within some of my edits. My choice would be to keep shortcut
> access and clarify the javadoc.
>
If Jsonb class remains only a holder for shortcuts and context creation, I
agree with you in keeping those methods and documenting the preferred usage.
>
> *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?
>>
> Professional deformation, happens to me, hopefully less over time :). Will
> be gone, found some more typos like these.
>
> *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?
>>
> For the purpose of the discussion I'd leave the callbacks out for now for
> both marshaller & unmarshaller and remove them from the initial proposal,
> they really do not belong to the discussion now. Will open up the
> discussion later.
>
> *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.
>>
> I think makes sense, the code is not much shorter. If there are no
> objections, I will remove these in next iteration.
>
> /"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.
>>
> So, you're suggesting for null input the method should return null? Why? I
> think the application code would need to deal with malformed code path in
> most cases, it does not have to deal with null though.
>
>
I feel it more natural to get null when the input is empty or root value is
null. I am thinking of a server returning empty json for errors, but
setting error codes and stuff like that. But perhaps it would be
interesting to have other opinions.
> *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.
>>
> If there is a specific reason to deviate, we can do so, but keeping the
> apis with similar feel has its benefits in adoption and lower learning
> curve for all those already aware of JAXB api.
>
>
What about the existing json libs API? Don't you think they differ from
JAXB, but they all have a similar API and people are used with them when
working with json? We want to provide an api for json users not JAXB users.
> - 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.
>>
> Yes, dealing with custom is something we'll discuss later.
>
It would be interesting to see some draft/examples of default ser/de as it
might explain or contradict some stuff.
>
> - I was expecting to see jsonp reader/writers instead of std java io
>> classes in Un/Marhsaller.
>>
> Left out for now.
>
>
I know you mentioned it but I wonder how do you see this integration with
jsonp? I was thinking that JSONB would be a layer over JSONP, thus using it
for all the low level io stuff - like what we see in json libs with the
streaming layer vs the databinding one.
> - Do we want the Un/Marshaller to be mutable (I see those setProperty
>> methods)?
>>
> That is a design decision. There's more dicussion following in this
> thread, so I'll reply there.
>
> - 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(...);
>>
>> I think this goes together with the mutability issue above.
Yes and also with some other comments about global design, shortcuts, api
familiar to users...
> Say for your example here, if the application code wants to use 2 ways to
> use marshaller - with formatted output and without, it has to create and
> hold 2 Jsonb instances, whereas with mutable marshaller/unmarshaller this
> usecase would perform better and code would be shorter.
It depends...if un/marshaller are reused then you can't do it (as you would
mutate the state for all the other threads that maybe don't want pretty
printing). If a new one is created each time... then I am curious to see
what the rest of the design looks like :)
> Also, the above does not allow to configure marshaller and unmarshaller
> differently and as well more-less assumes that marshaller and unmarshaller
> instances both are going to be used in the usecase.
I don't see the problem of assuming that both are being used. At worst the
user pays the small performance cost of un/marshaller instantiation and
configuration.
It does not prevent from configuring both separately, it can be different
configuration methods or keys. But this is another thing than making things
mutable.
About JsonbContext, what is its goal and responsibility? The above example
is also a suggestion on having Jsonb as a single entry point (and dropping
JsonbContext), the shortcut static methods become object methods and the
Builder configures Jsonb and the Un/Marshaller.
Jsonb and all the other components become immutable.
Would be again interesting to see other opinions :)
Eugen
There are usecases and applications where only one is needed (either
> marshaller or unmarshaller).
>
> MartiNG
>
> 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 <mailto:
>> 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
>> <http://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
>> <mailto: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
>> <http://git.java.net/jsonb-spec%7Egit> 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=visibl
>> <https://java.net/jira/secure/RapidBoard.jspa?rapidView=27&
>> view=planning.nodetail&versions=visible>
>>
>>
>>