JSON::Serializable

[kostya]

extracted from: https://gist.github.com/asterite/db0ee7947d7eceb17c894fabffffafc4

added extra fields working: 1bb4ffc

specs pass local when i comment lines if compare_versions(Crystal::VERSION, "0.25.0") >= 0

btw formatter just crashed: ./bin/crystal tool format spec/std/json/serializable_spec.cr

expecting ], not `::, `, at :41:9 (Exception)
  from src/compiler/crystal/tools/formatter.cr:4690:9 in 'check'

and i bad at writing docs.

i also extract strict and extra to modules, as it common behaviour: 824c293

[asterite]

Awesome! Can you put the whole setializable spec inside a compare_versions? Annotations don't parse well right now.

I'll take a look at the formatter issue soon, it should be simple to fix.

And let's do the same for YAML (it can be in this PR or someone else can send it).

I'll add the next version of the compiler to travis so we can run that code inside compare_versions.

[RX14]

We already run the spec suite once with a compiled compiler @asterite - we just don't have anyway way of detecting that the compiler is compiled.

[asterite]

@RX14 What I mean is that I just added CRYSTAL_CONFIG_VERSION=0.25.0 to travis and circle. With that, when the compiler is built, it will have inside it that version, and code inside compare_versions will now be compiled and run. So std specs will run against new features.

Does that make sense?

I just enabled it, but it still fails because the specs should also be behind a compare_versions. That whole spec file must be put inside {% if compare_versions(...) %} {% end %}.

[RX14]

@asterite it makes sense but I don't like it :)

I'd rather just detect if the version has the git suffix or something.

[asterite]

What git suffix? Where?

[kostya]

i also find this strange compilation bug:

require "json"

module JSONAttrModule
  property moo : Int32 = 10
end

class JSONAttrModuleTest
  include JSONAttrModule
  include JSON::Serializable

  @[JSON::Field(key: "phoo")]
  property foo = 15

  def initialize; end
end

class JSONAttrModuleTest2 < JSONAttrModuleTest
  property bar : Int32 
  def initialize(@bar : Int32)
  end
end

p JSONAttrModuleTest2.new(1)
p JSONAttrModuleTest2.from_json(%<{"bar":1}>)

Error in 1.cr:24: instantiating 'JSONAttrModuleTest2:Class#from_json(String)'

p JSONAttrModuleTest2.from_json(%<{"bar":1}>)
                      ^~~~~~~~~

in src/json/from_json.cr:13: no overload matches 'JSONAttrModuleTest2.new' with type JSON::PullParser
Overloads are:
 - JSONAttrModuleTest2.new(bar : Int32)

  new parser
  ^~~

[asterite]

@kostya When you inherit a type and define a new initialize, you must copy all the initialize from the parent type. In this case, you must do:

class JSONAttrModuleTest2 < JSONAttrModuleTest
  property bar : Int32 
  def initialize(@bar : Int32)
  end

  def initialize(pull : JSON::PullParser)
    {% @type %} # This is a hack that's needed, not sure if it's needed here, though
    super
  end
end

[kostya]

is it bug, or why it cant work without redefine?

[asterite]

It's similar to this:

class Foo
  def initialize(@x : Int32)
  end
end

class Bar < Foo
  # This initialize hides all initializes from superclasses, like in Java and C#,
  # because at this point we can't be sure all instance vars from Bar will be
  # initialized in Foo's initialize.
  def initialize(x, y)
    super(x)
  end
end

Bar.new(1)

So we must add an intialize(x) that calls super to make it work.

[asterite]

It's not a bug, it's how the language has always worked, I explained it a bit in the last comment.

The only "bug" is having to use that {% @type %} hack to make it work. I know why that works, but I don't know how to fix it to not require that hack. That part of the compiler has become too complex for me and I can't understand it anymore.

[kostya]

what do you think about this options: d5b218e

[asterite]

@kostya You can't use skip_file. The whole file can't be parsed right now because @[JSON::Field] didn't parse before. You have to do:

{% if compare_versoins(...) %}
  # put all the current code here
{% end %}

[asterite]

Strange:

in src/json.cr:97: Not a semantic version: "0.24.2+?"
{% if compare_versions(Crystal::VERSION, "0.25.0") >= 0 %}
                       ^~~~~~~~~~~~~~~~

What's that "0.24.2+?"...

[RX14]

@asterite the crystal version code calls out to git to find out how many commits it is ahead of the 0.24.2 tag. The answer here was clearly ?.

[asterite]

Maybe it's better then to implement this after we release 0.25.0.

[RX14]

@asterite {% if Crystal::VERSION.includes?("0.24.2+") %} should work.

[asterite]

Maybe just {% if Crystal::VERSION == "0.25.0" %} too (we want to run code only when we are that version, not sure why you compare to "0.24.2+")

[RX14]

@asterite no, the version of crystal on the ci isn't 0.25.0 because 0.25.0 hasn't been released yet. We must use 0.24.2+ because the version string of all versions of the compiler built from git after 0.24.2 was tagged in git start with 0.24.2+.

[asterite]

@RX14 I just added CRYSTAL_VERSION_CONFIG=0.25.0 in travis and CI. When you do make crystal, the new compiler will have version "0.25.0". The current compiler's version is "0.24.2+" (or something like that). With that, we can compiler/run code targetted for the next version, in CI, and it will also work when actually releasing it.

Or am I missing something?

[RX14]

@asterite There's nothing wrong with that technically, I'm arguing against it semantically (we already have a solution by testing with 0.24.2+, why not use it). After we release we'll have to manually update the CI to compiler version 0.25.0, then we can un-macro the specs to always run.

[asterite]

@RX14 Could you write a snippet on how you think we can use what you say? I can't imagine it. That is, please write a small diff of this PR using what you suggest.

[asterite]

(and of course, that also has to work on our machine... in my machine the version is "0.24.2")

[RX14]

@asterite compilers that don't support annotations are 0.24.2, compilers that do support annotations are either 0.24.2+or 0.25.0. Those are my assumptions. I'm sure you can imagine the diff from that.

[asterite]

Ah, OK. Sounds good then.

[RX14]

Oops, I mean VERSION.starts_with?("0.24.2+") || VERSION == "0.25.0" for when to enable these specs/code. Might as well use starts_with? for 0.25.0 too.

[asterite]

|| Crystal::VERSION == "0.25.0"

[RX14]

Actually please use || Crystal::VERSION.includes?("0.25.0").

[asterite]

|| Crystal::VERSION == "0.25.0"

[RX14]

ditto

[kostya]

somehow this is locally crashed:

./bin/crystal spec/std/json/serializable_spec.cr
Using compiled compiler at `.build/crystal'
Error in spec/std/json/serializable_spec.cr:1: expanding macro

{% if Crystal::VERSION.includes?("0.24.2+") || Crystal::VERSION == "0.25.0" %}
^

in spec/std/json/serializable_spec.cr:655: undefined constant JSONAttrWithQueryAttributes

        {% methods = JSONAttrWithQueryAttributes.methods %}
                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~

and when remove conditions works.

[asterite]

Now you are inside the {% if macro, you must use \{% and \{{ inside all of that, so that it expands later.

[kostya]

my english is bad, i cant write normal docs :)

[asterite]

@kostya That's a nice idea, but the names could conflict with other method names, like id, etc. So using [:root] is good for now.

About knowing the options, it can be documented. We can improve all of this later, if needed.

[asterite]

@kostya That's a nice idea, but [:root] is safer because it can't conflict with other methods on AST nodes, such as id. And about what can be used in an annotation, it can be documented for now. Plus annotations can also have positional arguments.

[asterite]

@RX14 I wouldn't mind shipping this without documentation. I guess no one except kostya tried this PR, so all of this can be experimental for now. If all works well, we can document it for 0.25.1 and also fix bugs that might appear, and improve things we find along the way.

[straight-shoota]

If this is merged and goes into 0.25.0, it should be documented. Someone else can jump in if @kostya can't do that. I didn't have time to look at it, yet

If no-one else has actually tried this, it shouldn't be merged. I don't think there is a rush. If it's merged after 0.25.0 is released, that's not a big deal. It even allows us to move entirely to the annotation based serialization without needing support for 0.24.1.

[kostya]

i added docs for json, if its ok, i copy it to yaml.

[RX14]

I'd rather not refer to this as a replacement for JSON.mapping, since JSON.mapping will probably go away before 1.0 if this works out. Better to provide a full description of what this code attempts to do, which is provide automatic serialization of the object to/from json.

[RX14]

this is probably a perfect place to show off @[JSON::Field(key: "lat")] and rename this property latitiude. Same for longitude.

[RX14]

How about

Including JSON::Serializable will create #to_json and self.from_json methods on the current class, and a constructor which takes a JSON::PullParser. By default, these methods serialize into a json object containing the value of every instance variable, the keys being the instance variable name. Most primitives and collections are supported (string, integer, array, hash, etc.), along with objects which define to_json and a constructor taking a JSON::PullParser. Union types are also supported, including unions with nil. If multiple types in a union parse correctly, it is undefined which one will be chosen.

[RX14]

To change how individual instance variables are parsed and serialized, the annotation JSON::Field can be placed on the instance variable. Annotating property, getter and setter macros is also allowed.

[RX14]

JSON::Field properties

[RX14]

the value of the key in the json object (by default the name of the instance variable)

[RX14]

respects

[RX14]

Remove this paragraph

[RX14]

If the module ... is included, ...

[RX14]

ditto

will be stored in a Hash(String, JSON::Any). On serialization, any keys inside json_unmapped will be serialized appended to the current json object.

[bew]

Typo: => latitude

[bew]

Typo: => latitude

[bew]

Too much replace ^^
it should still be longitude

[RX14]

Just this, then copy for YAML.

[RX14]

The JSON::Serializable module automatically generates methods for JSON serialization when included.

[RX14]

Needs to regain `code` and down the page.

[RX14]

supported as instance variable values

[RX14]

If the JSON::Serializable::Strict module is included, ...

[RX14]

ditto

[kostya]

i rebased and squashed, i think this is done.

[bcardiff]

I would rather merge this after 0.25. I haven't review it yet. But we will link the PR as an example of user defined annotation and have a bit more of feedback/time to review.

[RX14]

What's the benefit of merging after 0.25.0?

[sdogruyol]

We already have annotations in this release, so having JSON::Serializable in the std is a great use-case 👍

[vlazar]

Not having this as part of 0.25 release would probably result in much less feedback.

[bcardiff]

Ok, let's include them but, as followups:

1. ./next should be collapsed after release together with the Crystal::VERSION mention
2. I would've imagine that a common field serialization declaration would come handy
3. It would be great to rewrite mapping in terms of serializers. It would reduce the burden of keeping everything in sync. I think the mapping API is nice and has value.

[asterite]

At this point I'm thinking JSON::Serializable wasn't a very good idea. It has the namespace problem mentioned in #6663 and I'm not sure there's a good solution for that. Plus now the default value of an instance variable is related to the default value when parsing JSON, which is not always desired.

I think JSON.mapping was good: it was simple, just generated code immediately at (macro) call site, so all types were correctly resolved, and also it was clear what members were serialized, as opposed to know where you have to ignore some fields and all are included by default.

With that, I'd also like to remove user-defined annotations for now because they exhibit the problem in #6663. The language was working fine and it was simpler without all of this. We can revisit annotations later.

Thoughts?

[j8r]

Experimenting is good, I'm sure we'll end with a good solution for serialization.
What's sure is keeping both JSON::Serializable and JSON.mapping isn't a good idea in the long term, better to have one way to do things.

An option is to put annotation behind a flag instead of removing them, because they may be added again later.

[straight-shoota]

Thoughts?

My thought would be to open a new issue for this instead of reviving an old PR.

[RX14]

@asterite the reason we added JSON::Serializable was to support inheritance in json-serializable objects and to reduce duplication between JSON and YAML serialization definitions.

I think that solving those problems is harder than solving #6663. I think that resolving type paths in the scope in which they were originally defined instead of when they are pasted is a good default and doable.