Improving tapestry-json

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

Improving tapestry-json

Ben Weidig
Hi,

I'm thinking about improving tapestry-json but wanted to ask first if
there's actually some interest in doing so, before starting to code and
making a pull-request.

We have multiple utility classes for simpler JSON usage:

JSONArray:
- static JSONArray safeNew(String jsonStr)
- JSONArray from(Iterable<?> iterable)
- JSONArray from(Iterable<?> iterable, boolean preserveNull)
- boolean putUnique(Object obj)
- multiple typed toList(...) which might be refactored into a single
generic method

JSONObject:
- static JSONObject safeNew(String jsonStr)
- <K, V> Map<K, V> toMap(Function<String, K> keyFn, Function<Object, V>
valueFn)
- static JSONObject from(Map<String, Object> map)

JSONCollectors:
- static <T> Collector<T, JSONArray, JSONArray> toArray()
- a collector to JSONObject wasn't needed so far

Other functionality I would like to add:
- Stream-Support for JSONArray
- Safe getters, e.g., Optional<String> JSONArray#tryGet(String key),
although I haven't thought much about correct null vs NULL-sentinel
handling.

What do you think? Worth the effort, or really on utility classes?

Ben
Reply | Threaded
Open this post in threaded view
|

Re: Improving tapestry-json

Christopher
Hi Ben,

I use JSON quite extensively when pushing updates out from a Tapestry
service, via websocket, to remote desktop client apps.  So I'm keen for
any Tapestry enhancements that might streamline JSON support.

Mostly I use com.fasterxml.jackson for this.

Kind regards,

Chris Dodunski.


On 2020-07-25 19:33, Ben Weidig wrote:

> Hi,
>
> I'm thinking about improving tapestry-json but wanted to ask first if
> there's actually some interest in doing so, before starting to code and
> making a pull-request.
>
> We have multiple utility classes for simpler JSON usage:
>
> JSONArray:
> - static JSONArray safeNew(String jsonStr)
> - JSONArray from(Iterable<?> iterable)
> - JSONArray from(Iterable<?> iterable, boolean preserveNull)
> - boolean putUnique(Object obj)
> - multiple typed toList(...) which might be refactored into a single
> generic method
>
> JSONObject:
> - static JSONObject safeNew(String jsonStr)
> - <K, V> Map<K, V> toMap(Function<String, K> keyFn, Function<Object, V>
> valueFn)
> - static JSONObject from(Map<String, Object> map)
>
> JSONCollectors:
> - static <T> Collector<T, JSONArray, JSONArray> toArray()
> - a collector to JSONObject wasn't needed so far
>
> Other functionality I would like to add:
> - Stream-Support for JSONArray
> - Safe getters, e.g., Optional<String> JSONArray#tryGet(String key),
> although I haven't thought much about correct null vs NULL-sentinel
> handling.
>
> What do you think? Worth the effort, or really on utility classes?
>
> Ben

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

"A designer knows he has achieved perfection not when there is nothing left to add, but when there is nothing left to take away." - Antoine de Saint-Exupéry.
Reply | Threaded
Open this post in threaded view
|

Re: Improving tapestry-json

David Taylor
In reply to this post by Ben Weidig
Hi Ben,

Improvements would be quite welcome. We use the Tapestry JSON classes in
many places. One issue we have is that they are rather unfriendly when
it comes to parsing input received from the browser.  The Tapestry class
are very much a happy path design and do not provide a means to test the
validity of data. The exceptions thrown are very generic and not much
help either.

David

On 7/25/2020 3:33 AM, Ben Weidig wrote:

> Hi,
>
> I'm thinking about improving tapestry-json but wanted to ask first if
> there's actually some interest in doing so, before starting to code and
> making a pull-request.
>
> We have multiple utility classes for simpler JSON usage:
>
> JSONArray:
> - static JSONArray safeNew(String jsonStr)
> - JSONArray from(Iterable<?> iterable)
> - JSONArray from(Iterable<?> iterable, boolean preserveNull)
> - boolean putUnique(Object obj)
> - multiple typed toList(...) which might be refactored into a single
> generic method
>
> JSONObject:
> - static JSONObject safeNew(String jsonStr)
> - <K, V> Map<K, V> toMap(Function<String, K> keyFn, Function<Object, V>
> valueFn)
> - static JSONObject from(Map<String, Object> map)
>
> JSONCollectors:
> - static <T> Collector<T, JSONArray, JSONArray> toArray()
> - a collector to JSONObject wasn't needed so far
>
> Other functionality I would like to add:
> - Stream-Support for JSONArray
> - Safe getters, e.g., Optional<String> JSONArray#tryGet(String key),
> although I haven't thought much about correct null vs NULL-sentinel
> handling.
>
> What do you think? Worth the effort, or really on utility classes?
>
> Ben
>



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Improving tapestry-json

Ben Weidig
Hi,

@Chris: Where it's feasible we use Jackson, too. But sometimes it's easier
to just use a more "dumb but still JSON-compatible" type without needing an
ObjectMapper.
And the first-class support of in many parts of Tapestry makes it a better
choice for smaller use-cases. So more functionality in these types would be
great.

@David: I didn't want to touch much of the existing stuff, but you're
right. The "happy path" design can be such a nuisance sometimes...

Looks like I'm going to write a small proposal of my planned changes and
additions, to have something to discuss, and to better manage scope.

Ben
Reply | Threaded
Open this post in threaded view
|

Re: Improving tapestry-json

Ben Weidig
I wrote up a small file with all the changes I would like to see/do in
tapestry-json.

Either nicely formatted as Gist:
https://gist.github.com/benweidig/21d3876188a7761632816c1f2684b102


Or raw in the mailing list, for archival purposes:


tapestry-json Improvements
==========================

Summary
-------

Add modern Java 8 features to tapestry-json to make it more versatile.
Improve existing functionality without breaking changes.

Goals
-----

With Tapestry development picking up speed and supporting newer Java
version, its dependencies should also support some of the newly available
features:

* Stream-support
* Additional instance creators
* Approximate to the conceptional interfaces (Collection/Map)
* Generic methods
* Better/more informative exceptions

The general idea is making `JSONArray` more like `java.util.Collection`,
and `JSONObject` more like `java.util.Map`, without implementing the
interfaces explicitly.
If we would implement them directly, it would method duplication (`size()`
vs. `length()`, `add()` vs `put(...)`).

Motivation
----------

Tapestry's use of JSON types is deeply ingrained into its services.
By using its own types, we can improve upon them to adapt to any new
features.
The framework and Java itself evolved, and they should be too.

Java 8 was bringing a lot of new features, like lambdas, Streams, and
Optionals, thereby changing the way we can deal with collection types in a
significant way.

It's time to adapt tapestry-json to the new features available to us.


Risks and Assumptions
---------------------

Only minimal risk is assumed.

Existing functionality shouldn't be changed, for backward-compatibility.
Any new feature has to be able to stand on its own.

The only proposed change will be the exact exception types and messages.
Changing the type should not pose a problem, they will still be descendant
of `RuntimeException`.
And relying on the actual exception message is bad practice, and shouldn't
be part of the guaranteed API contract.

Alternatives
------------

Instead of extending existing types we could always use utility classes to
wrap the functionality.


Details of Proposed Changes
----------------------------

Most of the proposed changes are based on utility classes used in our
projects.
But some are just of theoretical nature, and might not be as easy as
implementable as thought.


### JSONArray

* `Stream<Object> stream()`
* `Stream<T> stream(Class<T> clazz)`
* `Stream<Object> parallelStream()`
* `Stream<T> parallelStream(Class<T> clazz)`
* `List<T> toList(Class<T> clazz)`
* `static JSONArray from(Iterable<?> iterable, boolean preserveNull)`
* `JSONArray(String jsonStr, boolean emptyIfInvalid)`
* `boolean putUnique(Object value)`
* `boolean isEmpty()`
* `boolean contains(Object obj)`
* `boolean removeIf(Predicate<Object> filter)`
* `boolean removeAll(Iterable<Object> iterable)`
* `void clear()`

### JSONObject

* `Map<String, T> toMap(Class<T> valueClazz)`
* `Optional<Type> tryGetType(String key)` for all types
* `Type getType(String key, Type fallbackValue)` for all types
* `static JSONObject from(Map<String, ?> map)`
* `void clear()`
* `void clear()`
* `Object putIfAbsent(String key, Object value)`
* `Object computeIfAbsent(String key, Function<String, Object> mapFn)`
* `Object computeIfPresent(String key, Function<String, Object> remapFn)`
* `void merge(JSONObject other)`

### Exceptions

* Use `IndexOutOfBoundsException` correctly
* `org.apache.tapestry5.json.JSON` should throw more fine-grained
exceptions:
  * Add `JSONTypeUnsupportedException`
  * Add `JSONTypeMismatchException`
  * Add `JSONValueNotFoundException`

### Streams

* Add Collectors to improve `Stream` use



Testing
-------

All added functionality should be unit tested to ensure no existing code
will be broken by it.






On Sat, Jul 25, 2020 at 1:12 PM Ben Weidig <[hidden email]> wrote:

> Hi,
>
> @Chris: Where it's feasible we use Jackson, too. But sometimes it's easier
> to just use a more "dumb but still JSON-compatible" type without needing an
> ObjectMapper.
> And the first-class support of in many parts of Tapestry makes it a better
> choice for smaller use-cases. So more functionality in these types would be
> great.
>
> @David: I didn't want to touch much of the existing stuff, but you're
> right. The "happy path" design can be such a nuisance sometimes...
>
> Looks like I'm going to write a small proposal of my planned changes and
> additions, to have something to discuss, and to better manage scope.
>
> Ben
>


--

Netzgut GmbH

Kirchstr. 18
69115 Heidelberg

Telefon:
+49 6221 39298 53

Telefax:
+49 6221 39298 59

E-Mail:[hidden email]

Handelsregister: Amtsgericht Mannheim, HRB 709833
Sitz der Gesellschaft: Heidelberg
Geschäftsführer: Felix Gonschorek, Benjamin Weidig
Ust-IdNr.: DE272871752
Reply | Threaded
Open this post in threaded view
|

Re: Improving tapestry-json

Thiago H de Paula Figueiredo
In reply to this post by Ben Weidig
On Sat, Jul 25, 2020 at 4:33 AM Ben Weidig <[hidden email]> wrote:

> Hi,
>

Hello!


> I'm thinking about improving tapestry-json but wanted to ask first if
> there's actually some interest in doing so, before starting to code and
> making a pull-request.
>

I cannot speak for the whole team or the project, but I'm definitely
interested in contributions to the Tapestry project. The only concerns I'd
have are backward compatibility (always very important for the project) and
test coverage, but you've already covered that in your proposal, so we're
off to a great start. :)

--
Thiago
Reply | Threaded
Open this post in threaded view
|

Re: Improving tapestry-json

Thiago H de Paula Figueiredo
In reply to this post by Ben Weidig
On Sat, Jul 25, 2020 at 8:12 AM Ben Weidig <[hidden email]> wrote:

> Hi,
>

Hello!


> @Chris: Where it's feasible we use Jackson, too. But sometimes it's easier
> to just use a more "dumb but still JSON-compatible" type without needing an
> ObjectMapper.
> And the first-class support of in many parts of Tapestry makes it a better
> choice for smaller use-cases. So more functionality in these types would be
> great.
>

Indeed, Jackson is great when you want to map JSON to Java objects.
Tapestry's own JSON support, as far as I know, was built to cover what
Tapestry itself needed, mostly AJAX support, not trying to be a generic
JSON library (although it can be used for that, of course). I agree that
more functionality in the JSONObject class family would be great.

Regarding a possible support for Jackson in Tapestry itself, I'd do it in a
separate subproject so we don't introduce another dependency which many
projects may not need. We could, for example, support Jackson types and
Jackson-mapped objects in the return value of onActivate() and event
handler methods just as JSONObject and JSONArray are, make it easy to have
type coercions to and from Jackson mapped objects, etc. Unfortunately, I
don't have the time to do it and now I'm focusing on getting Tapestry 5.6
released and then 5.7 with proper Java 9 modules support.


> --
Thiago
Reply | Threaded
Open this post in threaded view
|

Re: Improving tapestry-json

Thiago H de Paula Figueiredo
In reply to this post by Ben Weidig
On Sun, Jul 26, 2020 at 5:41 AM Ben Weidig <[hidden email]> wrote:

> * Stream-support
> * Additional instance creators
> * Approximate to the conceptional interfaces (Collection/Map)
> * Generic methods
> * Better/more informative exceptions
>

I really love everything here, specially the idea of having better, more
informative exceptions.


> The general idea is making `JSONArray` more like `java.util.Collection`,
> and `JSONObject` more like `java.util.Map`, without implementing the
> interfaces explicitly.
> If we would implement them directly, it would method duplication (`size()`
> vs. `length()`, `add()` vs `put(...)`).
>

I don't mind making JSONArray implement Collection<JSONCollection> and
JSONObject implement Map<String,JSONCollection> (JSONCollection is the
JSONObject and JSONArray superclass). Actually, I like the idea. Even if
it's not adopted, we could have they implementing Iterable.


>
> Motivation
> ----------
>
> Tapestry's use of JSON types is deeply ingrained into its services.
> By using its own types, we can improve upon them to adapt to any new
> features.
> The framework and Java itself evolved, and they should be too.
>
> Java 8 was bringing a lot of new features, like lambdas, Streams, and
> Optionals, thereby changing the way we can deal with collection types in a
> significant way.
>
> It's time to adapt tapestry-json to the new features available to us.
>

I definitely agree here.


> Testing
> -------
>
> All added functionality should be unit tested to ensure no existing code
> will be broken by it.
>

This is very, very important. Fortunately, we already have many tests in
tapestry-json (and in Tapestry overall) to cover backward compatibility. Of
course, new tests to cover the new functionality would be great.

Regarding everything else, I agree. I'm looking forward to this!


>
>
>
>
>
>
> On Sat, Jul 25, 2020 at 1:12 PM Ben Weidig <[hidden email]> wrote:
>
> > Hi,
> >
> > @Chris: Where it's feasible we use Jackson, too. But sometimes it's
> easier
> > to just use a more "dumb but still JSON-compatible" type without needing
> an
> > ObjectMapper.
> > And the first-class support of in many parts of Tapestry makes it a
> better
> > choice for smaller use-cases. So more functionality in these types would
> be
> > great.
> >
> > @David: I didn't want to touch much of the existing stuff, but you're
> > right. The "happy path" design can be such a nuisance sometimes...
> >
> > Looks like I'm going to write a small proposal of my planned changes and
> > additions, to have something to discuss, and to better manage scope.
> >
> > Ben
> >
>
>
> --
>
> Netzgut GmbH
>
> Kirchstr. 18
> 69115 Heidelberg
>
> Telefon:
> +49 6221 39298 53
>
> Telefax:
> +49 6221 39298 59
>
> E-Mail:[hidden email]
>
> Handelsregister: Amtsgericht Mannheim, HRB 709833
> Sitz der Gesellschaft: Heidelberg
> Geschäftsführer: Felix Gonschorek, Benjamin Weidig
> Ust-IdNr.: DE272871752
>


--
Thiago
Reply | Threaded
Open this post in threaded view
|

Re: Improving tapestry-json

Ben Weidig
Hi Thiago,

thanks for the feedback!

> I don't mind making JSONArray implement Collection<JSONCollection> and
> JSONObject implement Map<String,JSONCollection> (JSONCollection is the
> JSONObject and JSONArray superclass). Actually, I like the idea. Even if
> it's not adopted, we could have they implementing Iterable.

Do you mean Collection<Object> and Map<String, Object>?
Using JSONCollection for the value type doesn't make much sense based on
the underlying List<Object> / LinkedHashMap<String, Object>.
Or I do not understand what you exactly mean.

Implementing the interfaces directly would be the best option. This way,
the JSON types become specialized collection types, compatible with all the
others.
And they would gain a lot of functionality for free thanks to default
methods.
The duplicated methods should be marked deprecated in favor of the
Collection/Map ones.

Only problems I've seen at first glance:

* Signature change: JSONObject get(Object name) instead of JSONObject
get(String name)
* Return type / Signature change: void putAll(Map<? extends String, ?
extends Object> newProperties) instead of JSONObject putAll(Map<String, ?>
newProperties)

The second one would be a real breaking change, but IMO an acceptable one,
because it's easy to fix, and doesn't introduce side-effects.

Hopefully, I get some work done this week. If I run into any bigger issues,
I'll present my findings.

Ben


On Sun, Jul 26, 2020 at 9:51 PM Thiago H. de Paula Figueiredo <
[hidden email]> wrote:

> On Sun, Jul 26, 2020 at 5:41 AM Ben Weidig <[hidden email]> wrote:
>
> > * Stream-support
> > * Additional instance creators
> > * Approximate to the conceptional interfaces (Collection/Map)
> > * Generic methods
> > * Better/more informative exceptions
> >
>
> I really love everything here, specially the idea of having better, more
> informative exceptions.
>
>
> > The general idea is making `JSONArray` more like `java.util.Collection`,
> > and `JSONObject` more like `java.util.Map`, without implementing the
> > interfaces explicitly.
> > If we would implement them directly, it would method duplication
> (`size()`
> > vs. `length()`, `add()` vs `put(...)`).
> >
>
> I don't mind making JSONArray implement Collection<JSONCollection> and
> JSONObject implement Map<String,JSONCollection> (JSONCollection is the
> JSONObject and JSONArray superclass). Actually, I like the idea. Even if
> it's not adopted, we could have they implementing Iterable.
>
>
> >
> > Motivation
> > ----------
> >
> > Tapestry's use of JSON types is deeply ingrained into its services.
> > By using its own types, we can improve upon them to adapt to any new
> > features.
> > The framework and Java itself evolved, and they should be too.
> >
> > Java 8 was bringing a lot of new features, like lambdas, Streams, and
> > Optionals, thereby changing the way we can deal with collection types in
> a
> > significant way.
> >
> > It's time to adapt tapestry-json to the new features available to us.
> >
>
> I definitely agree here.
>
>
> > Testing
> > -------
> >
> > All added functionality should be unit tested to ensure no existing code
> > will be broken by it.
> >
>
> This is very, very important. Fortunately, we already have many tests in
> tapestry-json (and in Tapestry overall) to cover backward compatibility. Of
> course, new tests to cover the new functionality would be great.
>
> Regarding everything else, I agree. I'm looking forward to this!
>
>
> >
> >
> >
> >
> >
> >
> > On Sat, Jul 25, 2020 at 1:12 PM Ben Weidig <[hidden email]> wrote:
> >
> > > Hi,
> > >
> > > @Chris: Where it's feasible we use Jackson, too. But sometimes it's
> > easier
> > > to just use a more "dumb but still JSON-compatible" type without
> needing
> > an
> > > ObjectMapper.
> > > And the first-class support of in many parts of Tapestry makes it a
> > better
> > > choice for smaller use-cases. So more functionality in these types
> would
> > be
> > > great.
> > >
> > > @David: I didn't want to touch much of the existing stuff, but you're
> > > right. The "happy path" design can be such a nuisance sometimes...
> > >
> > > Looks like I'm going to write a small proposal of my planned changes
> and
> > > additions, to have something to discuss, and to better manage scope.
> > >
> > > Ben
> > >
> >
> >
> >
>
>
> --
> Thiago
>
Reply | Threaded
Open this post in threaded view
|

Re: Improving tapestry-json

David Taylor
In reply to this post by Ben Weidig
The proposal looks great and addresses all of our concerns and needs.
The changes to exception handling would be very welcome since the
current approach is problematic when implementing robust error handling
while decoding JSON data.

Best Regards,
David
emailsig

On 7/26/2020 4:40 AM, Ben Weidig wrote:

> I wrote up a small file with all the changes I would like to see/do in
> tapestry-json.
>
> Either nicely formatted as Gist:
> https://gist.github.com/benweidig/21d3876188a7761632816c1f2684b102
>
>
> Or raw in the mailing list, for archival purposes:
>
>
> tapestry-json Improvements
> ==========================
>
> Summary
> -------
>
> Add modern Java 8 features to tapestry-json to make it more versatile.
> Improve existing functionality without breaking changes.
>
> Goals
> -----
>
> With Tapestry development picking up speed and supporting newer Java
> version, its dependencies should also support some of the newly available
> features:
>
> * Stream-support
> * Additional instance creators
> * Approximate to the conceptional interfaces (Collection/Map)
> * Generic methods
> * Better/more informative exceptions
>
> The general idea is making `JSONArray` more like `java.util.Collection`,
> and `JSONObject` more like `java.util.Map`, without implementing the
> interfaces explicitly.
> If we would implement them directly, it would method duplication (`size()`
> vs. `length()`, `add()` vs `put(...)`).
>
> Motivation
> ----------
>
> Tapestry's use of JSON types is deeply ingrained into its services.
> By using its own types, we can improve upon them to adapt to any new
> features.
> The framework and Java itself evolved, and they should be too.
>
> Java 8 was bringing a lot of new features, like lambdas, Streams, and
> Optionals, thereby changing the way we can deal with collection types in a
> significant way.
>
> It's time to adapt tapestry-json to the new features available to us.
>
>
> Risks and Assumptions
> ---------------------
>
> Only minimal risk is assumed.
>
> Existing functionality shouldn't be changed, for backward-compatibility.
> Any new feature has to be able to stand on its own.
>
> The only proposed change will be the exact exception types and messages.
> Changing the type should not pose a problem, they will still be descendant
> of `RuntimeException`.
> And relying on the actual exception message is bad practice, and shouldn't
> be part of the guaranteed API contract.
>
> Alternatives
> ------------
>
> Instead of extending existing types we could always use utility classes to
> wrap the functionality.
>
>
> Details of Proposed Changes
> ----------------------------
>
> Most of the proposed changes are based on utility classes used in our
> projects.
> But some are just of theoretical nature, and might not be as easy as
> implementable as thought.
>
>
> ### JSONArray
>
> * `Stream<Object> stream()`
> * `Stream<T> stream(Class<T> clazz)`
> * `Stream<Object> parallelStream()`
> * `Stream<T> parallelStream(Class<T> clazz)`
> * `List<T> toList(Class<T> clazz)`
> * `static JSONArray from(Iterable<?> iterable, boolean preserveNull)`
> * `JSONArray(String jsonStr, boolean emptyIfInvalid)`
> * `boolean putUnique(Object value)`
> * `boolean isEmpty()`
> * `boolean contains(Object obj)`
> * `boolean removeIf(Predicate<Object> filter)`
> * `boolean removeAll(Iterable<Object> iterable)`
> * `void clear()`
>
> ### JSONObject
>
> * `Map<String, T> toMap(Class<T> valueClazz)`
> * `Optional<Type> tryGetType(String key)` for all types
> * `Type getType(String key, Type fallbackValue)` for all types
> * `static JSONObject from(Map<String, ?> map)`
> * `void clear()`
> * `void clear()`
> * `Object putIfAbsent(String key, Object value)`
> * `Object computeIfAbsent(String key, Function<String, Object> mapFn)`
> * `Object computeIfPresent(String key, Function<String, Object> remapFn)`
> * `void merge(JSONObject other)`
>
> ### Exceptions
>
> * Use `IndexOutOfBoundsException` correctly
> * `org.apache.tapestry5.json.JSON` should throw more fine-grained
> exceptions:
>    * Add `JSONTypeUnsupportedException`
>    * Add `JSONTypeMismatchException`
>    * Add `JSONValueNotFoundException`
>
> ### Streams
>
> * Add Collectors to improve `Stream` use
>
>
>
> Testing
> -------
>
> All added functionality should be unit tested to ensure no existing code
> will be broken by it.
>
>
>
>
>
>
> On Sat, Jul 25, 2020 at 1:12 PM Ben Weidig <[hidden email]> wrote:
>
>> Hi,
>>
>> @Chris: Where it's feasible we use Jackson, too. But sometimes it's easier
>> to just use a more "dumb but still JSON-compatible" type without needing an
>> ObjectMapper.
>> And the first-class support of in many parts of Tapestry makes it a better
>> choice for smaller use-cases. So more functionality in these types would be
>> great.
>>
>> @David: I didn't want to touch much of the existing stuff, but you're
>> right. The "happy path" design can be such a nuisance sometimes...
>>
>> Looks like I'm going to write a small proposal of my planned changes and
>> additions, to have something to discuss, and to better manage scope.
>>
>> Ben
>>
>



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Improving tapestry-json

Ben Weidig
Hi,

I've started with improving the exceptions, due to the lower impact:

https://github.com/apache/tapestry-5/compare/master...benweidig:feature/tapestry-json-exceptions?expand=1

The following new exceptions were added:
- JSONArrayIndexOutOfBoundsException
- JSONInvalidTypeException
- JSONSyntaxException
- JSONTypeMismatchException
- JSONValueNotFoundException

They're all trying to provide as much additional info as possible.
Therefore, I've also added the enum org.apache.tapestry5.json.JSONType, to
better represent valid JSON data types, and no longer use magic strings for
exception messages.

I've updated the JavaDoc to reflect the thrown exceptions, but I'm not
finished yet.

Specs were adapted as needed, and new tests were added if appropriate.

One possible bug / discrepancy between Javadoc and actual code was found:
The constructor org.apache.tapestry5.json.JSONArray.JSONArray(Object...) is
saying that it checks for non-finite doubles, but it wasn't using the
checkedPut method. I've changed that, and wrote a test for it.

New files were formatted with the provided Eclipse style, but the project
itself is quite a mixed bag of formatting.

These are my first changes, any feedback is welcome. The next step would be
checking any remaining RuntimeException if it could be replaced with a more
specific one.

Best regards
Ben

On Sat, Aug 1, 2020 at 7:10 AM David Taylor <[hidden email]>
wrote:

> The proposal looks great and addresses all of our concerns and needs.
> The changes to exception handling would be very welcome since the
> current approach is problematic when implementing robust error handling
> while decoding JSON data.
>
> Best Regards,
> David
> emailsig
>
> On 7/26/2020 4:40 AM, Ben Weidig wrote:
> > I wrote up a small file with all the changes I would like to see/do in
> > tapestry-json.
> >
> > Either nicely formatted as Gist:
> > https://gist.github.com/benweidig/21d3876188a7761632816c1f2684b102
> >
> >
> > Or raw in the mailing list, for archival purposes:
> >
> >
> > tapestry-json Improvements
> > ==========================
> >
> > Summary
> > -------
> >
> > Add modern Java 8 features to tapestry-json to make it more versatile.
> > Improve existing functionality without breaking changes.
> >
> > Goals
> > -----
> >
> > With Tapestry development picking up speed and supporting newer Java
> > version, its dependencies should also support some of the newly available
> > features:
> >
> > * Stream-support
> > * Additional instance creators
> > * Approximate to the conceptional interfaces (Collection/Map)
> > * Generic methods
> > * Better/more informative exceptions
> >
> > The general idea is making `JSONArray` more like `java.util.Collection`,
> > and `JSONObject` more like `java.util.Map`, without implementing the
> > interfaces explicitly.
> > If we would implement them directly, it would method duplication
> (`size()`
> > vs. `length()`, `add()` vs `put(...)`).
> >
> > Motivation
> > ----------
> >
> > Tapestry's use of JSON types is deeply ingrained into its services.
> > By using its own types, we can improve upon them to adapt to any new
> > features.
> > The framework and Java itself evolved, and they should be too.
> >
> > Java 8 was bringing a lot of new features, like lambdas, Streams, and
> > Optionals, thereby changing the way we can deal with collection types in
> a
> > significant way.
> >
> > It's time to adapt tapestry-json to the new features available to us.
> >
> >
> > Risks and Assumptions
> > ---------------------
> >
> > Only minimal risk is assumed.
> >
> > Existing functionality shouldn't be changed, for backward-compatibility.
> > Any new feature has to be able to stand on its own.
> >
> > The only proposed change will be the exact exception types and messages.
> > Changing the type should not pose a problem, they will still be
> descendant
> > of `RuntimeException`.
> > And relying on the actual exception message is bad practice, and
> shouldn't
> > be part of the guaranteed API contract.
> >
> > Alternatives
> > ------------
> >
> > Instead of extending existing types we could always use utility classes
> to
> > wrap the functionality.
> >
> >
> > Details of Proposed Changes
> > ----------------------------
> >
> > Most of the proposed changes are based on utility classes used in our
> > projects.
> > But some are just of theoretical nature, and might not be as easy as
> > implementable as thought.
> >
> >
> > ### JSONArray
> >
> > * `Stream<Object> stream()`
> > * `Stream<T> stream(Class<T> clazz)`
> > * `Stream<Object> parallelStream()`
> > * `Stream<T> parallelStream(Class<T> clazz)`
> > * `List<T> toList(Class<T> clazz)`
> > * `static JSONArray from(Iterable<?> iterable, boolean preserveNull)`
> > * `JSONArray(String jsonStr, boolean emptyIfInvalid)`
> > * `boolean putUnique(Object value)`
> > * `boolean isEmpty()`
> > * `boolean contains(Object obj)`
> > * `boolean removeIf(Predicate<Object> filter)`
> > * `boolean removeAll(Iterable<Object> iterable)`
> > * `void clear()`
> >
> > ### JSONObject
> >
> > * `Map<String, T> toMap(Class<T> valueClazz)`
> > * `Optional<Type> tryGetType(String key)` for all types
> > * `Type getType(String key, Type fallbackValue)` for all types
> > * `static JSONObject from(Map<String, ?> map)`
> > * `void clear()`
> > * `void clear()`
> > * `Object putIfAbsent(String key, Object value)`
> > * `Object computeIfAbsent(String key, Function<String, Object> mapFn)`
> > * `Object computeIfPresent(String key, Function<String, Object> remapFn)`
> > * `void merge(JSONObject other)`
> >
> > ### Exceptions
> >
> > * Use `IndexOutOfBoundsException` correctly
> > * `org.apache.tapestry5.json.JSON` should throw more fine-grained
> > exceptions:
> >    * Add `JSONTypeUnsupportedException`
> >    * Add `JSONTypeMismatchException`
> >    * Add `JSONValueNotFoundException`
> >
> > ### Streams
> >
> > * Add Collectors to improve `Stream` use
> >
> >
> >
> > Testing
> > -------
> >
> > All added functionality should be unit tested to ensure no existing code
> > will be broken by it.
> >
> >
> >
> >
> >
> >
> > On Sat, Jul 25, 2020 at 1:12 PM Ben Weidig <[hidden email]> wrote:
> >
> >> Hi,
> >>
> >> @Chris: Where it's feasible we use Jackson, too. But sometimes it's
> easier
> >> to just use a more "dumb but still JSON-compatible" type without
> needing an
> >> ObjectMapper.
> >> And the first-class support of in many parts of Tapestry makes it a
> better
> >> choice for smaller use-cases. So more functionality in these types
> would be
> >> great.
> >>
> >> @David: I didn't want to touch much of the existing stuff, but you're
> >> right. The "happy path" design can be such a nuisance sometimes...
> >>
> >> Looks like I'm going to write a small proposal of my planned changes and
> >> additions, to have something to discuss, and to better manage scope.
> >>
> >> Ben
> >>
> >
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>
Reply | Threaded
Open this post in threaded view
|

Re: Improving tapestry-json

Thiago H de Paula Figueiredo
In reply to this post by Ben Weidig
On Mon, Jul 27, 2020 at 9:15 AM Ben Weidig <[hidden email]> wrote:

> Hi Thiago,
>
> thanks for the feedback!
>
> > I don't mind making JSONArray implement Collection<JSONCollection> and
> > JSONObject implement Map<String,JSONCollection> (JSONCollection is the
> > JSONObject and JSONArray superclass). Actually, I like the idea. Even if
> > it's not adopted, we could have they implementing Iterable.
>
> Do you mean Collection<Object> and Map<String, Object>?
> Using JSONCollection for the value type doesn't make much sense based on
> the underlying List<Object> / LinkedHashMap<String, Object>.
> Or I do not understand what you exactly mean.
>

I believe you're right. Of course, you've gave more thought to that than I
did. :) I'm just thinking of using the most specific type possible, but in
this case Object seems to be the only option.


> Only problems I've seen at first glance:
>
> * Signature change: JSONObject get(Object name) instead of JSONObject
> get(String name)
> * Return type / Signature change: void putAll(Map<? extends String, ?
> extends Object> newProperties) instead of JSONObject putAll(Map<String, ?>
> newProperties)
>
> The second one would be a real breaking change, but IMO an acceptable one,
> because it's easy to fix, and doesn't introduce side-effects.
>

Yeah, perfect backward-compatibility is great but not a hard requirement.
And, anyway, I guess this isn't a method used in many places anyway.


>
> Hopefully, I get some work done this week. If I run into any bigger issues,
> I'll present my findings.
>
> Ben
>
>
> On Sun, Jul 26, 2020 at 9:51 PM Thiago H. de Paula Figueiredo <
> [hidden email]> wrote:
>
> > On Sun, Jul 26, 2020 at 5:41 AM Ben Weidig <[hidden email]> wrote:
> >
> > > * Stream-support
> > > * Additional instance creators
> > > * Approximate to the conceptional interfaces (Collection/Map)
> > > * Generic methods
> > > * Better/more informative exceptions
> > >
> >
> > I really love everything here, specially the idea of having better, more
> > informative exceptions.
> >
> >
> > > The general idea is making `JSONArray` more like
> `java.util.Collection`,
> > > and `JSONObject` more like `java.util.Map`, without implementing the
> > > interfaces explicitly.
> > > If we would implement them directly, it would method duplication
> > (`size()`
> > > vs. `length()`, `add()` vs `put(...)`).
> > >
> >
> > I don't mind making JSONArray implement Collection<JSONCollection> and
> > JSONObject implement Map<String,JSONCollection> (JSONCollection is the
> > JSONObject and JSONArray superclass). Actually, I like the idea. Even if
> > it's not adopted, we could have they implementing Iterable.
> >
> >
> > >
> > > Motivation
> > > ----------
> > >
> > > Tapestry's use of JSON types is deeply ingrained into its services.
> > > By using its own types, we can improve upon them to adapt to any new
> > > features.
> > > The framework and Java itself evolved, and they should be too.
> > >
> > > Java 8 was bringing a lot of new features, like lambdas, Streams, and
> > > Optionals, thereby changing the way we can deal with collection types
> in
> > a
> > > significant way.
> > >
> > > It's time to adapt tapestry-json to the new features available to us.
> > >
> >
> > I definitely agree here.
> >
> >
> > > Testing
> > > -------
> > >
> > > All added functionality should be unit tested to ensure no existing
> code
> > > will be broken by it.
> > >
> >
> > This is very, very important. Fortunately, we already have many tests in
> > tapestry-json (and in Tapestry overall) to cover backward compatibility.
> Of
> > course, new tests to cover the new functionality would be great.
> >
> > Regarding everything else, I agree. I'm looking forward to this!
> >
> >
> > >
> > >
> > >
> > >
> > >
> > >
> > > On Sat, Jul 25, 2020 at 1:12 PM Ben Weidig <[hidden email]> wrote:
> > >
> > > > Hi,
> > > >
> > > > @Chris: Where it's feasible we use Jackson, too. But sometimes it's
> > > easier
> > > > to just use a more "dumb but still JSON-compatible" type without
> > needing
> > > an
> > > > ObjectMapper.
> > > > And the first-class support of in many parts of Tapestry makes it a
> > > better
> > > > choice for smaller use-cases. So more functionality in these types
> > would
> > > be
> > > > great.
> > > >
> > > > @David: I didn't want to touch much of the existing stuff, but you're
> > > > right. The "happy path" design can be such a nuisance sometimes...
> > > >
> > > > Looks like I'm going to write a small proposal of my planned changes
> > and
> > > > additions, to have something to discuss, and to better manage scope.
> > > >
> > > > Ben
> > > >
> > >
> > >
> > >
> >
> >
> > --
> > Thiago
> >
>


--
Thiago