673548 - Combine helpers in head_http_server.js to a complete (or nearly complete) Sync server implementation

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Philipp von Weitershausen [:philikon]]

Right now we have ServerWBO, ServerCollection, track_collections_helper which can be combined in non-obvious ways to simulate a Sync server. We should make all of this easier and combine them into a fully (or at least adequately) functional Sync server on top of nsHttpServer.

Comment 1

[Richard Newman [:rnewman]]

Attachment: WIP. v1

In the spirit of never doing things by halves...

This passes its own mini tests, nothing more. Need to wire in auth, add tracking of timestamps, more helpers for setup, more invocation of callbacks, conform to Sync specs, implement other HTTP verbs..., ...

Comment 2

[Richard Newman [:rnewman]]

Attachment: Part 0: Content-Types, accessors, timestamp tracking. v1

Comment 3

[Richard Newman [:rnewman]]

Attachment: Part 1: return deleted IDs from ServerCollection delete handler. v1

Used for notifying observers in the next patch.

Comment 4

[Richard Newman [:rnewman]]

Attachment: Part 2: JS sync server. v1

Enough to implement the tests for the clients and bookmarks engine, and a few trivial tests of its own.

Comment 5

[Richard Newman [:rnewman]]

Attachment: Part 3: update Sync bookmark tests to use SyncServer. v1

Comment 6

[Gregory Szorc [:gps]]

I want this landed so bad. It would make my test code for send tab to device and add-on sync so much cleaner...

Comment 7

[Richard Newman [:rnewman]]

Attachment: Part 0b: convert test_clients_engine.js to Unix line endings. v1

This has been bugging me for ages, so time to fix it.

Comment 8

[Richard Newman [:rnewman]]

Attachment: Part 4: update test_clients_engine.js to use SyncServer. v1

Comment 9

[Philipp von Weitershausen [:philikon]]

Comment on attachment 562983 [details] [diff] [review]
Part 0: Content-Types, accessors, timestamp tracking. v1

>@@ -162,22 +164,71 @@ ServerWBO.prototype = {
>  * mapping of id -> ServerWBO objects.
>  * 
>  * Note that if you want these records to be accessible individually,
>  * you need to register their handlers with the server separately!
>  * 
>  * Passing `true` for acceptNew will allow POSTs of new WBOs to this
>  * collection. New WBOs will be created and wired in on the fly.
>  */
>-function ServerCollection(wbos, acceptNew) {
>+function ServerCollection(wbos, acceptNew, timestamp) {

Can you document the 'timestamp' parameter? Bonus points for converting that comment to JavaDoc style.

>+  /*
>+   * Track modified timestamp.
>+   * We can't just use the timestamps of contained WBOs: an empty collection
>+   * has a modified time.
>+   */
>+  this.timestamp = timestamp || new_timestamp();

<3

> }
> ServerCollection.prototype = {
> 
>+  /**
>+   * Convenience accessor for our WBO keys.
>+   * Excludes deleted items, of course.
>+   * filter applies to the WBO, not the id.
>+   */

Nit: JavaDoc style for the parameter.

>+  ids: function (filter) {

Nit: function name:

  ids: function ids(filter) {

I feel like this method should maybe be called .keys().

>+    return [id for ([id, wbo] in Iterator(this.wbos))
>+               if (wbo.payload &&
>+                   (!filter || filter(wbo)))];
>+  },

Why not use Array.filter here like you do in .objects()?

>+  /**
>+   * Convenience method to get an array of WBOs.
>+   * Optionally provide a filter function.
>+   *
>+   * @return an array of ServerWBOs.
>+   */
>+  objects: function (filter) {
>+    let os = [wbo for ([id, wbo] in Iterator(this.wbos))
>+              if (wbo.payload)];
>+    if (filter) {
>+      return os.filter(filter);
>+    }
>+    return os;
>+  },

I feel like this method should be called .wbos(), especially since you define a .wbo() method below.

Comment 10

[Philipp von Weitershausen [:philikon]]

Comment on attachment 562986 [details] [diff] [review]
Part 1: return deleted IDs from ServerCollection delete handler. v1

It's like somebody actually sat down and read a fucking spec! <333

>         case "DELETE":
>-          self.delete(options);
>+          _("Invoking ServerCollection.DELETE.");

Can we use log4moz please, instead of just dumping to stdout... All of the httpd.js sync server crap can probably share one logger identifier (Sync.Test.Server or whatever).

Comment 11

[Richard Newman [:rnewman]]

(In reply to Philipp von Weitershausen [:philikon] from comment #9)

> >+    return [id for ([id, wbo] in Iterator(this.wbos))
> >+               if (wbo.payload &&
> >+                   (!filter || filter(wbo)))];
> >+  },
> 
> Why not use Array.filter here like you do in .objects()?

Because the filter applies to the WBO, not the ID; we'd need to do something like

  Object.keys(this.wbos).filter(function (id) {
    let wbo = this.wbos[id];
    return wbo.payload && filter(wbo);
  }.bind(this));

with the appropriate check for no filter, of course.

The iterator gives us the objects without the additional lookup.

Comment 12

[Philipp von Weitershausen [:philikon]]

(In reply to Richard Newman [:rnewman] from comment #11)
> > Why not use Array.filter here like you do in .objects()?
> 
> Because the filter applies to the WBO, not the ID

Uh yeah makes sense.

Comment 13

[Philipp von Weitershausen [:philikon]]

Comment on attachment 562987 [details] [diff] [review]
Part 2: JS sync server. v1

Very, very nice!

One general note upfront: I would prefer if we used log4moz instead of just dumping to stdout.

>+var SyncServerCallback = {
>+  onCollectionDeleted: function onCollectionDeleted(user, collection) {},
>+  onItemDeleted: function onItemDeleted(user, collection, wboID) {}
>+};

s/var/let/ ?

>+SyncServer.prototype = {
>+  port:   8080,
>+  server: null,    // nsHttpServer.
>+  users:  null,    // Map of username => {collections, password}.
>+
>+  start: function start(port, cb) {
...
>+  stop: function stop(cb) {

Document plz.

>+      _("SyncServer: Warning: server not running. Can't stop me now!");

'cause I'm having a good time

>+  /**
>+   * This is invoked by the nsHttpServer. `this` is bound to the SyncServer;
>+   * `handler` is the nsHttpServer's handler.
>+   *
>+   * TODO: need to use the correct Sync API response codes and errors here.

See also my comments below.

>+   * TODO: auth.

TODO: username in path against username in BasicAuth

>+   */
>+  handleDefault: function handleDefault(handler, req, resp) {
>+    _("SyncServer: Handling request: " + req.method + " " + req.path);
>+    let parts = this.pathRE.exec(req.path);
>+    if (!parts) {
>+      _("SyncServer: Bad request: bad URL " + req.path);
>+      throw HTTP_400;

This should be a 404!

>+    }
>+
>+    let [all, version, username, first, rest] = parts;
>+    if (version != "1.1") {

const the version please.

>+      _("SyncServer: Bad request: unknown version.");
>+      throw HTTP_400;
>+    }

The current Sync server returns a 404 for this, so I think this too should be a 404. We should probably retrofit the spec for this to mandate 404 in this case and all others (see below).

>+    if (!this.userExists(username)) {
>+      _("SyncServer: Bad request: unknown user.");
>+      throw HTTP_400;
>+    }

This should be a 401 because the server doesn't tell you whether a user exists or not.

>+    // Hand off to the appropriate handler for this path component.
>+    if (first in this.toplevelHandlers) {
>+      let handler = this.toplevelHandlers[first];
>+      return handler.call(this, handler, req, resp, version, username, rest);
>+    }
>+    _("SyncServer: Bad request: unknown top-level " + first);
>+    throw HTTP_400;

404!

(Note that 400 has a special meaning in our API. It should always be accompanied by a response code: http://docs.services.mozilla.com/respcodes.html)

>+  /**
>+   * Collection of the handler methods we use for top-level path components.
>+   */
>+  toplevelHandlers: {
>+    "storage": function handleStorage(handler, req, resp, version, username, rest) {
>+      let match = this.storageRE.exec(rest);
>+      if (!match) {
>+        _("SyncServer: Unknown storage operation " + rest);
>+        throw HTTP_400;
>+      }

404!

>+      let [all, collection, wboID] = match;
>+      let coll = this.getCollection(username, collection);
>+      let respond = this.respond.bind(this, req, resp);
>+      switch (req.method) {
>+        case "GET":
>+          if (!coll) {
>+            // *cries inside*: Bug 687299.
>+            respond(200, "OK", "[]");
>+            return;
>+          }
>+          if (!wboID) {
>+            return coll.collectionHandler(req, resp);
>+          }
>+          let wbo = coll.wbos[wboID];
>+          if (!wbo) {
>+            respond(404, "Not found", "Not found"); // TODO: body code.

What body code?

>+          // Spot if this is a DELETE for some IDs, and don't blow away the
>+          // whole collection!
>+          // TODO: less hacky method.
>+          if (-1 == req.queryString.indexOf("ids=")) {
>+            // When you delete the entire collection, we drop it.
>+            _("Deleting entire collection.");
>+            delete this.users[username].collections[collection];
>+            this.callback.onCollectionDeleted(username, collection);
>+          }

else?!? At least stub it out with a TODO please.

>+    "info": function handleInfo(handler, req, resp, version, username, rest) {
>+      switch (rest) {
>+        case "collections":
>+          let body = JSON.stringify(this.infoCollections(username));
>+          this.respond(req, resp, 200, "OK", body, {
>+            "Content-Type": "application/json"
>+          });
>+          return;

at least stub out

  case "collection_usage":
  case "collection_counts":
  case "quota":
    //TODO
    this.respond(req, resp, 200, "OK", "TODO, {});
    return;

or something like that.

>+        default:
>+          // TODO
>+          _("SyncServer: Unknown info operation " + rest);
>+          throw HTTP_400;

404!


r- because I'd like to get the HTTP status codes fixed and the various missing pieces stubbed out with TODO. Otherwise: <3

Comment 14

[Philipp von Weitershausen [:philikon]]

Comment on attachment 562988 [details] [diff] [review]
Part 3: update Sync bookmark tests to use SyncServer. v1

>     // Create a server record for folder1 where we flip the order of
>     // the children.
>     let folder1_payload = store.createRecord(folder1_guid).cleartext;
>     folder1_payload.children.reverse();
>-    collection.wbos[folder1_guid] = new ServerWBO(
>-      folder1_guid, encryptPayload(folder1_payload));
>+    server.insertWBO("foo", "bookmarks", new ServerWBO(
>+      folder1_guid, encryptPayload(folder1_payload)));

Why not use createContents() here? Avoids having to make a new ServerWBO() here, hides it as an implementation detail.

>     // Create a bogus record that when synced down will provoke a
>     // network error which in turn provokes an exception in _processIncoming.
>     const BOGUS_GUID = "zzzzzzzzzzzz";
>-    let bogus_record = collection.wbos[BOGUS_GUID]
>-      = new ServerWBO(BOGUS_GUID, "I'm a bogus record!");
>+    let bogus_record = server.insertWBO("foo", "bookmarks",
>+      new ServerWBO(BOGUS_GUID, "I'm a bogus record!"));

Ditto.

>+    function isBookmark(wbo) {
>+      return wbo.type == "bookmark";
>+    }
>+    function isFolder(wbo) {
>+      return ((wbo.type == "folder") &&
>+              (wbo.id   != "menu") &&
>+              (wbo.id   != "toolbar"));
>+    }
>+
>     _("Verify that there's only one bookmark on the server, and it's Firefox.");
>     // Of course, there's also the Bookmarks Toolbar and Bookmarks Menu...
>-    wbos = [JSON.parse(JSON.parse(wbo.payload).ciphertext)
>-            for ([id, wbo] in Iterator(collection.wbos))
>-            if (wbo.payload)];
>+    let bookmarkWBOs    = server.user("foo").collection("bookmarks").payloads();
>+    let bookmarkItems   = bookmarkWBOs.filter(isBookmark);
>+    let bookmarkFolders = bookmarkWBOs.filter(isFolder);

I would prefer slightly shorter but clearer variable names:

    let payloads     = server.user("foo").collection("bookmarks").payloads();
    let bookmarkWBOs = payloads.filter(isBookmark);
    let folderWBOs   = payloads.filter(isFolder);

I would also prefer if we could inline the filter functions. Less jumping around when reading.

r=me with that.

Comment 15

[Philipp von Weitershausen [:philikon]]

Comment on attachment 563117 [details] [diff] [review]
Part 4: update test_clients_engine.js to use SyncServer. v1

>+  function check_clients_count(c) {
>+    let coll = user.collection("clients");
>+    // Treat a non-existent collection as empty.
>+    do_check_eq(c, coll ? coll.count() : 0);
>   }

</3 one letter variables. s/c/expectedCount/ please.

Also, if you invoke do_check_eq with Components.caller.stack as the third argument, the test output will refer to the check_clients_count() call site (which is where you want to be looking when the check fails.)

Comment 16

[Richard Newman [:rnewman]]

(In reply to Philipp von Weitershausen [:philikon] from comment #13)
 
> One general note upfront: I would prefer if we used log4moz instead of just
> dumping to stdout.

Done.
 

> >+          // Spot if this is a DELETE for some IDs, and don't blow away the
> >+          // whole collection!
> >+          // TODO: less hacky method.
> >+          if (-1 == req.queryString.indexOf("ids=")) {
> >+            // When you delete the entire collection, we drop it.
> >+            _("Deleting entire collection.");
> >+            delete this.users[username].collections[collection];
> >+            this.callback.onCollectionDeleted(username, collection);
> >+          }
> 
> else?!? At least stub it out with a TODO please.

No else or TODO needed. The trick here is that we already handled (in the line above) deleting the WBOs by invoking the deleted collection's handler. However, in the case of

  DELETE storage/foobar

we also need to remove foobar from the collections map. This clause tries to differentiate the above request from

  DELETE storage/foobar?ids=foo,baz

and do the right thing.

Comment 17

[Philipp von Weitershausen [:philikon]]

(In reply to Richard Newman [:rnewman] from comment #16)
> > >+          // Spot if this is a DELETE for some IDs, and don't blow away the
> > >+          // whole collection!
> > >+          // TODO: less hacky method.
> > >+          if (-1 == req.queryString.indexOf("ids=")) {
> > >+            // When you delete the entire collection, we drop it.
> > >+            _("Deleting entire collection.");
> > >+            delete this.users[username].collections[collection];
> > >+            this.callback.onCollectionDeleted(username, collection);
> > >+          }
> > 
> > else?!? At least stub it out with a TODO please.
> 
> No else or TODO needed. The trick here is that we already handled (in the
> line above) deleting the WBOs by invoking the deleted collection's handler.

Uhhhh riiiight! Can you add a comment there that explains that? :)

Comment 18

[Richard Newman [:rnewman]]

Attachment: Part 2: JS sync server. v2

Addressed review comments.

Comment 19

[Richard Newman [:rnewman]]

Attachment: Part 5: implement X-Weave-Records header. v1

Just for chuckles.

Comment 20

[Richard Newman [:rnewman]]

Attachment: Part 0a: Content-Types, accessors, timestamp tracking. v2

wbos/_wbos change. Added insert and insertWBO, adjusted tests to match. Other review nits.

Comment 21

[Richard Newman [:rnewman]]

Attachment: Part 0a: Content-Types, accessors, timestamp tracking. v3

Not sure I qfolded everything.

Comment 22

[Richard Newman [:rnewman]]

Attachment: Part 2: JS sync server. v3

Rebasing on top of part 0 changes.

Comment 23

[Richard Newman [:rnewman]]

Comment on attachment 563493 [details] [diff] [review]
Part 2: JS sync server. v3

Forgot to flag r?.

I'm not going to re-upload the other patches after rebasing; they're all simple enough, and tests pass.

Comment 24

[Philipp von Weitershausen [:philikon]]

Comment on attachment 563493 [details] [diff] [review]
Part 2: JS sync server. v3

>+  /**
>+   * Start the SyncServer's underlying HTTP server.
>+   *
>+   * @param port  the numeric port on which to start. A falsy value implies the
>+   *              default (8080).
>+   * @param cb    a callback function (of no arguments) which is invoked after
>+   *              startup.
>+   */

Generally our style is

  @param foo
         This is a bar.

That way you're also less likely to hit the 80 char limit. Please fix this is the other places too.

r=me with that.

Comment 25

[Philipp von Weitershausen [:philikon]]

Comment on attachment 563317 [details] [diff] [review]
Part 5: implement X-Weave-Records header. v1

>+      // Use options as a backchannel to report count.
>+      options.recordCount = data.length;

Neat! http://www.youtube.com/watch?v=A-Y6pAMa_VY

Comment 26

[Philipp von Weitershausen [:philikon]]

Laaaaaand eeeeeet

Comment 27

[Richard Newman [:rnewman]]

http://twinql.com/jpg/g6.jpg

https://hg.mozilla.org/services/services-central/rev/5f872e73513f
https://hg.mozilla.org/services/services-central/rev/97d04e6f59f2
https://hg.mozilla.org/services/services-central/rev/98ccacadc2de
https://hg.mozilla.org/services/services-central/rev/4c2d63309450
https://hg.mozilla.org/services/services-central/rev/19f09a29d3f6
https://hg.mozilla.org/services/services-central/rev/402a48392dbd
https://hg.mozilla.org/services/services-central/rev/49de539c895c

I will write up some docs.

Comment 28

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/mozilla-central/rev/5f872e73513f
https://hg.mozilla.org/mozilla-central/rev/97d04e6f59f2
https://hg.mozilla.org/mozilla-central/rev/98ccacadc2de
https://hg.mozilla.org/mozilla-central/rev/4c2d63309450
https://hg.mozilla.org/mozilla-central/rev/19f09a29d3f6
https://hg.mozilla.org/mozilla-central/rev/402a48392dbd
https://hg.mozilla.org/mozilla-central/rev/49de539c895c