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

Validation and schema: where does the "validation" key belongs? #174

Closed
almet opened this issue Sep 1, 2015 · 4 comments
Closed

Validation and schema: where does the "validation" key belongs? #174

almet opened this issue Sep 1, 2015 · 4 comments
Labels

Comments

@almet
Copy link
Member

almet commented Sep 1, 2015

A recent pull request introduced schema validation to Kinto. That is a really nice feature and I'm happy to see it shipped.

However, it seems odd to me to have the schema defined inside the data attribute. After some discussions with @Natim and @n1k0 it seems that we could all agree on the fact that we expected it to be at the same level than the data attribute. e.g.

{
   "data": {"foo": "bar"},
   "schema": ...
}

It seems the rationale for putting it inside the data (as it is right now) is to not break the protocol. However, I believe putting it alongside the data key doesn't break the protocol either: adding features to kinto by adding values next to data and permissions seems a reasonable choice: the cliquet protocol would sill be respected in this case.

@leplatrem what are your thoughts about this? Are we missing the point of having this defined inside the data attribute?

@leplatrem
Copy link
Contributor

First, let's note that there are two locations where the schema attribute was introduced:

  • on the collection object, for the JSON schema definition
  • on records, for the validated schema version

How it was done

Regarding the collection object, I found it consistent to put it in data. As @Natim said:

In this implementation, we are just providing a new property to the collection which is the schema.
Other property may come such as signing, description, title.

The schema is the collection data, it is completely consistent :) Because data is just the name we gave to the main payload of response, and has no further semantics. That's what kinto.js reads when it receives the response.

Plus it was very easy to implement, since completely in the rails of current cliquet resource code.

As for the record, since we wanted to have a way to filter records by schema version, it had to become a record attribute. In cliquet, we store the data content in the storage backend (which allows filtering) and the permissions in the permissions backend.

Removing the odd

Now. I fully understand your concern : keeping the application specific properties in data and every Kinto notion at the root level is a good (new) idea !

But IMO, in order to remain consistent, we should then also move id and last_modified !

Collection would look like this:

{
   "id": "087-7u8097908708",
   "last_modified": 456789986,
   "fingerprint": "11:D5:D2:0A:9A:F8:D9:FC:23:6E:5C",
   "data": {
       "author": "Bozo the clown",
   },
  "permissions": {
     "write": ["fxa:00567y09"]
  },
  "schema": {
    ....json-schema...
  }
}

Same idea for records:

{
   "id": "087-7u8097908708",
   "last_modified": 78945144,
   "fingerprint": "11:D5:D2:0A:9A:F8:D9:FC:23:6E:5C",
   "data": {
       "title": "F Society",
       "url": "http://thefreedictionary.com/decentralize",
   },
  "permissions": {
     "write": ["fxa:00567y09"]
  },
  "schema": 456789986
}

But, hey, this is not a minor task. It does not look like one of our priority either. And it would break kinto.js AFAIK.

Even though I find it attractive, I am not sure I want to put a foot in this energy absorbing swirl :) This is pretty huge.

Pragmatism

In order to implement your idea without inverting too much effort, we could :

  • leave id and last_modified as they are
  • leave the schema attribute on records as it is
  • move the schema attribute to the root of collection object

But, does it really bring any value ? Will it be fondamentally better ? What are our criterias to evaluate that ?

Please note that it would be a lot cleaner if we refactor some part of cliquet resource schema manipulation in order to achieve this. Otherwise payload validation (especially for PATCH request) will have even more ugly parts. I would be motivated to do it though.

@almet
Copy link
Member Author

almet commented Sep 7, 2015

Thanks for your answer 👍

We had a parallel discussion about the location of id and last_modified (which we named metadata). There are multiple reasons to put them in a different location than the data itself (one of them being that the clients currently need to mutate the returned objects to pass them to their consumers) and we were thinking about putting them in a metadata namespace.

I was concerned about the complexity it adds to the API, and that's the reason why I like your proposal: it separates the metadata from the data itself, without adding yet another namespace.

I would propose that we do that by iterations:

  1. Move the schema attribute to the root of the collection object. Leave the schema attribute on records as it is for now;
  2. Move id and last_modified to the root as well;
  3. Move the schema attribute of records to the root.

This change would impact both Kinto.js, and Syncto. If we have to break the API, better do it sooner than later: nobody is currently using this protocol for something that shipped. If we want/have to wait, we'll need to think handle backward compatibility, which isn't a concern we have right now.

Also, there is something I don't get: why are you saying this is that complex? What do you identify as the potential implementation problems?

@leplatrem
Copy link
Contributor

We talked about this in a meeting. The current implementation has the advantage to keep data content consistent between single record and list of records responses.

We came with the following conclusion:

  • We release Kinto with the schema validation feature as experimental and subject to change
  • We keep investigating to find a solution that is consistent and intuitive :)

@leplatrem
Copy link
Contributor

I would vote for closing this issue in favor of #256

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants