std/jsonutils should not fail if key is missing for Option[T]

nim-lang / Nim

Nim is a statically typed compiled systems programming language. It combines successful concepts from mature languages like Python, Ada and Modula. Its design focuses on efficiency, expressiveness, and elegance (in that order of priority).

https://nim-lang.org

Other

16.58k stars 1.47k forks source link

std/jsonutils should not fail if key is missing for Option[T] #18151

Open al6x opened 3 years ago

al6x commented 3 years ago

JSON should not fail if key is missing for Option[T] field.

Currently it fails, see example. It should work, as having missing key for Option is not just fine JSON, but is even more idiomatic JS/JSON as using explicit key key: null.

Example

import json, std/jsonutils, options

type Some* = object
  v*: Option[int]

echo "{}".parse_json.json_to(Some)

Current Output

/usercode/in.nim(6)      in
/playground/nim/lib/std/jsonutils.nim(250) jsonTo
/playground/nim/lib/std/jsonutils.nim(131) fromJson
/playground/nim/lib/std/jsonutils.nim(98) checkJsonImpl
Error: unhandled exception: ("Some", "v", {}) [ValueError]

Possible Solution

It's possible to use allowMissingKeys to handle it, but it's really wrong.

First - because as I already mentioned, using key: null is non standard practice in JS/JSON usually the whole field is missing. Also, best JS practices advise to never use null in JS, and use undefined instead, to avoid undefined/null confusion, so situation with explicit field with null is more like an exception than the common case, in common case the field would be missing. Also, null fields are discouraged because it adds more noise and increase JSON size.

Second, more serious problem, is that with setting allowMissingKeys to true, it's possible to miss serious bugs, as say you have object[price: int] but because you use allowMissingKeys you won't be able to detect wrong JSON with price missing, and it would parse it correctly and set price as 0.

> nim -v
Nim Compiler Version 1.4.6 [MacOSX: amd64]
Compiled at 2021-05-06
Copyright (c) 2006-2020 by Andreas Rumpf

active boot switches: -d:release

timotheecour commented 3 years ago

please update your issue to follow template, ie show the current error, the nim version+hash at which this was tested; this stuff matters

Also this code would fail in the current playground without the json_to, maybe it's worth to add this function to the jsonutils

i don't understand this part, this works in devel 0de3d4292f328f94c7a94af7e3e61e58ff43a252:

import json, std/jsonutils, options
type Some* = object
v*: Option[int]
# proc json_to*(json: JsonNode, T: typedesc, options: Joptions): T =
#   from_json(result, json, options)
echo "{}".parse_json.json_to(Some, Joptions(allowMissingKeys: true))

(please always check on devel, don't use nim-playground as substitute, unless it's always up to date)

JSON should not fail if key is missing for Option[T] field.

but why single out Option[T], what about ref T, ptr T? wouldn't the same argument hold?
and what about serialization, Some() serialize as {} or {v: null} ?
and what about non-fields, eg @[Some(), Some()], would that allow skipping nulls?

I'd be ok with adding a flag to skip null fields, eg:

Joptions = object
  # existing flags...
  allowMissingNullFields: bool # allow missing keys for JObject's when the corresponding type is pointer-like or Option[T]

ToJsonOptions = object
  enumMode*: EnumMode
  jsonNodeMode*: JsonNodeMode
  skipNullFields: bool # filter out keys with null and Option.None values for JObject's

al6x commented 3 years ago

Updated the ticket.

i don't understand this part, this works in devel

My bad, I checked on latest only, not on devel, please ignore that part.

but why single out Option[T], what about ref T, ptr T? wouldn't the same argument hold?

As far as I understand ref T is more like memory layout optimisation thing, and while it can be nil it's not supposed to be used that way. I consider ability to set ref T as nil as bad design decision that going to be deprecated or removed in the future (maybe I'm wrong and Nim core team has different opinion on that issue). So, in my understanding ref T should behave same way as T, if you want it to be optional it should be explicitly specified, same way, as Option[ref T].

As for ptr T - I never used pointers in Nim, and don't know much about it, can't comment.

and what about serialization, Some() serialize as {} or {v: null} ? and what about non-fields, eg @[Some(), Some()], would that allow skipping nulls?

Yea, these are shady :). For @[Some(), Some()] indeed you can't skip the nulls.

I'd be ok with adding a flag to skip null fields, eg: skipNull (ditto for serialization/deserialization options)

Looks like a reasonable solution to me.

timotheecour commented 3 years ago

As far as I understand ref T is more like memory layout optimisation thing, and while it can be nil it's not supposed to be used that way.

I disgree, I dont' see how things like this could work without nil ref:

var a: seq[ref T]
a.setLen 10 # user pre-allocates, then fills it

nim compiler, stdlib and 3rd party code make extensive use of nil references, I don't see that going away, and for good reasons, they're useful ; but that really shouldn't be discussed here, feel free to open an RFC for that.

Looks like a reasonable solution to me.

I've updated the proposal in https://github.com/nim-lang/Nim/issues/18151#issuecomment-853160159

al6x commented 3 years ago

I've updated the proposal in #18151 (comment)

+1. I personally would prefer it to be the default mode, but not insisting, other people may have different use cases and preferences.

al6x commented 3 years ago

and what about serialization, Some() serialize as {} or {v: null} ?

@timotheecour I recently worked with this exact case :). I'm writing Nim API for some plotting library. One example - plot a table, as on the image below:

This is what Nim DSL to bulid such table looks like:

plot "/promising_countries.json", table, jo {
  wsort:          { economy_score: 1.6, gov_spending: -1.0, age_0_14: 1.0 },
  column_filters: { economy_score: [">", 1.5] },
  columns:        [
    { id: "country",       type: "string" },
    { id: "gov_spending",  type: "number", format: { type: "line", ticks: [50, 100] } },
    { id: "economy_score", type: "number", format: { type: "line"} },
    { id: "age_0_14",      type: "number", format: { type: "line", ticks: [50, 100] } }
  ]
}

And this is what kind of JSON Nim generates from that DSL. Because instead of omiting optional fields it sets every such field as nil. This is an issue, because it's not just some binary-machine-only-format, but sometimes you want to check this generated JSON and see how it looks like.

I think there should at least be an option telling Nim JSON writer to omit optional fields (I personally would prefer that it should be the default behavior, but at the very least there shoudl be an option).

{
  "columns": [
    {
      "id": "country",
      "type": "string",
      "title": null,
      "format": null,
      "width": null,
      "min_max": null
    },
    {
      "id": "gov_spending",
      "type": "number",
      "title": null,
      "format": {
        "align": null,
        "small": null,
        "type": "line",
        "max": null,
        "color": null,
        "ticks": [
          50.0,
          100.0
        ],
        "domain": null,
        "scale": null,
        "log_unit": null
      },
      "width": null,
      "min_max": null
    },
    {
      "id": "economy_score",
      "type": "number",
      "title": null,
      "format": {
        "align": null,
        "small": null,
        "type": "line",
        "max": null,
        "color": null,
        "ticks": null,
        "domain": null,
        "scale": null,
        "log_unit": null
      },
      "width": null,
      "min_max": null
    },
    {
      "id": "age_0_14",
      "type": "number",
      "title": null,
      "format": {
        "align": null,
        "small": null,
        "type": "line",
        "max": null,
        "color": null,
        "ticks": [
          50.0,
          100.0
        ],
        "domain": null,
        "scale": null,
        "log_unit": null
      },
      "width": null,
      "min_max": null
    }
  ],
  "alter_columns": null,
  "title": null,
  "sort": null,
  "wsort": {
    "age_0_14": 1.0,
    "economy_score": 1.6,
    "gov_spending": -1.0
  },
  "filter": null,
  "column_filters": {
    "economy_score": [
      ">",
      "1.5"
    ]
  },
  "id": null,
  "selectable": null,
  "sortable": null,
  "toolbar": null,
  "warnings": null,
}

timotheecour commented 3 years ago

@al6x PR welcome for adding skipNullFields as described in https://github.com/nim-lang/Nim/issues/18151#issuecomment-853160159