Fixes #138 - Implement custom id schema's #143

michielbdejong · 2015-09-03T14:04:38Z

There are still variable names like options.forceUUID which refer to the default IdSchema, not sure if that's worth an API change?

I did not run the integration test locally (I don't have my environment set up correctly yet for that), but I added some unit tests, and unit tests all seem to pass.

The approach I took was:

Move src/transformers/ to src/plugins/.
Move the UUID-related code into src/plugins/IdSchema.js.
The collection creates an instance of that in this._idSchema in the collection constructor.
The IdSchema class is exposed as Kinto.IdSchema for people to derive their custom id schema from.
I also added Kinto.createIdSchema in parallel with Kinto.createRemoteTransformer.
I added a section to doc/api.md, let me know if it's clear
I didn't add much in the way of code comments, I think it's pretty clear, but let me know if you want more comments / docs / unit tests / etc.

In any case, I thought maybe you can give me some early feedback and let me know if you like the API, what you think of variable naming, and what I should improve?

Cheers!
Michiel.

n1k0 · 2015-09-03T14:15:02Z

src/collection.js

@@ -104,8 +106,14 @@ export default class Collection {
    });
  }

-  use(transfomer) {
-    this._transformers[transfomer.type].push(transfomer);
+  use(plugin) {


I kinda like plugin :) Thought about processor, but plugin sounds even more generic.

It seems that this brings a situation where different extensibility mechanisms (e.g. plugins, adapters, transformers) overlaps, without a clear distinction in their names.

Until now, we had a clear scoping with each of the concepts, if we introduce "plugins", I fear that we might lose this. Should we consider generators (like we did for Kinto server) ?

Oh, sorry I misunderstood this! We have different kinds of plugins (adapters, transformers, etc). Ignore these comments: this is a good proposal.

n1k0 · 2015-09-03T14:27:49Z

There are still variable names like options.forceUUID which refer to the default IdSchema, not sure if that's worth an API change?

It definitely needs at least a name change, eg. forceId.

I did not run the integration test locally (I don't have my environment set up correctly yet for that), but I added some unit tests, and unit tests all seem to pass.

The good news is, integration tests pass just fine (as you can see in Travis anyway) :)

The collection creates an instance of that in this._idSchema in the collection constructor.

Could we imagine having a single _plugins property containing both the IdSchema and transformers?

I added a section to doc/api.md, let me know if it's clear

Super clear! All in all this is shaping super good. Thanks again.

michielbdejong · 2015-09-03T14:34:06Z

[options.forceUUID] definitely needs at least a name change, eg. forceId.

OK! And then I'll also change the mentions of UUID to just 'id' in error messages. So technically, these are breaking changes, which implies a bump in the version string in package.json, correct?

Natim · 2015-09-03T14:35:44Z

docs/api.md

+    return this._next++;
+  }
+  validate(id) {
+    return ((id == parseInt(id, 10)) && (id >= 0));


I wonder if we should show an example that doesn't handle unicity, maybe it worth adding a note or something.

So you mean just return (typeof id == 'number');? Sure, that's simpler to read, yet still shows how the validate function should work.

Yes I like that, plus a note saying that:

In a real application, you want to make sure you do not generate twice the same record id on a collection.
This dummy example doesn't take care of id unicity. In case of id conflict you may loose data.

Yes, I like that because it makes the user learn about common issues when reading the docs.

michielbdejong · 2015-09-03T14:37:36Z

Could we imagine having a single _plugins property containing both the IdSchema and transformers?

OK, I'll move this._idSchema to this._plugins.idSchema and the this._transformers.remote array to this._plugins.remoteTransformers in src/collection.js, and then local transformers can go into a this._plugins.localTransformers array

Natim · 2015-09-03T14:38:35Z

docs/api.md

+
+### Limitations
+
+There's currently no way to deal with adding a custom id schema to an already filled remote database; that would mean remote data migrations, and both Kinto and Kinto.js don't provide this feature just yet.


There's currently no way to deal with adding a custom id schema to an already filled remote database

Isn't it what we are doing with Syncto? I think we should specify that there are no way to switch from UUID4 to custom id schema because if your remote database already respect the schema there are no limitation.

Right! True. The only thing to avoid is adding an id schema which the /local/ data does not adhere to. Will correct that.

Natim · 2015-09-03T14:49:41Z

This looks good man :)

Natim · 2015-09-03T14:50:38Z

technically, these are breaking changes, which implies a bump in the version string in package.json, correct?

I would go for it 👍

n1k0 · 2015-09-03T16:11:01Z

So technically, these are breaking changes, which implies a bump in the version string in package.json, correct?

Yes, rc4 it is. I'll package and publish it as soon as this is merged.

OK, I'll move this._idSchema to this._plugins.idSchema and the this._transformers.remote array to this._plugins.remoteTransformers in src/collection.js, and then local transformers can go into a this._plugins.localTransformers array

I'm wondering about _plugins.transformers.local and _plugins.transformers.remote… Yeah, that's a little long. Your call.

almet · 2015-09-07T08:29:05Z

docs/api.md

+
+By default, kinto.js uses UUID4 strings for record id's. If you want to work with an existing body of data, this may not be what you want.
+
+You can define a custom id schema on a collection by calling the `Collection#use()` method, which accepts a `Kinto.IdSchema`-derived object instance:


I feel having one use API which accepts different types of input is misleading. I've seen this pattern used for express (the node.js framework) in the past and it seems to be counter intuitive (e.g. really easy to use, not as easy to read).

Instead, we could have different methods to install the different configurations.

Here are different proposals:

Have a Collection#configure to which we would pass an object with an id-schema key;

Variant, pass two arguments to Collection#setConfig. e.g. setConfig('id-schema', object)

Have a Collection#setIdSchema to which we pass an object;

Thoughts?

almet · 2015-09-07T08:42:42Z

Thanks for working on this @michielbdejong.

michielbdejong · 2015-09-07T09:27:15Z

I like the first option! So that would mean, change use to be called configure and to take an object with keys instead of looking at the prototypes like it does now. So then instead of:

collection.use(myIdSchema);
collection.use(myRemoteTransformer);

it would become:

collection.configure({
  idSchema: myIdSchema,
  remoteTransformers: [ myRemoteTransformer ]
});

Looks much more readable to me, indeed. Since this PR is going to introduce (small) breaking API changes anyway, like changes in error message strings, and already changes use, shall I just include this change as well?

n1k0 · 2015-09-07T10:39:45Z

Looks much more readable to me, indeed

Definitely, let's do this!

shall I just include this change as well?

Hmm that's gonna be a rather big patch :) If you feel like it, do it — but please try to split the different facets of the implementation in distinct commits, for easier reviews.

n1k0 · 2015-09-07T10:40:36Z

Of course we could also iterate here; landing this patch first, filing a new issue for the Collection#configure thing which could be addressed as soon as this lands.

michielbdejong · 2015-09-07T12:47:06Z

OK, created #148 for this change, will submit a separate PR for that, and then rebase this one on that.

michielbdejong · 2015-09-07T14:42:01Z

Rebased this branch on #148 and applied the following review comments:

name change forceUUID -> forceId, etc.
add comment about what forceId does
simplify example (just check for typeof 'number'), and add note about unicity
Correct phrase:
"There's currently no way to deal with adding a custom id schema to an already filled remote database"

Did not yet apply the review comment about moving this._idSchema and this._transformers.remote into one data structure, because I'm not sure whether to call it this._plugins (and then maybe rename Collection#configure to Collection#configurePlugins), or calling it this._config so it's more logical in combination with the naming of the Collection#configure method.

n1k0 · 2015-09-07T15:58:27Z

or calling it this._config so it's more logical in combination with the naming of the Collection#configure method

That would make a lot of sense to me. _config sounds good.

n1k0 · 2015-09-07T15:59:50Z

That would make a lot of sense to me. _config sounds good.

BTW, how about supporting passing the config object as an arg to Kinto#collection? Eg.

const kinto = new Kinto();
const collection = kinto.collection("foo", {
  idSchema: myIdSchema,
  remoteTransformers: [ myRemoteTransformer ]
});

michielbdejong · 2015-09-08T11:06:57Z

Will improve test coverage, particularly for Kinto.createIdSchema, Kinto.IdSchema, and kinto.collection("articles", { idSchema: myIdSchema });.

michielbdejong · 2015-09-14T12:34:20Z

Need to fix how UuidSchema inherits from IdSchema, and check/improve test coverage.

michielbdejong · 2015-09-14T16:11:16Z

r? @Natim

n1k0 · 2015-09-14T16:56:41Z

src/collection.js

 import { cleanRecord } from "./api";

+import UuidSchema from "./schemas/uuidschema";


Nit: UUIDSchema?

n1k0 · 2015-09-14T17:18:36Z

Looks awesome to me; r=r+wc from me, though I'd like to hear from @Natim as well :)

n1k0 · 2015-09-15T08:34:24Z

Note that CI reported an error: Error: Cannot find module './schemas/UUIDSchema'; I think you need to reference the filename using the exact same case, here uuidschema. Or you could rename the file as UUIDSchema.js, your call.

almet · 2015-09-15T08:50:53Z

docs/api.md

@@ -558,3 +559,70 @@ coll = kinto.collection("articles", {
 There's currently no way to deal with adding transformers to an already filled remote database; that would mean remote data migrations, and both Kinto and Kinto.js don't provide this feature just yet.

 **As a rule of thumb, you should only start using transformers on an empty remote collection.**
+
+## Id schemas


nit: use "ID", as id is used to refer to something completely different.

n1k0 · 2015-09-15T13:13:36Z

Coverage being okay on coveralls, I think we can now safely land this. @michielbdejong please do :)

n1k0 · 2015-09-15T13:15:08Z

Just take care of squashing the commits first.

michielbdejong · 2015-09-15T14:12:17Z

change the api.md document in other places where "id" is supposed to be "ID"

Ah sorry, hadn't searched properly. I did not change the ones in code comments in src/, since they're very close to the lower-case variable name there, and I thought changing phrases like "the id field" to "the ID field" would be confusing. But I now changed all the ones in api.md outside code snippets.

Thanks for your reviews!

Fixes #138 - Implement custom id schema's

michielbdejong force-pushed the 1200156-custom-id-schemas branch from 4124375 to 320463c Compare September 3, 2015 14:05

michielbdejong referenced this pull request in michielbdejong/kinto.js Sep 3, 2015

Draft implementation

ff8f6d3

n1k0 reviewed Sep 3, 2015
View reviewed changes

Natim reviewed Sep 3, 2015
View reviewed changes

almet reviewed Sep 7, 2015
View reviewed changes

michielbdejong mentioned this pull request Sep 7, 2015

Move Collection#use to optional second argument of Kinto#collection #148

Closed

michielbdejong force-pushed the 1200156-custom-id-schemas branch 2 times, most recently from d19d1ae to 1df0fa1 Compare September 7, 2015 14:37

michielbdejong force-pushed the 1200156-custom-id-schemas branch from 1df0fa1 to 8f880d4 Compare September 8, 2015 10:57

This was referenced Sep 8, 2015

Fixes #148 - Extra arg on Kinto#collection to replace Collection#use. #149

Merged

Avoid typing and extending classes for transformers #155

Closed

michielbdejong force-pushed the 1200156-custom-id-schemas branch from 8f880d4 to df1c28a Compare September 14, 2015 11:42

michielbdejong force-pushed the 1200156-custom-id-schemas branch from df1c28a to 5678ab2 Compare September 14, 2015 12:29

michielbdejong changed the title ~~Implement custom id schema's, fix #138~~ Fixes #138 - Implement custom id schema's Sep 14, 2015

michielbdejong force-pushed the 1200156-custom-id-schemas branch 3 times, most recently from 83cb854 to be9c4af Compare September 14, 2015 16:11

n1k0 reviewed Sep 14, 2015
View reviewed changes

src/collection.js

import { cleanRecord } from "./api";

import UuidSchema from "./schemas/uuidschema";

Copy link

Contributor

n1k0 Sep 14, 2015

Choose a reason for hiding this comment

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

Nit: UUIDSchema?

michielbdejong mentioned this pull request Sep 15, 2015

Raise an error when an id is passed to Collection#create but not necessary #158

Closed

almet reviewed Sep 15, 2015
View reviewed changes

michielbdejong force-pushed the 1200156-custom-id-schemas branch from 0216b15 to 82b00c2 Compare September 15, 2015 14:05

Fixes Kinto#138 - Implement custom id schema's

82b00c2

michielbdejong added a commit that referenced this pull request Sep 15, 2015

Merge pull request #143 from michielbdejong/1200156-custom-id-schemas

6ad02a6

Fixes #138 - Implement custom id schema's

michielbdejong merged commit 6ad02a6 into Kinto:master Sep 15, 2015

michielbdejong deleted the 1200156-custom-id-schemas branch September 15, 2015 14:13

n1k0 mentioned this pull request Sep 29, 2015

Bump 1.0.0-rc.4. #187

Merged


		### Limitations

		There's currently no way to deal with adding a custom id schema to an already filled remote database; that would mean remote data migrations, and both Kinto and Kinto.js don't provide this feature just yet.


		By default, kinto.js uses UUID4 strings for record id's. If you want to work with an existing body of data, this may not be what you want.

		You can define a custom id schema on a collection by calling the `Collection#use()` method, which accepts a `Kinto.IdSchema`-derived object instance:

		import { cleanRecord } from "./api";

		import UuidSchema from "./schemas/uuidschema";

Fixes #138 - Implement custom id schema's #143

Fixes #138 - Implement custom id schema's #143

Conversation

michielbdejong commented Sep 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n1k0 commented Sep 3, 2015

michielbdejong commented Sep 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michielbdejong commented Sep 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Natim commented Sep 3, 2015

Natim commented Sep 3, 2015

n1k0 commented Sep 3, 2015

Choose a reason for hiding this comment

almet commented Sep 7, 2015

michielbdejong commented Sep 7, 2015

n1k0 commented Sep 7, 2015

n1k0 commented Sep 7, 2015

michielbdejong commented Sep 7, 2015

michielbdejong commented Sep 7, 2015

n1k0 commented Sep 7, 2015

n1k0 commented Sep 7, 2015

michielbdejong commented Sep 8, 2015

michielbdejong commented Sep 14, 2015

michielbdejong commented Sep 14, 2015

Choose a reason for hiding this comment

n1k0 commented Sep 14, 2015

n1k0 commented Sep 15, 2015

Choose a reason for hiding this comment

n1k0 commented Sep 15, 2015

n1k0 commented Sep 15, 2015

michielbdejong commented Sep 15, 2015