Closed Bug 673548 Opened 9 years ago Closed 8 years ago

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

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: philikon, Assigned: rnewman)

References

Details

(Whiteboard: [fixed in services][qa-])

Attachments

(7 files, 5 obsolete files)

1.79 KB, patch
philikon
: review+
Details | Diff | Splinter Review
13.07 KB, patch
philikon
: review+
Details | Diff | Splinter Review
34.26 KB, patch
philikon
: review+
Details | Diff | Splinter Review
12.47 KB, patch
philikon
: review+
Details | Diff | Splinter Review
3.41 KB, patch
philikon
: review+
Details | Diff | Splinter Review
53.27 KB, patch
philikon
: review+
Details | Diff | Splinter Review
20.47 KB, patch
philikon
: review+
Details | Diff | Splinter Review
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.
Assignee: nobody → rnewman
Attached patch WIP. v1 (obsolete) — Splinter Review
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..., ...
Attachment #558962 - Flags: feedback?(philipp)
Attachment #558962 - Attachment is obsolete: true
Attachment #558962 - Flags: feedback?(philipp)
Attachment #562983 - Flags: review?(philipp)
Used for notifying observers in the next patch.
Attachment #562986 - Flags: review?(philipp)
Attached patch Part 2: JS sync server. v1 (obsolete) — Splinter Review
Enough to implement the tests for the clients and bookmarks engine, and a few trivial tests of its own.
Attachment #562987 - Flags: review?(philipp)
Attachment #562986 - Attachment description: Part 1: return deleted IDs from ServerCollection delete handler. → Part 1: return deleted IDs from ServerCollection delete handler. v1
I want this landed so bad. It would make my test code for send tab to device and add-on sync so much cleaner...
This has been bugging me for ages, so time to fix it.
Attachment #563114 - Flags: review?(philipp)
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.
Attachment #562983 - Flags: review?(philipp) → review+
Attachment #563114 - Flags: review?(philipp) → review+
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).
Attachment #562986 - Flags: review?(philipp) → review+
(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.
(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 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
Attachment #562987 - Flags: review?(philipp) → review-
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.
Attachment #562988 - Flags: review?(philipp) → review+
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.)
Attachment #563117 - Flags: review?(philipp) → review+
(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.
(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? :)
Attached patch Part 2: JS sync server. v2 (obsolete) — Splinter Review
Addressed review comments.
Attachment #562987 - Attachment is obsolete: true
Attachment #563316 - Flags: review?(philipp)
Just for chuckles.
Attachment #563317 - Flags: review?(philipp)
wbos/_wbos change. Added insert and insertWBO, adjusted tests to match. Other review nits.
Attachment #562983 - Attachment is obsolete: true
Attachment #563459 - Flags: review?(philipp)
Not sure I qfolded everything.
Attachment #563459 - Attachment is obsolete: true
Attachment #563459 - Flags: review?(philipp)
Attachment #563492 - Flags: review?(philipp)
Rebasing on top of part 0 changes.
Attachment #563316 - Attachment is obsolete: true
Attachment #563316 - Flags: review?(philipp)
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.
Attachment #563493 - Flags: review?(philipp)
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.
Attachment #563493 - Flags: review?(philipp) → review+
Attachment #563492 - Flags: review?(philipp) → review+
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
Attachment #563317 - Flags: review?(philipp) → review+
Laaaaaand eeeeeet
Blocks: 690984
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.