`doNotSerialize`, `jsonName` pragmas for JSON serialization closes #8104, #10718, also fixes #11415 #11416

Vindaar · 2019-06-05T23:39:23Z

This PR adds the ability to exclude certain object fields from JSON serialization via %, by annotating the field with the {.noserialize.} pragma.

type
  User = object
    name: string
    age: int
    uid {.noserialize.}: int
let user = User(name: "Siri", age: 7, uid: 1234)
let uJson = % user
doAssert not uJson.hasKey("uid")

After discussion on IRC:
https://irclogs.nim-lang.org/05-06-2019.html#19:15:24
@dom96 asked me to create a PR for this.

While working on it I stumbled upon an issue with custom pragmas on variant objects: #11415.

However, to not break things, I also had to extend how typDef is determined in customPragmaNode (see 5da931f). This is a problem for the first test case in tjsonmacro.nim:
https://github.com/nim-lang/Nim/blob/devel/tests/stdlib/tjsonmacro.nim#L9-L45
Line 45 will fail with node is not a symbol. I couldn't really reproduce this without the % proc change, which is why I didn't open an issue for this as well.

Update: add `jsonName`

I also added a jsonName pragma, to customize the (de)serialization name of an object field.

type
  User = object
    name: string
    age {.jsonName: "userAge".}: int
let user = User(name: "Siri", age: 7)
let uJson = % user
doAssert uJson["userAge"].num == 7

This pragma closes #10718.

dom96

We might wish to do some bikeshedding on the name. noSerialize is fine, but I wonder if we should include "JSON" in the name and maybe come up with a more generic format for this.

Maybe {.json: NoExport.}. That would also allow us to do things like: {.json: "CustomFieldName".} in the future and keep consistency. Of course we can stick with noSerialize for now and deprecate it in the future for something more generic.

dom96 · 2019-06-08T23:11:57Z

lib/pure/json.nim

 proc `%`*[T: object](o: T): JsonNode =
  ## Construct JsonNode from tuples and objects.
+  ## An object field annotated with the `{.noserialize.}` pragma will not appear


Suggested change

## An object field annotated with the `{.noserialize.}` pragma will not appear

##

## An object field annotated with the `{.noserialize.}` pragma will not appear

RST rendering will not put a line break unless you include an empty line like I have done above

Oh, thanks! I'm not very experienced with RST.

dom96 · 2019-06-08T23:12:22Z

lib/pure/json.nim

@@ -363,10 +392,25 @@ proc `[]=`*(obj: JsonNode, key: string, val: JsonNode) {.inline.} =
  assert(obj.kind == JObject)
  obj.fields[key] = val

+template noserialize*() {.pragma.}


If possible please add documentation to this as well.

dom96 · 2019-06-08T23:13:02Z

lib/pure/json.nim

+## .. code-block:: nim
+##   import json
+##   let number = 10
+##   let numJson = % number


Nit: no space after %. Please change this below too.

dom96 · 2019-06-08T23:13:26Z

lib/pure/json.nim

+##
+## .. code-block:: nim
+##   import json
+##   let number = 10


I would simplify this and remove this variable, simply use the literal below.

dom96 · 2019-06-08T23:14:39Z

lib/pure/json.nim

+##   doAssert numJson.kind == JInt
+##
+## For objects it may be desirable to avoid serialization of one or more object
+## fields. This can be achieved by using the ``{.noserialize.}`` pragma.


Let's use camelCase here (and everywhere): {.noSerialize.}

Vindaar · 2019-06-11T13:09:53Z

Sorry, I overlooked your comment. I'll address the comments later today.

I have no strong feelings about the pragma name. Just picked the first thing that came to mind. Although I might actually prefer the {.json: WhatEver.} syntax.

alehander92 · 2019-06-11T17:26:51Z

keep in mind i can have a "NoExport" field in my json, and this might make a bit confusing json: NoExport: yes, i know it doesnt have " but still adds a moment of "let me think about it while i read it

dom96 · 2019-06-11T20:41:46Z

@alehander42 got any better suggestions?

Vindaar · 2019-06-11T20:41:54Z

Ok, I addressed the comments. For now I only changed the name to camelCase instead of changing it completely. I don't want to be the person making that decision.

disruptek · 2019-06-12T17:21:25Z

I like noSerialize because it's a notation that transcends implementation. JSON is but one of many possible serialization formats and by abstracting the concept, neither the code nor the programmer needs to change to adapt to new formats.

metagn · 2019-07-22T12:43:31Z

Don't mean to make things more complicated, just adding to the discussion:

If {.json: "CustomFieldName".} were to be implemented, you could also implement multiple aliases for the same field, as in {.json: ["CustomFieldName1", "CustomFieldName2"].}. You could even support things like field {.json: ["UnparsedInt": string].}: int or field {.json: string.}: int to convert types during marshalling. This would allow for either {.json: [].} or {.json: void.} to mean no serialization at all, but it looks kind of ugly.

If you still want to just use a name, you could still do something like {.json: noserialize.} and have other libraries do {.otherformat: noserialize.} as well. The transient keyword in java is also an option for a name as it serves the same purpose.

zah · 2019-07-22T13:45:55Z

In our work-in-progress generic serialization library, I've used a pragma called dontSerialize. That's longer than noSerialize, but it's also more proper grammar :P

https://github.com/status-im/nim-serialization/

dom96 · 2019-07-23T22:17:58Z

@zah doNotSerialize would be better :P

Vindaar · 2019-08-08T12:32:22Z

I renamed noSerialize to doNotSerialize and rebased on the current devel.

Adding the `{.noserialize.}` pragma to an object field means the given field will be ignored when serialized via the `%` proc.

I'm a little puzzled about this. With the change introduced in the previous commit to add the `noserialize` option for JSON, the first example in the `tjsonmacro.nim` does not work anymore. The generic `Point[float]` cannot be serialized, because of a `node is not a symbol` error. In order to get the type def implementation under the given case, we have to also get the implementation of the underlying type. Maybe this should be fixed in a different way?

Addresses the other comments of the PR too.

Vindaar · 2019-09-03T13:26:49Z

Due to @Phillips126 comment in #10718, I saw Araq's comment about adding a jsonName pragma to customize the (de)serialization names.

Since this PR is related anyways I thought I'd attempt to add this as well. Due to the addition of the jsonName pragma, the naming of the doNotSerialize pragma is probably open for bikeshedding again. :P (or if it should just be the result of {.jsonName: "".} instead of having a different pragma for no serialization.

The implementation isn't very pretty unfortunately for the deserialization, because the whole deserialization logic works on the type node obtained via getTypeImpl. The custom pragma information however is only available when calling getImpl on the type symbol. So I ended up implementing this feature by fixing up the code after the macro is done.

edit: Damn, it seems this implementation is too broken right now. I realized that it'd fail e.g. for seq[<object with jsonName>], fixed that to find table didn't work. After each fix I introduced more problems. I might rethink the whole thing again by fixing the actual to implementation after all.
However, the main problem I see is that it depends on the type of an nnkSym as to how one can extract the pragma information. That's a pain.
edit2: Seems like I managed to make it work for all cases I can think of. Will clean it up later and push.

Vindaar · 2019-10-17T08:36:04Z

With #12391 now merged, I'll close this PR and rewrite it. Will probably create a new PR once everything is working.

disruptek · 2020-04-19T16:17:19Z

Any thoughts about a new version of this PR?

Vindaar · 2020-04-20T07:43:49Z

@disruptek Sorry, I kinda forgot about it.

I'll try to take a stab at it in the next few days!

Vindaar · 2020-04-25T08:48:01Z

I just remembered why I put this on ice. It's because I was waiting for #11526 to be merged.

I just started to reimplement this. doNotSerialize is essentially the same as in this PR. But the jsonName part doesn't seem to be possible right now (without making some changes to the to procedures), because in the current logic I just can't seem to find a way to extract the pragma information from the types in foldObjectBody.
I'd have to extract the type information from the typedesc in to directly. This would then to require to pass that information to initFromJson, which is ugly. Would have to change the signature either of all initFromJson procs (which is bad, because only one needs the replacement information) or only for the object | tuple proc, but then I'd have to make sure to call the correct proc within the macros depending on the symbols type. Makes for ugly code.

So, I'll wait for a bit until the above PR is hopefully merged soon.

brentp · 2020-07-13T18:08:55Z

bump. this would be quite useful.

ghost · 2020-07-13T18:52:40Z

@Vindaar Well yes, you need to modify the .to procedure, you can check my custom implementation which is based on that PR - https://github.com/Yardanico/telenim/blob/master/src/telenim/json_custom.nim (called toCustom here)

Vindaar · 2020-07-13T18:55:18Z

@Yardanico Uhm, if you already have a working version of this around, feel free to create a PR for this!
If you don't feel like doing that, I'll take a look at it. I'm still not sure though about the custom pragmas implementation / would rather wait until that PR is merged.

ghost · 2020-07-13T19:28:40Z

@Vindaar yeah, we should want until #11526 is merged because I just copied needed parts of it into my json_custom

Clyybber · 2021-04-15T12:22:33Z

#11526 is now merged :)

Vindaar changed the title ~~allow excluding object fields from JSON serialization #8104, also fixes #11415~~ allow excluding object fields from JSON serialization closes #8104, also fixes #11415 Jun 5, 2019

dom96 reviewed Jun 8, 2019

View reviewed changes

Vindaar mentioned this pull request Aug 6, 2019

Fixes handling of nkBracketExpr nodes in opcGetImpl. #11879

Closed

Vindaar force-pushed the jsonNoSerialize branch from 763ad2a to 012a217 Compare August 8, 2019 12:31

narimiran assigned dom96 Aug 21, 2019

Vindaar changed the title ~~allow excluding object fields from JSON serialization closes #8104, also fixes #11415~~ doNotSerialize, jsonName pragmas for JSON serialization closes #8104, #10718, also fixes #11415 Sep 3, 2019

Vindaar and others added 9 commits September 3, 2019 15:21

fixes nim-lang#11415 by checking all children of recList

e34cd17

add test case for nim-lang#11415

2e0396c

add noserialize pragma to use with % for objects

446c71d

Adding the `{.noserialize.}` pragma to an object field means the given field will be ignored when serialized via the `%` proc.

remove echo of uJson in test case

026f307

fix access of uid as string instead of ident...

599f3d8

add docstring to noSerialize template, use camelCase for pragma

ad65b02

Addresses the other comments of the PR too.

rename noSerialize to doNotSerialize

ea0e71f

add jsonName pragma to (de)serialize under custom name, add tests

7f56c77

Vindaar force-pushed the jsonNoSerialize branch from cbb1c45 to 7f56c77 Compare September 3, 2019 13:22

Vindaar closed this Oct 17, 2019

tandy-1000 mentioned this pull request Jun 20, 2021

Allow camelCase -> snake_case when using toJson treeform/jsony#18

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`doNotSerialize`, `jsonName` pragmas for JSON serialization closes #8104, #10718, also fixes #11415 #11416

`doNotSerialize`, `jsonName` pragmas for JSON serialization closes #8104, #10718, also fixes #11415 #11416

Vindaar commented Jun 5, 2019 •

edited

Loading

dom96 left a comment

dom96 Jun 8, 2019

Vindaar Jun 11, 2019

dom96 Jun 8, 2019

Vindaar Jun 11, 2019

dom96 Jun 8, 2019

dom96 Jun 8, 2019

dom96 Jun 8, 2019

Vindaar commented Jun 11, 2019

alehander92 commented Jun 11, 2019

dom96 commented Jun 11, 2019

Vindaar commented Jun 11, 2019

disruptek commented Jun 12, 2019

metagn commented Jul 22, 2019

zah commented Jul 22, 2019 •

edited

Loading

dom96 commented Jul 23, 2019

Vindaar commented Aug 8, 2019

Vindaar commented Sep 3, 2019 •

edited

Loading

Vindaar commented Oct 17, 2019

disruptek commented Apr 19, 2020

Vindaar commented Apr 20, 2020

Vindaar commented Apr 25, 2020

brentp commented Jul 13, 2020

ghost commented Jul 13, 2020 •

edited by ghost

Loading

Vindaar commented Jul 13, 2020

ghost commented Jul 13, 2020

Clyybber commented Apr 15, 2021

	## An object field annotated with the `{.noserialize.}` pragma will not appear
	##
	## An object field annotated with the `{.noserialize.}` pragma will not appear

doNotSerialize, jsonName pragmas for JSON serialization closes #8104, #10718, also fixes #11415 #11416

doNotSerialize, jsonName pragmas for JSON serialization closes #8104, #10718, also fixes #11415 #11416

Conversation

Vindaar commented Jun 5, 2019 • edited Loading

Update: add jsonName

dom96 left a comment

Choose a reason for hiding this comment

dom96 Jun 8, 2019

Choose a reason for hiding this comment

Vindaar Jun 11, 2019

Choose a reason for hiding this comment

dom96 Jun 8, 2019

Choose a reason for hiding this comment

Vindaar Jun 11, 2019

Choose a reason for hiding this comment

dom96 Jun 8, 2019

Choose a reason for hiding this comment

dom96 Jun 8, 2019

Choose a reason for hiding this comment

dom96 Jun 8, 2019

Choose a reason for hiding this comment

Vindaar commented Jun 11, 2019

alehander92 commented Jun 11, 2019

dom96 commented Jun 11, 2019

Vindaar commented Jun 11, 2019

disruptek commented Jun 12, 2019

metagn commented Jul 22, 2019

zah commented Jul 22, 2019 • edited Loading

dom96 commented Jul 23, 2019

Vindaar commented Aug 8, 2019

Vindaar commented Sep 3, 2019 • edited Loading

Vindaar commented Oct 17, 2019

disruptek commented Apr 19, 2020

Vindaar commented Apr 20, 2020

Vindaar commented Apr 25, 2020

brentp commented Jul 13, 2020

ghost commented Jul 13, 2020 • edited by ghost Loading

Vindaar commented Jul 13, 2020

ghost commented Jul 13, 2020

Clyybber commented Apr 15, 2021

`doNotSerialize`, `jsonName` pragmas for JSON serialization closes #8104, #10718, also fixes #11415 #11416

`doNotSerialize`, `jsonName` pragmas for JSON serialization closes #8104, #10718, also fixes #11415 #11416

Vindaar commented Jun 5, 2019 •

edited

Loading

Update: add `jsonName`

zah commented Jul 22, 2019 •

edited

Loading

Vindaar commented Sep 3, 2019 •

edited

Loading

ghost commented Jul 13, 2020 •

edited by ghost

Loading