Closed Bug 1368951 Opened 2 years ago Closed 2 years ago

Sync test server doesn't support x-if-unmodified-since

Categories

(Firefox :: Sync, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: markh, Assigned: markh)

References

Details

Attachments

(1 file)

The sync server probably should do this, both for completeness, and for other random bugs that might want it (eg, bug 1355766)

This is a test-only patch (with an additional log call in engines.js) - I thought I'd discovered a sync bug, but it turns out that the issues were all due to how the tests run.

The test changes are somewhat unfortunate, but I can't really think of a better option - most of these would be unnecessary if the engines tracked this.lastModified for writes, but they don't, and doing that only for test seem wrong. It could also be argued that advancing this.lastModified *at all* during the sync is wrong - it should start as the time from info/collections, and the incoming phase shouldn't move it - but I'm not going to explore that :)

Typically, the test changes are where |engine._sync()| is called (rather than |service.sync()|. When the sync GETs records, we can get away with setting engine.lastModified to null - but if it only POSTs, we set engine.lastModified to the collection lastModified (which would normally happen via the info/collections call done as part of the service.sync.

I also got sick of the SQlite log noise everywhere, so silenced that globally.
Comment on attachment 8872922 [details]
Bug 1368951 - add x-if-unmodified-since support to the sync test server.

Can you see a better way to arrange the test changes?
Attachment #8872922 - Flags: feedback?(tchiovoloni)
Attachment #8872922 - Flags: feedback?(kit)
Comment on attachment 8872922 [details]
Bug 1368951 - add x-if-unmodified-since support to the sync test server.

https://reviewboard.mozilla.org/r/144468/#review148476

Sorry this was such a slog. Unless we switch to `Service.sync([name])` instead of `sync_engine_and_validate_telem`, which will probably raise even more issues, I think your approach is the way to go.

::: services/sync/modules/engines.js:1253
(Diff revision 1)
>      }
>  
> -    // Mobile: check if we got the maximum that we requested; get the rest if so.
> +    // History: check if we got the maximum that we requested; get the rest if so.
>      if (handled.length == newitems.limit) {
> +      // XXX - this block appears to have no test coverage (eg, throwing here,
> +      // or commenting the entire block causes no tests to fail.)

Not related to this patch, but, following up from our chat on IRC: this block confuses me because I'm not sure what the expected semantics *are*.

In the first `getBatched` above, we'll fetch up to 5k records since `lastSync`, in an unspecified sort order. (Ryan mentioned we shouldn't depend on this, but I went trawling through the server code, and I *think* this is where we sort newest first by default: https://github.com/mozilla-services/server-syncstorage/blob/10ab50027f3d7b5bf7af4354ebdbc62cc6a81a2f/syncstorage/storage/sql/queries_generic.py#L222-L228).

We then make a separate, unbatched request for up to 5k IDs, also since the `lastSync` (which we haven't fast-forwarded yet), but explicitly sorting by index, which, for history, is based on the frecency.

So:

* It's likely that IDs we've already downloaded will show up again (I'm assuming frecent sites are, well, frequently visited), so we'll do some busy work fetching and filtering those out.
* If you've visited sites frequently in the past, but not since the last sync, we might still sync those down (Maybe? IIUC, we don't listen for `onFrecencyChanged` or `onManyFrecenciesChanged` notifications, so we won't, and probably shouldn't, reupload those visits with a new sort index).
* The most succinct way to describe what's fetched seems to be "some possibly overlapping subset" of your history, which makes me wary.

Is it worth trying to articulate the desired behavior before we add tests for this block?

::: services/sync/tests/unit/head_http_server.js:948
(Diff revision 1)
> +      let checkXIUSFailure = () =>  {
> +        if (req.hasHeader("x-if-unmodified-since")) {
> +          let xius = parseFloat(req.getHeader("x-if-unmodified-since"));
> +          // Sadly the way our tests are setup, we often end up with xius of
> +          // zero (typically when syncing just one engine, so the date from
> +          // info/collections isn't used) - so we allow that to work.

This behavior is really unfortunate. A single engine sync still fires all the observer notifications, and involves the scheduler, but skips fast-forwarding `lastModified`. I don't think that's really actionable; I'm just disappointed that we have to fake our way around it in tests.

::: services/sync/tests/unit/test_bookmark_engine.js:721
(Diff revision 1)
>      item2.dateAdded += 10000;
>      collection.insert(item2GUID, encryptPayload(item2.cleartext), now / 1000 - 10);
> -    engine.lastSync = now / 1000 - 20;
>  
> +    engine.lastModified = null;
>      await sync_engine_and_validate_telem(engine, false);

I wonder if it's worth passing the last modified date to `sync_engine_and_validate_telem`, just because I can see forgetting to copy-paste the `engine.lastModified = ...` line would be a gotcha in the future.

::: services/sync/tests/unit/test_bookmark_order.js:145
(Diff revision 1)
>    }];
>    for (let record of serverRecords) {
>      collection.insert(record.id, encryptPayload(record), timestamp);
>    }
>  
> +  engine.lastModified = collection.timestamp;

Hmm, does `collection.insert` update its timestamp? I didn't see that it does, which surprised me. But maybe that's not relevant for this test?

::: services/sync/tests/unit/test_clients_engine.js:143
(Diff revision 1)
>      deletedCollections = [];
>      deletedItems       = [];
>      check_clients_count(1);
> +
> +    engine.lastModified = server.getCollection("foo", "clients").timestamp;
>      engine._sync();

...Except here we have bare `_sync` calls, so maybe passing the last modified time to `sync_engine_and_validate_telem` doesn't make sense.
Attachment #8872922 - Flags: feedback?(kit) → feedback+
>  but I went trawling through the server code, and I *think* this is where we sort newest first by default

A friendly reminder that the go server might also do something different :-)

> explicitly sorting by index, which, for history, is based on the frecency.

For the record, the python server (ironically) does not index the "index" field, so you should expect queries that sort by index to be significantly slower on large collections.
(In reply to Ryan Kelly [:rfkelly] from comment #4)
> For the record, the python server (ironically) does not index the "index"
> field, so you should expect queries that sort by index to be significantly
> slower on large collections.

Huh, TIL. I believe bookmarks is the only engine to try and do that.
Priority: -- → P1
Comment on attachment 8872922 [details]
Bug 1368951 - add x-if-unmodified-since support to the sync test server.

https://reviewboard.mozilla.org/r/144468/#review151472

This looks fine. I agree with kit's concerns (especially wrt confusion in engines.js), but don't think that should stop us from improving the mock server.
Attachment #8872922 - Flags: review+
Priority: P1 → P2
Comment on attachment 8872922 [details]
Bug 1368951 - add x-if-unmodified-since support to the sync test server.

Marking as f+ to get this out of my queue since I already r+ed it. To be clear, I think new boilerplate is unfortunate, but we already have a decent amount of it. If it's a real issue we could always write a `syncIndividualEngine(e)` helper, or similar.

(Although, it might be more trouble than it's worth, since IDK if it could know the right thing to do in all cases.)
Attachment #8872922 - Flags: feedback?(tchiovoloni) → feedback+
Blocks: 1355766
Comment on attachment 8872922 [details]
Bug 1368951 - add x-if-unmodified-since support to the sync test server.

https://reviewboard.mozilla.org/r/144468/#review148476

> Not related to this patch, but, following up from our chat on IRC: this block confuses me because I'm not sure what the expected semantics *are*.
> 
> In the first `getBatched` above, we'll fetch up to 5k records since `lastSync`, in an unspecified sort order. (Ryan mentioned we shouldn't depend on this, but I went trawling through the server code, and I *think* this is where we sort newest first by default: https://github.com/mozilla-services/server-syncstorage/blob/10ab50027f3d7b5bf7af4354ebdbc62cc6a81a2f/syncstorage/storage/sql/queries_generic.py#L222-L228).
> 
> We then make a separate, unbatched request for up to 5k IDs, also since the `lastSync` (which we haven't fast-forwarded yet), but explicitly sorting by index, which, for history, is based on the frecency.
> 
> So:
> 
> * It's likely that IDs we've already downloaded will show up again (I'm assuming frecent sites are, well, frequently visited), so we'll do some busy work fetching and filtering those out.
> * If you've visited sites frequently in the past, but not since the last sync, we might still sync those down (Maybe? IIUC, we don't listen for `onFrecencyChanged` or `onManyFrecenciesChanged` notifications, so we won't, and probably shouldn't, reupload those visits with a new sort index).
> * The most succinct way to describe what's fetched seems to be "some possibly overlapping subset" of your history, which makes me wary.
> 
> Is it worth trying to articulate the desired behavior before we add tests for this block?

Thanks - that's insightful. I took the low-road here and added a reference to this note in the comment block.

> Hmm, does `collection.insert` update its timestamp? I didn't see that it does, which surprised me. But maybe that's not relevant for this test?

Good catch! I fixed this.
Pushed by mhammond@skippinet.com.au:
https://hg.mozilla.org/integration/autoland/rev/33c3f7d522c5
add x-if-unmodified-since support to the sync test server. r=tcsc
https://hg.mozilla.org/mozilla-central/rev/33c3f7d522c5
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Depends on: 1392409
You need to log in before you can comment on or make changes to this bug.