Add some enhancements to jsonutils.nim

[bobeff]

• Use jsonutils.nim hookable API to add a possibility to deserialize JSON arrays directly to HashSet and OrderedSet types and respectively to serialize those types to JSON arrays.

• Also add a possibility to deserialize JSON null objects to Nim option objects and respectively to serialize Nim option object to JSON object if isSome or to JSON null object if isNone.

• Move serialization/deserialization functionality for Table and OrderedTable types from jsonutils.nim to tables.nim via the hookable API.

• Add object jsonutils.Joptions and parameter from its type to jsonutils.fromJson procedure to control whether to allow deserializing JSON objects to Nim objects when the JSON has some extra or missing keys.

• Add unit tests for the added functionalities to tjsonutils.nim.

[timotheecour]

great, that was on my roadmap, thanks for tackling this.
for allowMissingFields I'd prefer to wrap it into a single (optional) ref object (eg: Joptions) for following reasons:

• I expect more options will be needed for customization
• a single ref object is easier to forward to procs rather than passing each option individually; that is true for both internal procs (eg fromJson calling fromJsonFields) but also for user code.
• a set is not flexible enough as it only allows binary flags

so I'm suggesting this API, which is easier to extend:

type Joptions* = ref object
  allowMissingFields*: bool
proc initJoptions(): Joptions = ...
proc fromJson*[T](a: var T, b: JsonNode, opt = initJoptions()) = ...

EDIT: see rest of discussions, this is what we ended up with (object instead of ref object):

type Joptions* = object
  allowMissingFields*: bool
proc initJoptions(): Joptions = ...
proc fromJson*[T](a: var T, b: JsonNode, opt = initJoptions()) = ...

[Araq]

Why not at least a var object instead? You don't take ownership of this thing.

[timotheecour]

making it object instead of ref object is ok, but why make it var? it would prevent passing the options as an rvalue (and fromJson/toJson is not modifying joptions).

here's the modified API with object instead of ref object (but no var)

type Joptions* = object
  allowMissingFields*: bool
proc initJoptions(): Joptions = ...
proc fromJson*[T](a: var T, b: JsonNode, opt = initJoptions()) = ...

[Araq]

here's the modified API with object instead of ref object (but no var)

Even better then.

[timotheecour]

one final note: if you prefer you can split your PR in 2, to move the allowMissingFields (embedded in an object) in a separate PR since the rest of this PR seems ready; up to you.

[bobeff]

I also implemented the serialization and deserialization of Option[T] types via the hookable API.

[bobeff]

Hm. I just tested that my code is not working with nested variant objects.

[timotheecour]

The version I approved was before the commit Move JSON hooks to jsonutils.nim which IMO goes in the wrong direction; compared to right before that commit, jsonutils now adds a (recursive) dependency on at least 7 modules: os, osseps, pathnorm, posix, sets, strtabs, times, which have nothing to do with serialization.

In particular, os, posix and times are not lightweight dependencies.

This negates the whole point of the json hooks I had introduced in jsonutils (#14563), and prevents using jsonFrom, jsonTo in those modules due to cyclic dependency issues. Having toJsonHook/fromHsonHook in collections is arguably better, it just adds 2 symbols to collections but doesn't cause extra dependencies or prevent use of jsonutils in those modules.

See also #15133 (comment) above and #14549 (comment), it looks like we're going in circles...

json should be light and unrelated to any OS specific stuff, but is now a kitchen sink of dependencies; here's the total imports for import json: [...]

[c-blake]

It sounds to me like @Araq & @timotheecour have conflicting visions of future directions here leading to said circularity. So, again there should be a How-To-Marshal-in-Nim RFC to get more exposure and again concrete use cases would help clarify these conflicting theoretical concerns.

[timotheecour]

I'm not against an RFC but I thought the conclusion had already been reached that serialization should not import "the world":

#14549 (comment)

@Araq: Sure, my major gripe is a json.nim that imports all the stdlib collections, there should be a better way. At least it's better than everything depending on json though...

#14549 (comment)

we have dead code elimination anyway, so I don't see it being that bad

it still needs to be parsed and is not cheap, eg look at this: #14179
simply adding dependency on os (without using any symbols) yields a 30x slowdown in that case
and it can lead to cyclic dependency issues

Adding (recursive) dependency on os+times+the other modules (see above) IMO makes this PR non-mergeable; it was perfectly fine before the commit Move JSON hooks to jsonutils.nim

[c-blake]

Well, I read #14549 (comment) as @PMunch and @Araq both also wanting an RFC about the broader issues.

[Clyybber]

@timotheecour None of these modules import jsonutils, which recursive/cyclic dependencies are you talking about?

In particular, os, posix and times are not lightweight dependencies.

posix and times are imported by os which is imported by strtabs (the fact that strtabs imports os is more of a strtabs "issue" rather than an issue that concerns this PR).
Nothing in this PR explicitly imports any of the modules you listed.

[bobeff]

I think that it is not so important whether all collections will depend on JSON or there will be a module dedicated to collections serialization importing all collections or will be separate modules for JSON serialization for each collection. Everything I want is to have capabilities to serialize/de-serialize collections and to allow missing/extra keys. This functionality was already started in the way proposed by Timothee in strtabs.nim. I implemented it following his design and then changed it in the way suggested by Andreas.

Feel free to revert the e61da21 if you again decide the other way, but I find the current situation with two separate APIs

• one in json.nim
• another in jsonutils.nim which is extended in this pull request.

both of them not complete to be worse than not an ideal solution or even a completely missing solution.

[timotheecour]

@Clyybber the problem is two-fold; after the commit Move JSON hooks to jsonutils.nim, you can't have jsonTo, jsonFrom without importing all those recursive dependencies (os, times etc), even if you don't care about strtabs or don't need to serialize a StringTableRef in your code; even simply parsing modules is not cheap, see #14179 which caused a 30x slowdown by adding os dependency and later got fixed.

None of these modules import jsonutils, which recursive/cyclic dependencies are you talking about?

that commit doesn't create a cyclic dependency (that wouldn't even be possible) but prevents future imports which would create cyclic dependencies.

these problems were nonexistant before this commit.

[bobeff]

For a comparison, take a look at https://en.cppreference.com/w/cpp/container/unordered_map . No reference to json in sight, a much better design.

@Araq, @timotheecour This is not a completely fair comparison because even today there is no notion for JSON in the entire C++ standard library. But from the other side, the most modern languages which have such support like C# for example have dedicated modules for serialization. Maybe simply the too generic name jsonuitls.nim have to be changed to something like jsonserialization.nim?

if you don't care about strtabs or don't need to serialize a StringTableRef in your code; even simply parsing modules is not cheap

The other variant which could be acceptable for both of you as it was mentioned before is to have a separate module for JSON serialization for each collection type like strtabsjsonserialization.nim, tablesjsonserialization.nim, setsjsonserialization.nim, optionsjsonserialization.nim and so on? I think that this will be a nice compromise, between the two points of view.

[timotheecour]

The other variant which could be acceptable for both of you

as I mentioned earlier I would be totally ok with that; modules are cheap.

[bobeff]

@Araq @timotheecour If Araq also approves it I will make it that way.

[timotheecour]

how about:
strtabsjsonserialization.nim => strtabs_json.nim (etc) ? non ambiguous and shorter

[bobeff]

@timotheecour Ok but let us wait for Andreas to approve the design and the names of the modules because I already feel a little bored from this issue and I hope this to be the last time I have to change the things.

[zah]

FWIW, this approach with a separate "bridge" module (e.g. strtabs_json) is what I use in nim-json-serialization as well:
https://github.com/status-im/nim-json-serialization/blob/master/json_serialization/std/sets.nim

I've made it a bit easier for the users to import a single module by re-exporting the the bridged modules from there:

import json_serialization/std/tables

var t = initTable[string, int]()

[c-blake]

As mentioned before, using a more general term ("marshal" since "serialization" is potentially confusing and different from the current "marshal" module: lose-lose) instead of "json" might be more forward looking..that is if this module hook design can support other marshalled formats (e.g. some 10+x faster binary one someday). If it can only support json for whatever reasons then "json" is fine, though that seems like an unfortunately limited design for the stdlib.

[bobeff]

@c-blake Except if we won't also have separate modules for separate marshaling formats e.g. tables_json.nim, tables_xml.nim, tables_binary.nim and so on. Here we have the same problem with the multiple imports and the compilation speed. If I want to marshal only to JSON or only to some binary format why I have to import the modules for all other formats?

[c-blake]

Ok. In that case I would recommend a subdirectory json/ (somewhere..maybe std/marshal/json if that won't conflict with std/marshal.nim?) to be followed (someday, possibly) by std/marshal/xml/ std/marshal/binary/, etc. Sounds like there are at least 4 stdlib names under such dirs (sets, tables, strtabs, options) so far. Maybe more could happen.

[bobeff]

Ok. I find the structure proposed by c-blake to be appropriate, but I also think that the current generic functionality in jsonutils.nim also have to be moved somewhere in this structure under some name like std/marshal/json/json.nim for example and jsonutils.nim to be deprecated. It will be good the old serialization API in json.nim also to be deprecated to not confuse the people what to use.

[c-blake]

Co-locating the jsonutils-equivalent near the fork for type-special-purpose modules sounds reasonable to me. Let's see if @Araq likes this new direction. Don't know about deprecating json.nim or what to do about the marshal.nim module in light of all this. Those also seem like questions for @Araq.

[bobeff]

Don't know about deprecating json.nim

To be clear I don't mean deprecation of the entire json.nim, but only the part related to serialization/de-serialization of different standard data types. The part dedicated to working with JsonNodes should stay as is.

[Araq]

I'm writing an RFC that describes in detail how this feature should be designed. Once we agreed on the RFC, we can implement it and deprecate what doesn't fit.

[Araq]

RFC is here, nim-lang/RFCs#247

[bobeff]

As this is a working backward compatible solution and currently I don't have time to reimplement it according to the RFC, is there any chance to merge this pull request as a temporary solution until the RFC is implemented?

[Araq]

Ok, will review it once again.