Skip to content
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

lib/json: unify the writing API using serialization only #2312

Merged
merged 19 commits into from
Sep 30, 2016

Conversation

xymus
Copy link
Contributor

@xymus xymus commented Sep 14, 2016

Some context first, before this PR, there were 2 API to write JSON:

  • The module static provided to_json and to_pretty_json which supported writing only basic Nit types associated with JSON types and objects with a manually implemented to_json.
  • The module serialization provides serialize_to_json which supports all Serializable types, a superset of the JSON types, and where standard Nit objects are serialized to JSON objects by generated code.

The advantages of the serialization API are:

  • Objects written to JSON with some metadata (mainly the name of the Nit type) can be deserialized back to Nit objects.
  • The boilerplate code to write the attributes is generated, usually a single is serialize after the module declarations does the trick. (core_serialize_to, the equivalent of to_json, doesn't have to be implemented in each classes, the generated version is usually enough.)
  • Implementing core_serialize_to (or leaving it to the generated code) instead of to_json makes the object compatible with both the JSON and binary serialization services.
  • In general, the serialization API allows to easily write Nit objects to JSON and build JSON objects using Map instances:
import json::serialization

class Person
    serialize

    var name: String
    var year_of_birth: Int
end

var bob = new Person("Bob", 1986)
assert bob.serialize_to_json(pretty=true, plain=true) == """
{
    "name": "Bob",
    "year_of_birth": 1986
}"""

var charlie = new Map[String, nullable Serializable]
charlie["name"] = "Charlie"
charlie["year_of_birth"] = 1968
assert charlie.serialize_to_json(pretty=true, plain=true) == """
{
    "name": "Charlie",
    "year_of_birth": 1968
}"""

So this PR drops the static writing API, and replaces its services (to_json and to_pretty_json) by aliases in serialization. Some subclasses have been updated to implement the recursive serialize_to instead of the to_json, the changes are minimal to avoid breaking any software, but they are not optimal. The result is a single JSON writing API!

The generated JSON should not have changed much (maybe a bit less or more white spaces), except for some error messages which were translated to invalid JSON and are now serialized to a valid JSON object.

I expect some tests to fail as this is a big change, and clients of the json::static writing services (@Morriar) should probably double check my changes to their code.

In the future, I'm thinking of dropping the name serialize_to_json and to_pretty_json and move the optionnal parameters to to_json. The JSON module also really needs refactoring at this point.

@Morriar
Copy link
Member

Morriar commented Sep 14, 2016

+1

@xymus xymus force-pushed the json-unify-write branch 2 times, most recently from 54b5aa0 to 1e54045 Compare September 14, 2016 19:19
@BlackMinou
Copy link
Contributor

+1

@xymus xymus force-pushed the json-unify-write branch 5 times, most recently from 45d8a8e to d02cb01 Compare September 15, 2016 14:44
@xymus xymus force-pushed the json-unify-write branch 3 times, most recently from cdcfa6d to 5a35318 Compare September 15, 2016 17:08
@xymus
Copy link
Contributor Author

xymus commented Sep 15, 2016

The remaining tests fail because of #2296. I may have to rename the json::serialization module juste so the tests can pass.

@xymus xymus force-pushed the json-unify-write branch 4 times, most recently from 2be86ff to 690e9c3 Compare September 19, 2016 13:38
@xymus
Copy link
Contributor Author

xymus commented Sep 19, 2016

Rerolled to fix all clients by making the generated JSON closer to what generated static.
I've also done some refactoring to work around #2296:

Because of the refactor, I'll have to do a manual merge with #2311.

@@ -120,7 +120,7 @@ class Achievement
json["reward"].as(Int))
end

redef fun to_json do
redef fun to_json_object do do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do do?

@@ -104,7 +104,7 @@ class GameEvent
time = new ISODate.from_string(json["time"].as(String))
end

redef fun to_json do
redef fun to_json_object do do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do do!

@xymus
Copy link
Contributor Author

xymus commented Sep 19, 2016

Removed do dos.

There are a total of 3 API to read JSON:
* `JsonDeserializer` reads JSON to recreate complex Nit objects (discussed here),
* the module `json::dynamic` provides an easy API to explore JSON objects,
* the module `json::static` provides the low-level parse JSON with creates basic Nit objects.
Copy link
Contributor

@lbajolet lbajolet Sep 19, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you mean by with creates basic Nit objects, can you clarify this sentence?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed it, that sentence was scrambled...

Copy link
Contributor

@lbajolet lbajolet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the modified sentence in the README, +1

privat added a commit that referenced this pull request Sep 23, 2016
Some context first, before this PR, there were 2 API to write JSON:
* The module `static` provided `to_json` and `to_pretty_json` which supported writing only basic Nit types associated with JSON types and objects with a manually implemented `to_json`.
* The module `serialization` provides `serialize_to_json` which supports all `Serializable` types, a superset of the JSON types, and where standard Nit objects are serialized to JSON objects by generated code.

The advantages of the `serialization` API are:
* Objects written to JSON with some metadata (mainly the name of the Nit type) can be deserialized back to Nit objects.
* The boilerplate code to write the attributes is generated, usually a single `is serialize` after the module declarations does the trick. (`core_serialize_to`, the equivalent of `to_json`, doesn't have to be implemented in each classes, the generated version is usually enough.)
* Implementing `core_serialize_to` (or leaving it to the generated code) instead of `to_json` makes the object compatible with both the JSON and binary serialization services.
* In general, the `serialization` API allows to easily write Nit objects to JSON and build JSON objects using `Map` instances:
~~~
import json::serialization

class Person
    serialize

    var name: String
    var year_of_birth: Int
end

var bob = new Person("Bob", 1986)
assert bob.serialize_to_json(pretty=true, plain=true) == """
{
    "name": "Bob",
    "year_of_birth": 1986
}"""

var charlie = new Map[String, nullable Serializable]
charlie["name"] = "Charlie"
charlie["year_of_birth"] = 1968
assert charlie.serialize_to_json(pretty=true, plain=true) == """
{
    "name": "Charlie",
    "year_of_birth": 1968
}"""
~~~

So this PR drops the `static` writing API, and replaces its services (`to_json` and `to_pretty_json`) by aliases in `serialization`. Some subclasses have been updated to implement the recursive `serialize_to` instead of the `to_json`, the changes are minimal to avoid breaking any software, but they are not optimal. The result is a single JSON writing API!

The generated JSON should not have changed much (maybe a bit less or more white spaces), except for some error messages which were translated to invalid JSON and are now serialized to a valid JSON object.

I expect some tests to fail as this is a big change, and clients of the `json::static` writing services (@Morriar) should probably double check my changes to their code.

In the future, I'm thinking of dropping the name `serialize_to_json` and `to_pretty_json` and move the optionnal parameters to `to_json`. The JSON module also really needs refactoring at this point.

Pull-Request: #2312
Reviewed-by: Alexandre Terrasa <[email protected]>
Reviewed-by: Romain Chanoir <[email protected]>
Reviewed-by: Jean Privat <[email protected]>
Reviewed-by: Lucas Bajolet <[email protected]>
@privat privat merged commit 88b5b3f into nitlang:master Sep 30, 2016
@xymus xymus deleted the json-unify-write branch November 16, 2016 15:23
privat added a commit that referenced this pull request Nov 24, 2016
Was broken since #2312

* Migrate `model_json` to new serialization API
* Add tests to `model_json` to ensure this doesn't happen anymore
* Migrate `web` package and `nitweb` to new API

@xymus a lot of serialization for you. Note: for each entity, I needed two deserialization version `full` and no full. Wasn't sure on how to handle this concern properly with your API

Pull-Request: #2327
Reviewed-by: Alexis Laferrière <[email protected]>
privat added a commit that referenced this pull request Dec 3, 2016
Was broken since #2312

* Migrate `model_json` to new serialization API
* Add tests to `model_json` to ensure this doesn't happen anymore
* Migrate `web` package and `nitweb` to new API

@xymus a lot of serialization for you. Note: for each entity, I needed two deserialization version `full` and no full. Wasn't sure on how to handle this concern properly with your API

Pull-Request: #2327
Reviewed-by: Alexis Laferrière <[email protected]>
privat added a commit that referenced this pull request Dec 3, 2016
Was broken since #2312

* Migrate `model_json` to new serialization API
* Add tests to `model_json` to ensure this doesn't happen anymore
* Migrate `web` package and `nitweb` to new API

@xymus a lot of serialization for you. Note: for each entity, I needed two deserialization version `full` and no full. Wasn't sure on how to handle this concern properly with your API

Pull-Request: #2327
Reviewed-by: Alexis Laferrière <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants