New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JSON::Serializable #6082

Merged
merged 1 commit into from Jun 7, 2018

Conversation

@kostya
Contributor

kostya commented May 9, 2018

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

@kostya kostya referenced this pull request May 9, 2018

Merged

User-defined annotations #6063

@asterite

This comment has been minimized.

Contributor

asterite commented May 9, 2018

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

This comment has been minimized.

Member

RX14 commented May 9, 2018

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

This comment has been minimized.

Contributor

asterite commented May 9, 2018

@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

This comment has been minimized.

Member

RX14 commented May 9, 2018

@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

This comment has been minimized.

Contributor

asterite commented May 9, 2018

What git suffix? Where?

@kostya

This comment has been minimized.

Contributor

kostya commented May 9, 2018

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

This comment has been minimized.

Contributor

asterite commented May 9, 2018

@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

This comment has been minimized.

Contributor

kostya commented May 9, 2018

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

@asterite

This comment has been minimized.

Contributor

asterite commented May 9, 2018

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

This comment has been minimized.

Contributor

asterite commented May 9, 2018

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

This comment has been minimized.

Contributor

kostya commented May 9, 2018

what do you think about this options: d5b218e

@asterite

This comment has been minimized.

Contributor

asterite commented May 9, 2018

@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

This comment has been minimized.

Contributor

asterite commented May 9, 2018

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

This comment has been minimized.

Member

RX14 commented May 9, 2018

@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

This comment has been minimized.

Contributor

asterite commented May 9, 2018

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

@RX14

This comment has been minimized.

Member

RX14 commented May 9, 2018

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

@asterite

This comment has been minimized.

Contributor

asterite commented May 9, 2018

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

This comment has been minimized.

Member

RX14 commented May 9, 2018

@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

This comment has been minimized.

Contributor

asterite commented May 9, 2018

@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

This comment has been minimized.

Member

RX14 commented May 9, 2018

@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

This comment has been minimized.

Contributor

asterite commented May 9, 2018

@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

This comment has been minimized.

Contributor

asterite commented May 9, 2018

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

@RX14

This comment has been minimized.

Member

RX14 commented May 9, 2018

@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

This comment has been minimized.

Contributor

asterite commented May 9, 2018

Ah, OK. Sounds good then.

@RX14

This comment has been minimized.

Member

RX14 commented May 9, 2018

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.

@@ -1,4 +1,4 @@
{% if compare_versions(Crystal::VERSION, "0.25.0") >= 0 %}
{% if Crystal::VERSION.includes?("0.24.2+") %}

This comment has been minimized.

@asterite

asterite May 9, 2018

Contributor

|| Crystal::VERSION == "0.25.0"

This comment has been minimized.

@RX14

RX14 May 9, 2018

Member

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

src/json.cr Outdated
@@ -94,6 +94,6 @@ end
require "./json/*"
{% if compare_versions(Crystal::VERSION, "0.25.0") >= 0 %}
{% if Crystal::VERSION.includes?("0.24.2+") %}

This comment has been minimized.

@asterite

asterite May 9, 2018

Contributor

|| Crystal::VERSION == "0.25.0"

This comment has been minimized.

@RX14

RX14 May 9, 2018

Member

ditto

@kostya

This comment has been minimized.

Contributor

kostya commented May 9, 2018

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

This comment has been minimized.

Contributor

asterite commented May 9, 2018

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

@asterite

This comment has been minimized.

Contributor

asterite commented May 23, 2018

@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

This comment has been minimized.

Contributor

asterite commented May 23, 2018

@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

This comment has been minimized.

Contributor

asterite commented May 23, 2018

@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

This comment has been minimized.

Contributor

straight-shoota commented May 23, 2018

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

This comment has been minimized.

Contributor

kostya commented May 31, 2018

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

@kostya kostya force-pushed the kostya:json_attributes branch from 2dcc3b1 to fdfd955 May 31, 2018

@@ -2,6 +2,109 @@ module JSON
annotation Field
end
# The `JSON::Serializable` module is a better alternative to `JSON.mapping`.

This comment has been minimized.

@RX14

RX14 Jun 2, 2018

Member

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.

#
# class Location
# include JSON::Serializable
# property lat : Float64

This comment has been minimized.

@RX14

RX14 Jun 2, 2018

Member

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

#
# ### Usage
#
# `JSON::Serializable` use to serialize all instanse variables (but any of them can be excluded).

This comment has been minimized.

@RX14

RX14 Jun 2, 2018

Member

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.

# that accepts a `JSON::PullParser` and returns an object from it. Union types are supported,
# if multiple types in the union can be mapped from the JSON, it is undefined which one will be chosen.
#
# To guide the serialization are using property annotation `JSON::Field`, example:

This comment has been minimized.

@RX14

RX14 Jun 2, 2018

Member

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.

# end
# ```
#
# Supported field properties:

This comment has been minimized.

@RX14

RX14 Jun 2, 2018

Member

JSON::Field properties

#
# Supported field properties:
# * **ignore**: if `true` skip this field in seriazation and deserialization (by default false)
# * **key**: the property name in the JSON document (as opposed to the property name)

This comment has been minimized.

@RX14

RX14 Jun 2, 2018

Member

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

# * **presense**: if `true`, a `@{{key}}_present` instance variable will be generated when the key was present (even if it has a `null` value), `false` by default
# * **emit_null**: if `true`, emits a `null` value for nilable property (by default nulls are not emitted)
#
# Deserialization also respect default values of variables:

This comment has been minimized.

@RX14

RX14 Jun 2, 2018

Member

respects

# A.from_json(%<{"a":1}>) # => A(@a=1, @b=1.0)
# ```
#
# When JSON::Serializable included it basically defines a constructor accepting a `JSON::PullParser` that reads from

This comment has been minimized.

@RX14

RX14 Jun 2, 2018

Member

Remove this paragraph

#
# ### Extensions: `JSON::Serializable::Strict` and `JSON::Serializable::Unmapped`.
#
# If module `JSON::Serializable::Strict` included, unknown properties in the JSON

This comment has been minimized.

@RX14

RX14 Jun 2, 2018

Member

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

# If module `JSON::Serializable::Strict` included, unknown properties in the JSON
# document will raise a parse exception. By default the unknown properties
# are silently ignored.
# If module `JSON::Serializable::Unmapped` included, unknown properties in the JSON

This comment has been minimized.

@RX14

RX14 Jun 2, 2018

Member

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.

# property lng : Float64
#
# @[JSON::Field(key: "lat")]
# property latitiude : Float64

This comment has been minimized.

@bew

bew Jun 2, 2018

Contributor

Typo: => latitude

# include JSON::Serializable
#
# @[JSON::Field(key: "lat")]
# property latitiude : Float64

This comment has been minimized.

@bew

bew Jun 2, 2018

Contributor

Typo: => latitude

#
# @[JSON::Field(key: "lng")]
# property longitude : Float64

This comment has been minimized.

@bew

bew Jun 2, 2018

Contributor

Too much replace ^^
it should still be longitude

@RX14

Just this, then copy for YAML.

annotation Field
end
# The `JSON::Serializable` is module which provide automatic serialization of the object to/from json.

This comment has been minimized.

@RX14

RX14 Jun 4, 2018

Member

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

#
# ### Usage
#
# Including JSON::Serializable will create #to_json and self.from_json methods on the current class,

This comment has been minimized.

@RX14

RX14 Jun 4, 2018

Member

Needs to regain `code` and down the page.

# 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

This comment has been minimized.

@RX14

RX14 Jun 4, 2018

Member

supported as instance variable values

#
# ### Extensions: `JSON::Serializable::Strict` and `JSON::Serializable::Unmapped`.
#
# If module `JSON::Serializable::Strict` included, unknown properties in the JSON

This comment has been minimized.

@RX14

RX14 Jun 4, 2018

Member

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

# If module `JSON::Serializable::Strict` included, unknown properties in the JSON
# document will raise a parse exception. By default the unknown properties
# are silently ignored.
# If module `JSON::Serializable::Unmapped` included, unknown properties in the JSON

This comment has been minimized.

@RX14

RX14 Jun 4, 2018

Member

ditto

@RX14

RX14 approved these changes Jun 4, 2018

@kostya kostya force-pushed the kostya:json_attributes branch from a8168dc to 7ddf535 Jun 6, 2018

@kostya

This comment has been minimized.

Contributor

kostya commented Jun 6, 2018

i rebased and squashed, i think this is done.

@bcardiff

This comment has been minimized.

Member

bcardiff commented Jun 6, 2018

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

This comment has been minimized.

Member

RX14 commented Jun 6, 2018

What's the benefit of merging after 0.25.0?

@sdogruyol

This comment has been minimized.

Member

sdogruyol commented Jun 6, 2018

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

@vlazar

This comment has been minimized.

vlazar commented Jun 7, 2018

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

@bcardiff

bcardiff approved these changes Jun 7, 2018 edited

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.

@RX14 RX14 added this to the 0.25.0 milestone Jun 7, 2018

@RX14 RX14 merged commit a1e7233 into crystal-lang:master Jun 7, 2018

4 checks passed

ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@asterite

This comment has been minimized.

Contributor

asterite commented Oct 30, 2018

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

This comment has been minimized.

Contributor

j8r commented Oct 30, 2018

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

This comment has been minimized.

Contributor

straight-shoota commented Oct 30, 2018

Thoughts?

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

@RX14

This comment has been minimized.

Member

RX14 commented Oct 30, 2018

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment