1216749 - Land the Firefox Kinto.js client

Status: RESOLVED FIXED

(Cloud Services :: Firefox: Common, defect)

Description

[Mark Goodwin [:mgoodwin]]

I've been working on making kinto.js work in platform to support bug 1197703.

Comment 1

[Mark Goodwin [:mgoodwin]]

Attachment: Bug1216749.patch

The bits of kinto.js that are specific to this are:
The entry point - https://github.com/Kinto/kinto.js/blob/212-firefox-entry-point/fx-src/index.js
The storage adapter - https://github.com/Kinto/kinto.js/blob/212-firefox-entry-point/fx-src/FirefoxStorage.js

Some tests for the latter are included in this patch.

Nothing else in this file is specific to this platform.

Comment 2

[Mark Goodwin [:mgoodwin]]

Attachment: Bug1216749.patch

Updated to address feedback on kinto.js PR #219

Comment 3

[Richard Newman [:rnewman]]

(In reply to Mark Goodwin [:mgoodwin] from comment #1)

> The bits of kinto.js that are specific to this are:


I reviewed that PR. I think it's worth the effort to rewrite this as a tiny wrapper around Sqlite.jsm…

Comment 4

[Mark Goodwin [:mgoodwin]]

(In reply to Richard Newman [:rnewman] from comment #3)
> I reviewed that PR.

Awesome, thanks.

> I think it's worth the effort to rewrite this as a tiny
> wrapper around Sqlite.jsm…

You're absolutely correct. I did that and now it's much nicer.

Comment 5

[Mark Goodwin [:mgoodwin]]

Attachment: Bug1216749.patch

I made some things better, specifically:
* Storage adapter re-written to take advantage of Sqlite.jsm
* Test coverage improved in test_storage_adapter.js
* Tests in test_storage_adapter.js made rather less horrible because of a tasks related lightbulb moment

Comment 6

[Mark Goodwin [:mgoodwin]]

Attachment: Bug1216749.patch

Added test coverage to prevent regressions of Adapter API inconsistencies

Comment 7

[Mark Goodwin [:mgoodwin]]

Attachment: Bug1216749_client_test.patch

Some tests for the actual kinto client

Comment 8

[Mark Goodwin [:mgoodwin]]

Attachment: Bug1216749_client_test.patch

Added some kinto client tests that involve interaction with a server. The intent here is not to test core kinto.js functionality (this has excellent coverage  in the kinto.js project), more a sanity test.

Comment 9

[Mark Goodwin [:mgoodwin]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb9b0f7b76b1

Comment 10

[Richard Newman [:rnewman]]

Mark, you have test failures in that try push.

Comment 11

[Mark Goodwin [:mgoodwin]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7927581612d

Comment 12

[Richard Newman [:rnewman]]

Comment on attachment 8678148 [details] [diff] [review]
Bug1216749_client_test.patch

Review of attachment 8678148 [details] [diff] [review]:
-----------------------------------------------------------------

Overall points:

* Test failures on try!
* I'd like to see a test that attempts to break this:
** Call .create twice with the same record, and with a different record but the same ID.
** Kick off two operations without waiting for the first promise to resolve. What happens? With your current code this might open the database twice concurrently, and if they're both writes you'll get a SQLITE_BUSY.

::: services/common/tests/unit/test_kinto.js
@@ +73,5 @@
> +  // create an expected number of records
> +  const expected = 10;
> +  const newRecord = { foo: "bar" };
> +  for (let i = 0; i < expected; i++) {
> +    yield collection.create(newRecord);

Great example of DB flapping here -- this'll open the DB ten times!

@@ +102,5 @@
> +  do_check_eq(getResult.data.id, deleteResult.data.id);
> +  // and check that get no longer returns the record
> +  try {
> +    getResult = yield collection.get(createResult.data.id);
> +    do_throw("there should not be a result");

It surprises me a little that .get throws if the ID is not present.

@@ +141,5 @@
> +add_task(function* test_kinto_sync(){
> +  const configPath = "/v1/";
> +  const recordsPath = "/v1/buckets/default/collections/test_collection/records";
> +  // register a handler
> +  function handleResponse (aRequest, aResponse) {

New code shouldn't use `aFoo` and `gBar`. (Throughout)

@@ +155,5 @@
> +      for (let headerLine of sampled.sampleHeaders) {
> +        let headerElements = headerLine.split(':');
> +        aResponse.setHeader(headerElements[0], headerElements[1].trimLeft());
> +      }
> +      aResponse.setHeader("Date",(new Date()).toUTCString());

Space after comma.

Comment 13

[Richard Newman [:rnewman]]

Comment on attachment 8677519 [details] [diff] [review]
Bug1216749.patch

Review of attachment 8677519 [details] [diff] [review]:
-----------------------------------------------------------------

Rubberstamp with the understanding that you'll land the newest snapshot!

::: services/common/moz.build
@@ +14,5 @@
>  ]
>  
>  EXTRA_JS_MODULES['services-common'] += [
>      'logmanager.js',
> +    'moz-kinto-client.js',

Do you want to guard this with a build define of its own, or ship it everywhere?

Comment 14

[Mark Goodwin [:mgoodwin]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=59079eec0594

Comment 15

[Mark Goodwin [:mgoodwin]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=973c6d480cef

Comment 16

[Mark Goodwin [:mgoodwin]]

Attachment: Bug1216749.patch

Updated with current snapshot

Comment 17

[Mark Goodwin [:mgoodwin]]

Attachment: Bug1216749_client_test.patch

Addressed feedback; specifically:
 - Try failures should be fixed (it's taking a while for Windows xpcshell runs to happen so we'll have to wait and see). The issue here was related to use of super in es6 classes - will investigate further later since I'd actually like to use these eventually.
 - Added tests for multiple creations, for create-with-id.
 - Added test for multiple collections simultaneously writing to the db.
 - removed use of aFoo and gBar
 - fixed a whitespace nit.

Re. .get throwing for a missing id; that's what the API does. It's a little most sensible when you think that it's actually a promise failing to resolve, maybe?

Comment 18

[Richard Newman [:rnewman]]

(In reply to Mark Goodwin [:mgoodwin] from comment #17)

> Re. .get throwing for a missing id; that's what the API does. It's a little
> most sensible when you think that it's actually a promise failing to
> resolve, maybe?

Yeah, understood; that was a comment about the API rather than your code!

This is the kind of modeling decision that we make in languages with Optional and Result -- when we don't find what we're looking for, is that a failure (just like "your database is corrupt"!), or do we actually find nothing, and return/resolve nil?

I usually fall in the latter camp, so that failures are real failures, but JavaScript doesn't make it easy to be explicit about when values might be undefined or null, so I can understand the motivation to reject when the value isn't a valid non-null record.

Comment 19

[Richard Newman [:rnewman]]

Comment on attachment 8680119 [details] [diff] [review]
Bug1216749_client_test.patch

Review of attachment 8680119 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with comments addressed.

::: services/common/tests/unit/test_kinto.js
@@ +31,5 @@
> +function* clear_collection() {
> +  const collection = do_get_kinto_collection();
> +  yield collection.db.open();
> +  yield collection.clear();
> +  yield collection.db.close();

try..finally

@@ +57,5 @@
> +  collection.create(newRecord);
> +  collection.create(newRecord);
> +  collection.create(newRecord);
> +  yield collection.create(newRecord);
> +  yield collection.db.close();

finally (throughout)

@@ +74,5 @@
> +  let newRecord = { foo: "bar" };
> +
> +  // perform several write operations alternately without waiting for promises
> +  // to resolve
> +  for (let i=0; i < 10; i++) {

Nit: spaces around =

@@ +76,5 @@
> +  // perform several write operations alternately without waiting for promises
> +  // to resolve
> +  for (let i=0; i < 10; i++) {
> +    collection1.create(newRecord);
> +    collection2.create(newRecord);

Accumulate these promises then wait on them below, rather than assuming that line 83 will wait for all of these to finish.

@@ +160,5 @@
> +  yield collection.db.open();
> +  const expected = 10;
> +  const created = [];
> +  for (let i = 0; i < expected; i++) {
> +    let newRecord = { foo: "test "+i };

Nit: spaces around operators (throughout).

Comment 20

[Mark Goodwin [:mgoodwin]]

Attachment: Bug1216749_client_test.patch

Addressed nits. Specifically:
- enclosed operations in try block so connections are closed in finally
- added required spaces around operators
- accumulated promises on non-yielded operations, ensured they're waited for

Comment 21

[Mark Goodwin [:mgoodwin]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb7e06f72273

Comment 22

[Mark Goodwin [:mgoodwin]]

Attachment: Bug1216749.patch

Updated with newest snapshot.

Comment 23

[Mark Goodwin [:mgoodwin]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=31520de5b47a

Comment 24

[Mark Goodwin [:mgoodwin]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3d4c9b49f44e04b9354a54d2a26b4a255a14ee88
Bug 1216749 - Land the Firefox Kinto.js client (r=rnewman)

https://hg.mozilla.org/integration/mozilla-inbound/rev/6bb73066c5fe8ebec14ee23430a7f2cdb47ffd47
Bug 1216749 - Additional kinto client tests (r=rnewman)

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3d4c9b49f44e
https://hg.mozilla.org/mozilla-central/rev/6bb73066c5fe

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/3d4c9b49f44e
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/6bb73066c5fe

Comment 27

[Carsten Book [:Tomcat]]

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion