744323 - Port test HTTP server to protocol 2.0

Status: RESOLVED FIXED

(Cloud Services :: Firefox: Common, defect)

Description

[Gregory Szorc [:gps]]

As part of supporting protocol 2.0, I plan to make a *copy* of the existing test JS HTTP Sync server in services/common and make the necessary changes to support protocol 2.0.

This is a prerequisite to writing the standalone storage service 2.0 client library.

Comment 1

[Gregory Szorc [:gps]]

Attachment: Port test storage HTTP server to protocol 2.0

Sync has a very basic JS implementation of the storage server, version 1.1.

In this bug, I've taken that code and ported it to version 2.0 of the storage service. Not all features are implemented and I could probably clean up the code a bit more, so I'm only asking for feedback.

It may be helpful to diff the JS file containing the server with services/sync/tests/unit/head_http_server.js from services-central to see what's changed.

Comment 2

[Gregory Szorc [:gps]]

Comment on attachment 613889 [details] [diff] [review]
Port test storage HTTP server to protocol 2.0

I'm giving this patch some love today. Will hopefully drop something without missing features next.

Comment 3

[Gregory Szorc [:gps]]

Attachment: Port test storage HTTP server to protocol 2.0, v2

Refactored a bunch of things, especially the tests.

Main additions were support for If-Modified-Since and If-Unmodified-Since on individual BSO's. Although, I'm still missing support in the GET /storage/collection case. I figure that would be a good follow-up.

I figure if this is "good enough" we can check it in now and file follow-up bugs for feature improvement later. If there is anything too wrong, we should probably identity it now, however.

rfkelly: not sure how well you can read JS. I'm mostly interested in identifying where I don't follow the spec or match Python server's semantics.

Comment 4

[Ryan Kelly [:rfkelly]]

Comment on attachment 614267 [details] [diff] [review]
Port test storage HTTP server to protocol 2.0, v2

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

Looks good to me, some specific comments inline.  General notes:
 * X-Timestamp is supposed to be "atomic" throughout a request, so that e.g the X-Timestamp value on a PUT request will equal the modified time recorded for that BSO.
 * X-If-Modified-Since is allowed on all GET requests, including to /storage/collection and to /info/collections.
 * Likewise, X-If-Unmodified-Since is supported on POST /storage/collection as well as on PUT /storage/collection/id.

I'm going to try to run my python functional tests against this server, just to see how well they line up at this stage.

::: services/common/tests/unit/head_storage_server.js
@@ +91,5 @@
> +    delete this.modified;
> +  },
> +
> +  commonHandler: function commonHandler(request, response) {
> +    response.setHeader("X-Timestamp", "" + new_timestamp(), false);

The spec requires that X-Timestamp match the modified time on any BSOs touched during a request.  Consider something like the (r.newModified ? r.newModified : new_timestamp()) idiom you use further down

@@ +360,5 @@
> +      // Use options as a backchannel to report count.
> +      options.recordCount = data.length;
> +    } else {
> +      let data = [id for ([id, bso] in Iterator(this._bsos))
> +                     if (this._inResultSet(bso, options))];

Why not "if (bso.modified && this._inResultSet(bso, options)" like used above?

@@ +387,5 @@
> +        this.insertBSO(bso);
> +      }
> +      if (bso) {
> +        bso.payload = record.payload;
> +        bso.modified = new_timestamp();

Technically all BSOs created during a POST must be assigned the same timestamp.

@@ +762,5 @@
> +
> +  /*
> +   * Regular expressions for splitting up Storage request paths.
> +   * Storage URLs are of the form:
> +   *   /$apipath/$userid/$version/$further

$apipath/$version/$userid/$further

@@ +779,5 @@
> +   *
> +   * Path: [all, version, first, rest]
> +   * Storage: [all, collection?, id?]
> +   */
> +  pathRE: /^\/([0-9]+(?:\.[0-9]+)?)(?:\/([a-zA-Z0-9]+)\/([^\/]+)(?:\/(.+))?)?$/,

The python server explicitly limits the userid component to an integer

Comment 5

[Gregory Szorc [:gps]]

> I'm going to try to run my python functional tests against this server, just
> to see how well they line up at this stage.

You can do that?! \o/

Unfortunately, getting this server to run might be a little cumbersome. You'll have to launch xpcshell with a special combination of options and "header" code to get the environment right. While I don't want to discourage you from learning, you might find it in the best interest of your sanity to let me figure that out for you :)

I'd definitely be interested in hooking up a make target to launch a server instance. Maybe I can do that for the next patch...

Comment 6

[Richard Newman [:rnewman]]

(In reply to Gregory Szorc [:gps] from comment #5)

> Unfortunately, getting this server to run might be a little cumbersome.
> You'll have to launch xpcshell with a special combination of options and
> "header" code to get the environment right.

... you mean... like... a test that doesn't shut down the server?

:P

Comment 7

[Ryan Kelly [:rfkelly]]

(In reply to Gregory Szorc [:gps] from comment #5)
> > I'm going to try to run my python functional tests against this server, just
> > to see how well they line up at this stage.
> 
> You can do that?! \o/

Heh, just note that I said *try*...

It should be possible as described in Bug 731511, although I'll probably have to fiddle with it a bit to get the auth right.

> Unfortunately, getting this server to run might be a little cumbersome.
> You'll have to launch xpcshell with a special combination of options and
> "header" code to get the environment right. While I don't want to discourage
> you from learning, you might find it in the best interest of your sanity to
> let me figure that out for you :)

I got s-c built and have started playing with running the tests, but yes, there's a lot for me to learn.  If you are able to make it easy to run, so much the better for me :-)

Comment 8

[Gregory Szorc [:gps]]

As rnewman said, changing an existing test to not shut down the server would probably work just fine for your purposes.

Comment 9

[Richard Newman [:rnewman]]

Comment on attachment 614267 [details] [diff] [review]
Port test storage HTTP server to protocol 2.0, v2

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

Further to Ryan's comments.

This is not the most thorough review I've ever done, but then I'm pretty exhausted. Please forgive me. Most of this code looks reused, so that gives me hope.

::: services/common/tests/unit/head_storage_server.js
@@ +154,5 @@
> +
> +      if (reqModified < this.modified) {
> +        response.setStatusLine(request.httpVersion, 412, "Precondition Failed");
> +        return;
> +      }

Perhaps log if it's greater than?

::: services/common/tests/unit/test_storage_server.js
@@ +26,5 @@
> + */
> +function validateResponse(response) {
> +  do_check_true("x-timestamp" in response.headers);
> +
> +  let cl;

You can move this inside the `if`.

@@ +194,5 @@
> +  let server = new StorageServer();
> +  server.registerUser("123", "password");
> +  server.startSynchronous(PORT);
> +
> +  let path = "/2.0/123/info/collections";

Seeing that 2.0 really throws me!

Comment 10

[Gregory Szorc [:gps]]

Attachment: Port test storage HTTP server to protocol 2.0, v3

This now passes the Python functional tests \o/

Comment 11

[Gregory Szorc [:gps]]

Attachment: Port test storage HTTP server to protocol 2.0, v4

I don't have chicken noodle soup for sickly rnewman. But I do have a fresh patch for review!

Changes since last revision:

* Converted patch to Mercurial format
* Support recent changes to Python/spec (all functional tests pass)
* Changed standalone server launching so most logic is in a Python file, not Makefile

In the long run, I'd like to refactor this further. But, that would require bug 485255 and bug 748490. That can be a follow-up. I think this patch is good enough (especially since it is test code).

Comment 12

[Gregory Szorc [:gps]]

I'm going to refactor this as a testing module. Go big or go home.

Comment 13

[Gregory Szorc [:gps]]

Nevermind. Went down that path and it is too much work. Will make a good follow-up.

Comment 14

[Richard Newman [:rnewman]]

Comment on attachment 622556 [details] [diff] [review]
Port test storage HTTP server to protocol 2.0, v4

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

I'm not qualified to review run_storage_server.py, so please have a Pythonista look that over.

First part: r+ on everything but head_storage_server.py. That's up next…

::: services/common/tests/unit/test_storage_server.js
@@ +14,5 @@
> +  let url = "http://127.0.0.1:" + PORT + path;
> +  _("url: " + url);
> +  let req = new RESTRequest(url);
> +
> +  let header = basic_auth_header(user || "123", password || "password");

Pull out DEFAULT_USER, DEFAULT_PASSWORD.

@@ +27,5 @@
> + */
> +function validateResponse(response) {
> +  do_check_true("x-timestamp" in response.headers);
> +
> +  let cl;

This can move inside the `if`.

@@ +68,5 @@
> +  return error;
> +}
> +
> +/**
> + * Helper function to synchronously do a PUT request.

Can we match the PUT and GET comments, please, and polish up the English? :D

@@ +92,5 @@
> +
> +  let error = cb.wait();
> +
> +  if (!error) {
> +    validateResponse(request.response);

This pattern is repeated three times. How about

function waitForResponse(cb, request) {
  let error = cb.wait();
  if (!error) {
    validateResponse(request.response);
  }
  return error;
}

::: services/common/utils.js
@@ +332,5 @@
> +    if (!value) {
> +      return;
> +    }
> +
> +    return;

This can't be right.

@@ +339,5 @@
> +    }
> +
> +    if (value < 0) {
> +      throw new Error("Timestamp value is negative: " + value);
> +    }

I'd probably put these cheaper range checks before the floor/ceil check.

Comment 15

[Gregory Szorc [:gps]]

Tarek: Pourriez-vous s'il vous plaît examiner le Python?

Comment 16

[Gregory Szorc [:gps]]

rnewman: the in-tree JS tests are redundant with the Python functional tests. Therefore, they are quite minimal. As much as I would like to port the entire test suite to JS or devise a way to run the Python functional tests against the JS server on every checkin, I don't have the time. The former I think is a waste of time. The latter would make a good follow-up bug.

Comment 17

[Richard Newman [:rnewman]]

(In reply to Gregory Szorc [:gps] from comment #16)
> The former I think is a waste of time. The latter would make a good follow-up
> bug.

Agreed.

Comment 18

[Richard Newman [:rnewman]]

Comment on attachment 622556 [details] [diff] [review]
Port test storage HTTP server to protocol 2.0, v4

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

This seems curiously familiar :D

::: services/common/tests/unit/head_storage_server.js
@@ +258,5 @@
> +   * Track modified timestamp.
> +   * We can't just use the timestamps of contained BSOs: an empty collection
> +   * has a modified time.
> +   */
> +   CommonUtils.ensureMillisecondsTimestamp(timestamp);

Indentation.

@@ +265,5 @@
> +  this._log = Log4Moz.repository.getLogger(STORAGE_HTTP_LOGGER);
> +}
> +StorageServerCollection.prototype = {
> +  BATCH_MAX_COUNT: 100,
> +  BATCH_MAX_SIZE: 1024 * 1024,

Indicate units here. Bytes? Chars? Records?

@@ +285,5 @@
> +      size += bso.payload.length;
> +    }
> +
> +    return size;
> +  },

Newline after here.

@@ +375,5 @@
> +  remove: function remove(id) {
> +    delete this._bsos[id];
> +  },
> +
> +  _inResultSet: function(bso, options) {

Name.

@@ +393,5 @@
> +      }
> +    }
> +
> +    if (options.older) {
> +      if (bso.modified >= options.older) {

Docs say:

  older: a timestamp in milliseconds. Only objects that were last modified before this time will be returned.
  newer: a timestamp in milliseconds. Only objects that were last modified after this time will be returned.

Please consult the main server code to decide whether these should be inclusive (one or both), and ensure that the docs are adjusted to mention inclusivity.

@@ +421,5 @@
> +
> +    return true;
> +  },
> +
> +  count: function(options) {

Name.

@@ +432,5 @@
> +    }
> +    return c;
> +  },
> +
> +  get: function(options) {

Name.

@@ +489,5 @@
> +
> +    return data;
> +  },
> +
> +  post: function(input, timestamp) {

Name.

@@ +552,5 @@
> +            bso.sortindex = parseInt(record.sortindex, 10);
> +          }
> +
> +        } else {
> +          failed[record.id] = "no bso configured";

Ugh at mixed conventions for error messages. ("Payload is not a string!" versus "no bso configured".)

@@ +846,5 @@
> +  let handler = this.server._handler;
> +  handler._handleDefault = this.handleDefault.bind(this, handler);
> +}
> +StorageServer.prototype = {
> +  DEFAULT_QUOTA: 1024 * 1024,

Unit.

Comment 19

[Ryan Kelly [:rfkelly]]

(In reply to Richard Newman [:rnewman] from comment #18)
> @@ +393,5 @@
> > +      }
> > +    }
> > +
> > +    if (options.older) {
> > +      if (bso.modified >= options.older) {
> 
> Docs say:
> 
>   older: a timestamp in milliseconds. Only objects that were last modified
> before this time will be returned.
>   newer: a timestamp in milliseconds. Only objects that were last modified
> after this time will be returned.
> 
> Please consult the main server code to decide whether these should be
> inclusive (one or both), and ensure that the docs are adjusted to mention
> inclusivity.

They are both interpreted as exclusive by the server.  Proposal:  update docs to say "strictly before" and "strictly after".

Comment 20

[Richard Newman [:rnewman]]

(In reply to Ryan Kelly [:rfkelly] from comment #19)

> They are both interpreted as exclusive by the server.  Proposal:  update
> docs to say "strictly before" and "strictly after".

Then if this chunk of code passes the Python functional tests, we need some more functional tests :D

Comment 21

[Ryan Kelly [:rfkelly]]

Well, that's embarrassing...here is the code that's supposed to be testing this, from https://github.com/mozilla-services/server-syncstorage/blob/master/syncstorage/tests/functional/test_storage.py#L141:

        res = self.app.get(self.root + '/storage/col2?newer=%s' % ts)
        res = res.json["items"]
        try:
            self.assertEquals(res, ['129'])
        except AssertionError:
            # XXX not sure why this fails sometimes
            pass

Filed Bug 757592 to track fixing of this server-side.

Comment 22

[Ryan Kelly [:rfkelly]]

Latest server-syncstorage master now contains some extra tests for strictness of "older" and "newer".

Comment 23

[Gregory Szorc [:gps]]

Attachment: Port test storage HTTP server to protocol 2.0, v5

Didn't incorporate feedback yet. Instead, refactored slightly to use httpd.js as a module (bug 755196 and bug 757860) and to expose this server as a module itself. Along the way, I changed the standalone server code slightly so the Python driver could be reused for the AITC standalone server.

Because we are now using testing JS modules, this is blocked from landing until the buildbot infrastructure is fully configured. That should hopefully happen sometime in the next week or so.

rnewman: I carried the r+ forward because I don't feel enough changed. Look at the intradiff. If it is above your threshold, go ahead and re-review.

tarek: I'd still like your feedback on le Python. It isn't the best Python in the world. But, it doesn't need to be since it is test code.

Comment 24

[Tarek Ziadé (:tarek)]

Comment on attachment 626519 [details] [diff] [review]
Port test storage HTTP server to protocol 2.0, v5

[...]
>diff --git a/services/common/tests/run_server.py b/services/common/tests/run_server.py
>new file mode 100755
>--- /dev/null
>+++ b/services/common/tests/run_server.py
>@@ -0,0 +1,78 @@
>+#!/usr/bin/python
>+
>+# This Source Code Form is subject to the terms of the Mozilla Public
>+# License, v. 2.0. If a copy of the MPL was not distributed with this file,
>+# You can obtain one at http://mozilla.org/MPL/2.0/.
>+
>+from shutil import rmtree
>+from subprocess import Popen
>+from sys import argv
>+from sys import exit
>+from tempfile import mkdtemp
>+
>+DEFAULT_PORT = 8080
>+DEFAULT_HOSTNAME = 'localhost'
>+
>+def run_server(srcdir, objdir, js_file, hostname=None, port=None):
>+    if hostname is None:
>+        hostname = DEFAULT_HOSTNAME
>+
>+    if port is None:
>+        port = DEFAULT_PORT

^ since you have a default value for hostname+port, instead of this condition, 
  just adapt your function signature like this:


def run_server(srcdir, objdir, js_file, hostname=DEFAULT_HOSTNAME, port=DEFAULT_PORT):


>+
>+    dist_dir = '%s/dist' % objdir
>+    head_dir = '%s/services/common/tests/unit' % srcdir
>+
[..]
>+if __name__ == '__main__':
>+    if len(argv) == 4:
>+        exit(run_server(argv[1], argv[2], argv[3]))
>+
>+    if len(argv) == 5:
>+        exit(run_server(argv[1], argv[2], argv[3], port=argv[4]))
>+
>+    if len(argv) == 6:
>+        exit(run_server(argv[1], argv[2], argv[3], port=argv[4],
>+            hostname=argv[5]))
>+
>+    print 'Usage: %s /path/to/srcdir /path/to/objdir [js_file] [port] [address]'
>+    exit(1)


You've reached point where you should use a command line args library, to avoid 
doing all that low level work. You'll also get type checking for free.

Argparse is a good one (in the stdlib for py 2.7 as a standalone lib for 2.6)

See this example: https://github.com/mozilla-services/circus/blob/master/circus/circusd.py#L71

Full doc: http://docs.python.org/dev/library/argparse.html

otherwise, lokks good



>diff --git a/services/common/tests/run_storage_server.js b/services/common/tests/run_storage_server.js
>new file mode 100644
>--- /dev/null
>+++ b/services/common/tests/run_storage_server.js
>@@ -0,0 +1,25 @@
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+/**
>+ * This file runs a Storage Service server.
>+ *
>+ * It is meant to be executed with an xpcshell.
>+ *
>+ * The Makefile in this directory contains a target to run it:
>+ *
>+ *   $ make storage-server
>+ */
>+
>+Cu.import("resource://testing-common/services-common/storageserver.js");
>+
>+initTestLogging();
>+
>+let server = new StorageServer();
>+server.allowAllUsers = true;
>+server.startSynchronous(SERVER_PORT);
>+_("Storage server started on port " + SERVER_PORT);
>+
>+// Launch the thread manager.
>+_do_main();
>diff --git a/services/common/tests/unit/head_helpers.js b/services/common/tests/unit/head_helpers.js
>--- a/services/common/tests/unit/head_helpers.js
>+++ b/services/common/tests/unit/head_helpers.js
>@@ -1,14 +1,15 @@
> /* This Source Code Form is subject to the terms of the Mozilla Public
>  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>  * You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> Cu.import("resource://testing-common/httpd.js");
> Cu.import("resource://services-common/log4moz.js");
>+Cu.import("resource://services-common/utils.js");
> 
> let btoa = Cu.import("resource://services-common/log4moz.js").btoa;
> let atob = Cu.import("resource://services-common/log4moz.js").atob;
> 
> function do_check_empty(obj) {
>   do_check_attribute_count(obj, 0);
> }
> 
>@@ -85,17 +86,17 @@
>   for (let path in handlers) {
>     server.registerPathHandler(path, handlers[path]);
>   }
>   try {
>     server.start(port);
>   } catch (ex) {
>     _("==========================================");
>     _("Got exception starting HTTP server on port " + port);
>-    _("Error: " + Utils.exceptionStr(ex));
>+    _("Error: " + CommonUtils.exceptionStr(ex));
>     _("Is there a process already listening on port " + port + "?");
>     _("==========================================");
>     do_throw(ex);
>   }
> 
>   return server;
> }
> 
>@@ -113,24 +114,17 @@
>   };
> }
> 
> /*
>  * Read bytes string from an nsIInputStream.  If 'count' is omitted,
>  * all available input is read.
>  */
> function readBytesFromInputStream(inputStream, count) {
>-  var BinaryInputStream = Components.Constructor(
>-      "@mozilla.org/binaryinputstream;1",
>-      "nsIBinaryInputStream",
>-      "setInputStream");
>-  if (!count) {
>-    count = inputStream.available();
>-  }
>-  return new BinaryInputStream(inputStream).readBytes(count);
>+  return CommonUtils.readBytesFromInputStream(inputStream, count);
> }
> 
> /*
>  * Ensure exceptions from inside callbacks leads to test failures.
>  */
> function ensureThrows(func) {
>   return function() {
>     try {
>diff --git a/services/common/tests/unit/head_http.js b/services/common/tests/unit/head_http.js
>new file mode 100644
>--- /dev/null
>+++ b/services/common/tests/unit/head_http.js
>@@ -0,0 +1,29 @@
>+ /* Any copyright is dedicated to the Public Domain.
>+   http://creativecommons.org/publicdomain/zero/1.0/ */
>+
>+Cu.import("resource://services-common/utils.js");
>+
>+function basic_auth_header(user, password) {
>+  return "Basic " + btoa(user + ":" + CommonUtils.encodeUTF8(password));
>+}
>+
>+function basic_auth_matches(req, user, password) {
>+  if (!req.hasHeader("Authorization")) {
>+    return false;
>+  }
>+
>+  let expected = basic_auth_header(user, CommonUtils.encodeUTF8(password));
>+  return req.getHeader("Authorization") == expected;
>+}
>+
>+function httpd_basic_auth_handler(body, metadata, response) {
>+  if (basic_auth_matches(metadata, "guest", "guest")) {
>+    response.setStatusLine(metadata.httpVersion, 200, "OK, authorized");
>+    response.setHeader("WWW-Authenticate", 'Basic realm="secret"', false);
>+  } else {
>+    body = "This path exists and is protected - failed";
>+    response.setStatusLine(metadata.httpVersion, 401, "Unauthorized");
>+    response.setHeader("WWW-Authenticate", 'Basic realm="secret"', false);
>+  }
>+  response.bodyOutputStream.write(body, body.length);
>+}
>diff --git a/services/common/tests/unit/test_load_modules.js b/services/common/tests/unit/test_load_modules.js
>--- a/services/common/tests/unit/test_load_modules.js
>+++ b/services/common/tests/unit/test_load_modules.js
>@@ -6,14 +6,23 @@
>   "log4moz.js",
>   "preferences.js",
>   "rest.js",
>   "stringbundle.js",
>   "tokenserverclient.js",
>   "utils.js",
> ];
> 
>+const test_modules = [
>+  "storageserver.js",
>+];
>+
> function run_test() {
>   for each (let m in modules) {
>     let resource = "resource://services-common/" + m;
>     Components.utils.import(resource, {});
>   }
>+
>+  for each (let m in test_modules) {
>+    let resource = "resource://testing-common/services-common/" + m;
>+    Components.utils.import(resource, {});
>+  }
> }
>diff --git a/services/common/tests/unit/test_storage_server.js b/services/common/tests/unit/test_storage_server.js
>new file mode 100644
>--- /dev/null
>+++ b/services/common/tests/unit/test_storage_server.js
>@@ -0,0 +1,514 @@
>+/* Any copyright is dedicated to the Public Domain.
>+   http://creativecommons.org/publicdomain/zero/1.0/ */
>+
>+Cu.import("resource://services-common/async.js");
>+Cu.import("resource://services-common/rest.js");
>+Cu.import("resource://services-common/utils.js");
>+Cu.import("resource://testing-common/services-common/storageserver.js");
>+
>+const PORT = 8080;
>+
>+/**
>+ * Helper function to prepare a RESTRequest against the server.
>+ */
>+function localRequest(path, user, password) {
>+  _("localRequest: " + path);
>+  let url = "http://127.0.0.1:" + PORT + path;
>+  _("url: " + url);
>+  let req = new RESTRequest(url);
>+
>+  let header = basic_auth_header(user || "123", password || "password");
>+  req.setHeader("Authorization", header);
>+  req.setHeader("Accept", "application/json");
>+
>+  return req;
>+}
>+
>+/**
>+ * Helper function to validate an HTTP response from the server.
>+ */
>+function validateResponse(response) {
>+  do_check_true("x-timestamp" in response.headers);
>+
>+  let cl;
>+
>+  if ("content-length" in response.headers) {
>+    cl = parseInt(response.headers["content-length"]);
>+
>+    if (cl != 0) {
>+      do_check_true("content-type" in response.headers);
>+      do_check_eq("application/json", response.headers["content-type"]);
>+    }
>+  }
>+
>+  if (response.status == 204 || response.status == 304) {
>+    do_check_false("content-type" in response.headers);
>+
>+    if ("content-length" in response.headers) {
>+      do_check_eq(response.headers["content-length"], "0");
>+    }
>+  }
>+
>+  if (response.status == 405) {
>+    do_check_true("allow" in response.headers);
>+  }
>+}
>+
>+/**
>+ * Helper function to perform GET request synchronously.
>+ */
>+function doGetRequest(request) {
>+  let cb = Async.makeSpinningCallback();
>+  request.get(cb);
>+
>+  let error = cb.wait();
>+
>+  if (!error) {
>+    validateResponse(request.response);
>+  }
>+
>+  return error;
>+}
>+
>+/**
>+ * Helper function to synchronously do a PUT request.
>+ *
>+ * Returns Error passed into onComplete callback.
>+ */
>+function doPutRequest(request, data) {
>+  let cb = Async.makeSpinningCallback();
>+  request.put(data, cb);
>+
>+  let error = cb.wait();
>+
>+  if (!error) {
>+    validateResponse(request.response);
>+  }
>+
>+  return error;
>+}
>+
>+function doDeleteRequest(request) {
>+  let cb = Async.makeSpinningCallback();
>+  request.delete(cb);
>+
>+  let error = cb.wait();
>+
>+  if (!error) {
>+    validateResponse(request.response);
>+  }
>+
>+  return error;
>+}
>+
>+function run_test() {
>+  Log4Moz.repository.getLogger("Services.Common.Test.StorageServer").level =
>+    Log4Moz.Level.Trace;
>+  initTestLogging();
>+
>+  run_next_test();
>+}
>+
>+add_test(function test_creation() {
>+  _("Ensure a simple server can be created.");
>+
>+  // Explicit callback for this one.
>+  let server = new StorageServer({
>+    __proto__: StorageServerCallback,
>+  });
>+  do_check_true(!!server);
>+
>+  server.start(PORT, function () {
>+    _("Started on " + server.port);
>+    do_check_eq(server.port, PORT);
>+    server.stop(run_next_test);
>+  });
>+});
>+
>+add_test(function test_synchronous_start() {
>+  _("Ensure starting using startSynchronous works.");
>+
>+  let server = new StorageServer();
>+  server.startSynchronous(PORT);
>+  server.stop(run_next_test);
>+});
>+
>+add_test(function test_url_parsing() {
>+  _("Ensure server parses URLs properly.");
>+
>+  let server = new StorageServer();
>+
>+  // Check that we can parse a BSO URI.
>+  let parts = server.pathRE.exec("/2.0/12345/storage/crypto/keys");
>+  let [all, version, user, first, rest] = parts;
>+  do_check_eq(all, "/2.0/12345/storage/crypto/keys");
>+  do_check_eq(version, "2.0");
>+  do_check_eq(user, "12345");
>+  do_check_eq(first, "storage");
>+  do_check_eq(rest, "crypto/keys");
>+  do_check_eq(null, server.pathRE.exec("/nothing/else"));
>+
>+  // Check that we can parse a collection URI.
>+  parts = server.pathRE.exec("/2.0/123/storage/crypto");
>+  let [all, version, user, first, rest] = parts;
>+  do_check_eq(all, "/2.0/123/storage/crypto");
>+  do_check_eq(version, "2.0");
>+  do_check_eq(user, "123");
>+  do_check_eq(first, "storage");
>+  do_check_eq(rest, "crypto");
>+
>+  // We don't allow trailing slash on storage URI.
>+  parts = server.pathRE.exec("/2.0/1234/storage/");
>+  do_check_eq(parts, undefined);
>+
>+  // storage alone is a valid request.
>+  parts = server.pathRE.exec("/2.0/123456/storage");
>+  let [all, version, user, first, rest] = parts;
>+  do_check_eq(all, "/2.0/123456/storage");
>+  do_check_eq(version, "2.0");
>+  do_check_eq(user, "123456");
>+  do_check_eq(first, "storage");
>+  do_check_eq(rest, undefined);
>+
>+  parts = server.storageRE.exec("storage");
>+  let [all, storage, collection, id] = parts;
>+  do_check_eq(all, "storage");
>+  do_check_eq(collection, undefined);
>+
>+  run_next_test();
>+});
>+
>+add_test(function test_basic_http() {
>+  let server = new StorageServer();
>+  server.registerUser("345", "password");
>+  do_check_true(server.userExists("345"));
>+  server.startSynchronous(PORT);
>+
>+  _("Started on " + server.port);
>+  let req = localRequest("/2.0/storage/crypto/keys");
>+  _("req is " + req);
>+  req.get(function (err) {
>+    do_check_eq(null, err);
>+    server.stop(run_next_test);
>+  });
>+});
>+
>+add_test(function test_info_collections() {
>+  let server = new StorageServer();
>+  server.registerUser("123", "password");
>+  server.startSynchronous(PORT);
>+
>+  let path = "/2.0/123/info/collections";
>+
>+  _("info/collections on empty server should be empty object.");
>+  let request = localRequest(path, "123", "password");
>+  let error = doGetRequest(request);
>+  do_check_eq(error, null);
>+  do_check_eq(request.response.status, 200);
>+  do_check_eq(request.response.body, "{}");
>+
>+  _("Creating an empty collection should result in collection appearing.");
>+  let coll = server.createCollection("123", "col1");
>+  let request = localRequest(path, "123", "password");
>+  let error = doGetRequest(request);
>+  do_check_eq(error, null);
>+  do_check_eq(request.response.status, 200);
>+  let info = JSON.parse(request.response.body);
>+  do_check_attribute_count(info, 1);
>+  do_check_true("col1" in info);
>+  do_check_eq(info.col1, coll.timestamp);
>+
>+  server.stop(run_next_test);
>+});
>+
>+add_test(function test_bso_get_existing() {
>+  _("Ensure that BSO retrieval works.");
>+
>+  let server = new StorageServer();
>+  server.registerUser("123", "password");
>+  server.createContents("123", {
>+    test: {"bso": {"foo": "bar"}}
>+  });
>+  server.startSynchronous(PORT);
>+
>+  let coll = server.user("123").collection("test");
>+
>+  let request = localRequest("/2.0/123/storage/test/bso", "123", "password");
>+  let error = doGetRequest(request);
>+  do_check_eq(error, null);
>+  do_check_eq(request.response.status, 200);
>+  do_check_eq(request.response.headers["content-type"], "application/json");
>+  let bso = JSON.parse(request.response.body);
>+  do_check_attribute_count(bso, 3);
>+  do_check_eq(bso.id, "bso");
>+  do_check_eq(bso.modified, coll.bso("bso").modified);
>+  let payload = JSON.parse(bso.payload);
>+  do_check_attribute_count(payload, 1);
>+  do_check_eq(payload.foo, "bar");
>+
>+  server.stop(run_next_test);
>+});
>+
>+add_test(function test_bso_404() {
>+  _("Ensure the server responds with a 404 if a BSO does not exist.");
>+
>+  let server = new StorageServer();
>+  server.registerUser("123", "password");
>+  server.createContents("123", {
>+    test: {}
>+  });
>+  server.startSynchronous(PORT);
>+
>+  let request = localRequest("/2.0/123/storage/test/foo");
>+  let error = doGetRequest(request);
>+  do_check_eq(error, null);
>+
>+  do_check_eq(request.response.status, 404);
>+  do_check_false("content-type" in request.response.headers);
>+
>+  server.stop(run_next_test);
>+});
>+
>+add_test(function test_bso_if_modified_since_304() {
>+  _("Ensure the server responds properly to X-If-Modified-Since for BSOs.");
>+
>+  let server = new StorageServer();
>+  server.registerUser("123", "password");
>+  server.createContents("123", {
>+    test: {bso: {foo: "bar"}}
>+  });
>+  server.startSynchronous(PORT);
>+
>+  let coll = server.user("123").collection("test");
>+  do_check_neq(coll, null);
>+
>+  // Rewind clock just in case.
>+  coll.timestamp -= 10000;
>+  coll.bso("bso").modified -= 10000;
>+
>+  let request = localRequest("/2.0/123/storage/test/bso", "123", "password");
>+  request.setHeader("X-If-Modified-Since", "" + server.serverTime());
>+  let error = doGetRequest(request);
>+  do_check_eq(null, error);
>+
>+  do_check_eq(request.response.status, 304);
>+  do_check_false("content-type" in request.response.headers);
>+
>+  let request = localRequest("/2.0/123/storage/test/bso", "123", "password");
>+  request.setHeader("X-If-Modified-Since", "" + (server.serverTime() - 20000));
>+  let error = doGetRequest(request);
>+  do_check_eq(null, error);
>+  do_check_eq(request.response.status, 200);
>+  do_check_eq(request.response.headers["content-type"], "application/json");
>+
>+  server.stop(run_next_test);
>+});
>+
>+add_test(function test_bso_if_unmodified_since() {
>+  _("Ensure X-If-Unmodified-Since works properly on BSOs.");
>+
>+  let server = new StorageServer();
>+  server.registerUser("123", "password");
>+  server.createContents("123", {
>+    test: {bso: {foo: "bar"}}
>+  });
>+  server.startSynchronous(PORT);
>+
>+  let coll = server.user("123").collection("test");
>+  do_check_neq(coll, null);
>+
>+  let time = coll.timestamp;
>+
>+  _("Ensure we get a 412 for specified times older than server time.");
>+  let request = localRequest("/2.0/123/storage/test/bso", "123", "password");
>+  request.setHeader("X-If-Unmodified-Since", time - 5000);
>+  request.setHeader("Content-Type", "application/json");
>+  let payload = JSON.stringify({"payload": "foobar"});
>+  let error = doPutRequest(request, payload);
>+  do_check_eq(null, error);
>+  do_check_eq(request.response.status, 412);
>+
>+  _("Ensure we get a 204 if update goes through.");
>+  let request = localRequest("/2.0/123/storage/test/bso", "123", "password");
>+  request.setHeader("Content-Type", "application/json");
>+  request.setHeader("X-If-Unmodified-Since", time + 1);
>+  let error = doPutRequest(request, payload);
>+  do_check_eq(null, error);
>+  do_check_eq(request.response.status, 204);
>+  do_check_true(coll.timestamp > time);
>+
>+  // Not sure why a client would send X-If-Unmodified-Since if a BSO doesn't
>+  // exist. But, why not test it?
>+  _("Ensure we get a 201 if creation goes through.");
>+  let request = localRequest("/2.0/123/storage/test/none", "123", "password");
>+  request.setHeader("Content-Type", "application/json");
>+  request.setHeader("X-If-Unmodified-Since", time);
>+  let error = doPutRequest(request, payload);
>+  do_check_eq(null, error);
>+  do_check_eq(request.response.status, 201);
>+
>+  server.stop(run_next_test);
>+});
>+
>+add_test(function test_bso_delete_not_exist() {
>+  _("Ensure server behaves properly when deleting a BSO that does not exist.");
>+
>+  let server = new StorageServer();
>+  server.registerUser("123", "password");
>+  server.user("123").createCollection("empty");
>+  server.startSynchronous(PORT);
>+
>+  server.callback.onItemDeleted = function onItemDeleted(username, collection,
>+                                                         id) {
>+    do_throw("onItemDeleted should not have been called.");
>+  };
>+
>+  let request = localRequest("/2.0/123/storage/empty/nada", "123", "password");
>+  let error = doDeleteRequest(request);
>+  do_check_eq(error, null);
>+  do_check_eq(request.response.status, 404);
>+  do_check_false("content-type" in request.response.headers);
>+
>+  server.stop(run_next_test);
>+});
>+
>+add_test(function test_bso_delete_exists() {
>+  _("Ensure proper semantics when deleting a BSO that exists.");
>+
>+  let server = new StorageServer();
>+  server.registerUser("123", "password");
>+  server.startSynchronous(PORT);
>+
>+  let coll = server.user("123").createCollection("test");
>+  let bso = coll.insert("myid", {foo: "bar"});
>+  let timestamp = coll.timestamp;
>+
>+  server.callback.onItemDeleted = function onDeleted(username, collection, id) {
>+    delete server.callback.onItemDeleted;
>+    do_check_eq(username, "123");
>+    do_check_eq(collection, "test");
>+    do_check_eq(id, "myid");
>+  };
>+
>+  let request = localRequest("/2.0/123/storage/test/myid", "123", "password");
>+  let error = doDeleteRequest(request);
>+  do_check_eq(error, null);
>+  do_check_eq(request.response.status, 204);
>+  do_check_eq(coll.bsos().length, 0);
>+  do_check_true(coll.timestamp > timestamp);
>+
>+  _("On next request the BSO should not exist.");
>+  let request = localRequest("/2.0/123/storage/test/myid", "123", "password");
>+  let error = doGetRequest(request);
>+  do_check_eq(error, null);
>+  do_check_eq(request.response.status, 404);
>+
>+  server.stop(run_next_test);
>+});
>+
>+add_test(function test_bso_delete_unmodified() {
>+  _("Ensure X-If-Unmodified-Since works when deleting BSOs.");
>+
>+  let server = new StorageServer();
>+  server.startSynchronous(PORT);
>+  server.registerUser("123", "password");
>+  let coll = server.user("123").createCollection("test");
>+  let bso = coll.insert("myid", {foo: "bar"});
>+
>+  let modified = bso.modified;
>+
>+  _("Issuing a DELETE with an older time should fail.");
>+  let path = "/2.0/123/storage/test/myid";
>+  let request = localRequest(path, "123", "password");
>+  request.setHeader("X-If-Unmodified-Since", modified - 1000);
>+  let error = doDeleteRequest(request);
>+  do_check_eq(error, null);
>+  do_check_eq(request.response.status, 412);
>+  do_check_false("content-type" in request.response.headers);
>+  do_check_neq(coll.bso("myid"), null);
>+
>+  _("Issuing a DELETE with a newer time should work.");
>+  let request = localRequest(path, "123", "password");
>+  request.setHeader("X-If-Unmodified-Since", modified + 1000);
>+  let error = doDeleteRequest(request);
>+  do_check_eq(error, null);
>+  do_check_eq(request.response.status, 204);
>+  do_check_true(coll.bso("myid").deleted);
>+
>+  server.stop(run_next_test);
>+});
>+
>+add_test(function test_missing_collection_404() {
>+  _("Ensure a missing collection returns a 404.");
>+
>+  let server = new StorageServer();
>+  server.registerUser("123", "password");
>+  server.startSynchronous(PORT);
>+
>+  let request = localRequest("/2.0/123/storage/none", "123", "password");
>+  let error = doGetRequest(request);
>+  do_check_eq(error, null);
>+  do_check_eq(request.response.status, 404);
>+  do_check_false("content-type" in request.response.headers);
>+
>+  server.stop(run_next_test);
>+});
>+
>+add_test(function test_get_storage_405() {
>+  _("Ensure that a GET on /storage results in a 405.");
>+
>+  let server = new StorageServer();
>+  server.registerUser("123", "password");
>+  server.startSynchronous(PORT);
>+
>+  let request = localRequest("/2.0/123/storage", "123", "password");
>+  let error = doGetRequest(request);
>+  do_check_eq(error, null);
>+  do_check_eq(request.response.status, 405);
>+  do_check_eq(request.response.headers["allow"], "DELETE");
>+
>+  server.stop(run_next_test);
>+});
>+
>+add_test(function test_delete_storage() {
>+  _("Ensure that deleting all of storage works.");
>+
>+  let server = new StorageServer();
>+  server.registerUser("123", "password");
>+  server.createContents("123", {
>+    foo: {a: {foo: "bar"}, b: {bar: "foo"}},
>+    baz: {c: {bob: "law"}, blah: {law: "blog"}}
>+  });
>+
>+  server.startSynchronous(PORT);
>+
>+  let request = localRequest("/2.0/123/storage", "123", "password");
>+  let error = doDeleteRequest(request);
>+  do_check_eq(error, null);
>+  do_check_eq(request.response.status, 204);
>+  do_check_attribute_count(server.users["123"].collections, 0);
>+
>+  server.stop(run_next_test);
>+});
>+
>+add_test(function test_x_num_records() {
>+  let server = new StorageServer();
>+  server.registerUser("123", "password");
>+
>+  server.createContents("123", {
>+    crypto: {foos: {foo: "bar"},
>+             bars: {foo: "baz"}}
>+  });
>+  server.startSynchronous(PORT);
>+  let bso = localRequest("/2.0/123/storage/crypto/foos");
>+  bso.get(function (err) {
>+    // BSO fetches don't have one.
>+    do_check_false("x-num-records" in this.response.headers);
>+    let col = localRequest("/2.0/123/storage/crypto");
>+    col.get(function (err) {
>+      // Collection fetches do.
>+      do_check_eq(this.response.headers["x-num-records"], "2");
>+      server.stop(run_next_test);
>+    });
>+  });
>+});
>diff --git a/services/common/tests/unit/xpcshell.ini b/services/common/tests/unit/xpcshell.ini
>--- a/services/common/tests/unit/xpcshell.ini
>+++ b/services/common/tests/unit/xpcshell.ini
>@@ -1,10 +1,10 @@
> [DEFAULT]
>-head = head_global.js head_helpers.js
>+head = head_global.js head_helpers.js head_http.js
> tail =
> 
> # Test load modules first so syntax failures are caught early.
> [test_load_modules.js]
> 
> [test_utils_atob.js]
> [test_utils_encodeBase32.js]
> [test_utils_json.js]
>@@ -16,8 +16,10 @@
> [test_async_chain.js]
> [test_async_querySpinningly.js]
> [test_log4moz.js]
> [test_observers.js]
> [test_preferences.js]
> [test_restrequest.js]
> [test_tokenauthenticatedrequest.js]
> [test_tokenserverclient.js]
>+
>+[test_storage_server.js]
>diff --git a/services/common/utils.js b/services/common/utils.js
>--- a/services/common/utils.js
>+++ b/services/common/utils.js
>@@ -90,16 +90,30 @@
>   nextTick: function nextTick(callback, thisObj) {
>     if (thisObj) {
>       callback = callback.bind(thisObj);
>     }
>     Services.tm.currentThread.dispatch(callback, Ci.nsIThread.DISPATCH_NORMAL);
>   },
> 
>   /**
>+   * Spin the event loop and return once the next tick is executed.
>+   *
>+   * This is an evil function and should not be used in production code. It
>+   * exists in this module for ease-of-use.
>+   */
>+  waitForNextTick: function waitForNextTick() {
>+    let cb = Async.makeSyncCallback();
>+    this.nextTick(cb);
>+    Async.waitForSyncCallback(cb);
>+
>+    return;
>+  },
>+
>+  /**
>    * Return a timer that is scheduled to call the callback after waiting the
>    * provided time or as soon as possible. The timer will be set as a property
>    * of the provided object with the given timer name.
>    */
>   namedTimer: function namedTimer(callback, wait, thisObj, name) {
>     if (!thisObj || !name) {
>       throw "You must provide both an object and a property name for the timer!";
>     }
>@@ -378,16 +392,69 @@
>     let fos = FileUtils.openSafeFileOutputStream(file);
>     let is = this._utf8Converter.convertToInputStream(out);
>     NetUtil.asyncCopy(is, fos, function (result) {
>       if (typeof callback == "function") {
>         callback.call(that);
>       }
>     });
>   },
>+
>+  /**
>+   * Ensure that the specified value is defined in integer milliseconds since
>+   * UNIX epoch.
>+   *
>+   * This throws an error if the value is not an integer, is negative, or looks
>+   * like seconds, not milliseconds.
>+   *
>+   * If the value is null or 0, no exception is raised.
>+   *
>+   * @param value
>+   *        Value to validate.
>+   */
>+  ensureMillisecondsTimestamp: function ensureMillisecondsTimestamp(value) {
>+    if (!value) {
>+      return;
>+    }
>+
>+    return;
>+    if (Math.floor(value) != Math.ceil(value)) {
>+      throw new Error("Timestamp value is not an integer: " + value);
>+    }
>+
>+    if (value < 0) {
>+      throw new Error("Timestamp value is negative: " + value);
>+    }
>+
>+    // Catch what looks like seconds, not milliseconds.
>+    if (value < 10000000000) {
>+      throw new Error("Timestamp appears to be in seconds: " + value);
>+    }
>+  },
>+
>+  /**
>+   * Read bytes from an nsIInputStream into a string.
>+   *
>+   * @param stream
>+   *        (nsIInputStream) Stream to read from.
>+   * @param count
>+   *        (number) Integer number of bytes to read. If not defined, or
>+   *        0, all available input is read.
>+   */
>+  readBytesFromInputStream: function readBytesFromInputStream(stream, count) {
>+    let BinaryInputStream = Components.Constructor(
>+        "@mozilla.org/binaryinputstream;1",
>+        "nsIBinaryInputStream",
>+        "setInputStream");
>+    if (!count) {
>+      count = stream.available();
>+    }
>+
>+    return new BinaryInputStream(stream).readBytes(count);
>+  },
> };
> 
> XPCOMUtils.defineLazyGetter(CommonUtils, "_utf8Converter", function() {
>   let converter = Cc["@mozilla.org/intl/scriptableunicodeconverter"]
>                     .createInstance(Ci.nsIScriptableUnicodeConverter);
>   converter.charset = "UTF-8";
>   return converter;
> });

Comment 25

[Gregory Szorc [:gps]]

(In reply to Tarek Ziadé (:tarek) from comment #24)

> >+if __name__ == '__main__':
> >+    if len(argv) == 4:
> >+        exit(run_server(argv[1], argv[2], argv[3]))
> >+
> >+    if len(argv) == 5:
> >+        exit(run_server(argv[1], argv[2], argv[3], port=argv[4]))
> >+
> >+    if len(argv) == 6:
> >+        exit(run_server(argv[1], argv[2], argv[3], port=argv[4],
> >+            hostname=argv[5]))
> >+
> >+    print 'Usage: %s /path/to/srcdir /path/to/objdir [js_file] [port] [address]'
> >+    exit(1)
> 
> 
> You've reached point where you should use a command line args library, to
> avoid 
> doing all that low level work. You'll also get type checking for free.
> 
> Argparse is a good one (in the stdlib for py 2.7 as a standalone lib for 2.6)

Yeah, I agree it is on the threshold of acceptable use to not use a parser. I was hoping to avoid the extra lines of code for configuring the parser.

I'm familiar with argparse and typically use it. It is also worth mentioning that the official Mozilla build system is currently stuck on Python 2.5 in places. Bug 724191 tracks upgrading everything to 2.7. That means no argparse :( Although, this functionality isn't part of the official build system, so I could probably sneak it in ;)

Comment 26

[Gregory Szorc [:gps]]

https://hg.mozilla.org/services/services-central/rev/d818dcdb317b

Comment 27

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/d818dcdb317b