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 <david.tay...@extensiatech.com> 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 <b...@netzgut.net> 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: dev-unsubscr...@tapestry.apache.org > For additional commands, e-mail: dev-h...@tapestry.apache.org > >
