760466 - Make JS storage server pass Python functional tests

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Gregory Szorc [:gps]]

Somewhere between bug 744323 being submitted for review and it landing, a few of the Python functional tests broke. This is a follow-up bug to address those regressions.

Comment 1

[Gregory Szorc [:gps]]

Attachment: Make pass functional tests, v1

This patch makes the JS server pass the Python functional tests:

python syncstorage/tests/functional/test_storage.py http://localhost:8080/
...s...............s.............s.
----------------------------------------------------------------------
Ran 35 tests in 23.420s

OK (skipped=3)

Those older and newer changes were made during review when the server landed. I think the original logic was right and we were all just reading the code wrong. I think the _() slipped in during a qref. Oh, Mercurial.

Comment 2

[Richard Newman [:rnewman]]

Comment on attachment 629748 [details] [diff] [review]
Make pass functional tests, v1

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

::: services/common/storageserver.js
@@ +406,5 @@
>        }
>      }
>  
>      if (options.older) {
> +      if (bso.modified >= options.older) {

Don't forget to follow up with Ryan about spec changes and the additional test he wrote.

Comment 3

[Gregory Szorc [:gps]]

Yeah, Ryan already made the changes to the functional tests. The spec could still use a little rewording...

Comment 4

[Gregory Szorc [:gps]]

Patch requires bug 757860. No big deal. I'll just wait.

Comment 5

[Gregory Szorc [:gps]]

Attachment: Part 2: Add support for X-I-U-S on collection URIs

Since this hasn't landed yet, I'm going to overload it to include support for X-If-Unmodified-Since, which is currently missing from a few URI endpoints. This is related to bug 762147.

Comment 6

[Gregory Szorc [:gps]]

Comment on attachment 630909 [details] [diff] [review]
Part 2: Add support for X-I-U-S on collection URIs

I discovered an open issue in the implementation in bug 762147. Will wait for clarification from rfk (in the form of MOAR tests) then will resubmit for review once I've verified behaviour is sane.

Comment 7

[Richard Newman [:rnewman]]

Comment on attachment 630909 [details] [diff] [review]
Part 2: Add support for X-I-U-S on collection URIs

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

::: services/common/storageserver.js
@@ -767,1 @@
>  

I see this as applying to services/common/storageserver.js, not services/common/tests/unit/storageserver.js. Intentional?

@@ +791,5 @@
> +          continue;
> +        }
> +
> +        serverModified = bso.modified;
> +      }

I don't think this logic is correct, in that it doesn't match the behavior of the production server.

The modified time is a property of the collection as a whole. We don't care about individual records for POST:

  > 412 Precondition Failed: an object in the collection has been modified since the timestamp in the X-If-Unmodified-Since header.

A closer approximation to the correct logic is to:

  let serverModified = 0;
  for (let bso of this._bsos) {
    if (bso.modified > serverModified) {
      serverModified = bso.modified;
    }
  }

but note that this does not behave correctly for an empty collection, which has a modified time and can return 412.

You might need to switch to tracking collection modified times.

Comment 8

[Gregory Szorc [:gps]]

You obviously didn't see the review cancellation and linked bug where I pointed out the implementation was likely wrong, did you? ;)

Comment 9

[Richard Newman [:rnewman]]

(In reply to Gregory Szorc [:gps] from comment #8)
> You obviously didn't see the review cancellation and linked bug where I
> pointed out the implementation was likely wrong, did you? ;)

Curse you, Szorc.

Comment 10

[Gregory Szorc [:gps]]

Attachment: Part 2: Add support for X-I-U-S on collection URIs, v2

New patch. Does things properly.

Comment 11

[Richard Newman [:rnewman]]

Comment on attachment 632804 [details] [diff] [review]
Part 2: Add support for X-I-U-S on collection URIs, v2

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

This looks OK to me…

::: services/common/storageserver.js
@@ +772,5 @@
>      }
>  
> +    if (request.hasHeader("x-if-unmodified-since")) {
> +      let requestModified = parseInt(request.getHeader("x-if-unmodified-since"),
> +                                     10);

Man, you're strict on these line lengths, huh?

@@ +784,5 @@
> +                       "time is newer than client's time.");
> +        response.setStatusLine(request.httpVersion, "412", "Precondition Failed");
> +        return;
> +      }
> +

Nit: surplus newline.

@@ +809,5 @@
> +      let serverModified = this.timestamp;
> +
> +      this._log.debug("Request modified time: " + requestModified +
> +                      "; Server modified time: " + serverModified);
> +      if (serverModified > requestModified) {

Invert this conditional and the log statement to match the… hey, wait a minute! This code is entirely duplicated from the last method!

Pull this code out into a separate method, willya? `if (!ensureUnmodifiedSince(request, response)) { return; }`?

@@ +1246,5 @@
>    /**
>     * HTTP response utility.
>     */
>    respond: function respond(req, resp, code, status, body, headers, timestamp) {
> +    this._log.info("Response: " + code + " " + status);

Debuggery?

Comment 12

[Gregory Szorc [:gps]]

(In reply to Richard Newman [:rnewman] from comment #11) 
> @@ +1246,5 @@
> >    /**
> >     * HTTP response utility.
> >     */
> >    respond: function respond(req, resp, code, status, body, headers, timestamp) {
> > +    this._log.info("Response: " + code + " " + status);
> 
> Debuggery?

Initially, yes. But, I think it is useful. A server should log and request and response parameters, right? Either way, this file isn't used in production, so meh.

Comment 13

[Gregory Szorc [:gps]]

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

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Can those tests be run on tbpl?

Comment 15

[Gregory Szorc [:gps]]

(In reply to :Ms2ger from comment #14)
> Can those tests be run on tbpl?

Not easily, no. The problem is somewhat complicated because it involves the mess that is Python packaging of the server code. We don't want to burden buildbot with this dependency.

Anyway, bug 760128 tracks a more robust, continuous integration solution.

Comment 16

[Gregory Szorc [:gps]]

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