496485 - make Service.wipeServer work properly

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Ed Lee :Mardak]

Each sync engine knows how to wipe itself from the server by deleting the engine directory and the crypto meta url.

WeaveSvc_wipeServer manually deletes the engine directory by name instead of calling the engine's wipe server, and it doesn't delete the crypto/meta.

So switch to SyngEngine's?

Comment 1

[Dan Mills [:thunder]]

We should use the SyncEngine's wipe method, I agree.

Comment 2

[Mike Connor [:mconnor]]

Attachment: use wipeServer if possible

wipeServer works for engines, but doesn't work for other collections, including those that aren't implemented in the current client.  So we should use engine.wipeServer if possible, and otherwise issue the deletes directly (and add the crypto URL to the manual deletes)

Comment 3

[Ed Lee :Mardak]

Comment on attachment 437718 [details] [diff] [review]
use wipeServer if possible

>+++ b/source/modules/service.js
>           // If we have a list of engines, make sure it's one we want
>           if (engines && engines.indexOf(name) == -1)
>             continue;
>+          
>+          let engine;
>+          if (engine = Engines.get(name))
>+            engine.wipeServer();
Nothing special that requires the assignment to be done this way. Just assign when declaring engine and check for engine != null.

It might be saner to just special case check for engines without getting infoURL and just looping over the array to grab the engine and wipeServer(). This means we might issue deletes for stuff that isn't there, but that's not too bad and makes sure the crypto record goes away.

>+          else {
>+            new Resource(this.storageURL + name).delete();
>+            new Resource(this.storageURL + "crypto/" + name).delete();
These things that aren't engines aren't engines, so they don't have an accompanying crypto record.

Comment 4

[Mike Connor [:mconnor]]

(In reply to comment #3)
> (From update of attachment 437718 [details] [diff] [review])
> >+++ b/source/modules/service.js
> >           // If we have a list of engines, make sure it's one we want
> >           if (engines && engines.indexOf(name) == -1)
> >             continue;
> >+          
> >+          let engine;
> >+          if (engine = Engines.get(name))
> >+            engine.wipeServer();
> Nothing special that requires the assignment to be done this way. Just assign
> when declaring engine and check for engine != null.

Fixed.

> It might be saner to just special case check for engines without getting
> infoURL and just looping over the array to grab the engine and wipeServer().
> This means we might issue deletes for stuff that isn't there, but that's not
> too bad and makes sure the crypto record goes away.

Fixed, sort of.  We'll only hit infoURL if we don't call this with an arg.

> >+          else {
> >+            new Resource(this.storageURL + name).delete();
> >+            new Resource(this.storageURL + "crypto/" + name).delete();
> These things that aren't engines aren't engines, so they don't have an
> accompanying crypto record.

There may be engines that aren't part of the current client, we should still clear their crypto records if they exist.

Comment 5

[Mike Connor [:mconnor]]

Attachment: v2

Comment 6

[Ed Lee :Mardak]

Comment on attachment 437745 [details] [diff] [review]
v2

>+          let engine = Engines.get(name);
>+          if (engine)
>+            engine.wipeServer();
>+          else { // delete manually
>+            new Resource(this.storageURL + name).delete();
>+            new Resource(this.storageURL + "crypto/" + name).delete();
Still not sure what's the benefit of special casing engines here vs blindly deleting crypto/<collection>. Might as well just get rid of SyncEngine_wipeServer code.

Could perhaps be slightly smarter to detect that the whole crypto collection is going to be deleted.

Comment 7

[Mike Connor [:mconnor]]

It was basically future-proofing against some engine doing something tricksy so they do extra stuff on wipe server.  That said, I'm _entirely_ happy to nuke SyncEngine_wipeServer and just make people call this with the engine name as an arg, and removing it from the engine object!

Comment 8

[Mike Connor [:mconnor]]

Attachment: v3

* just nuke storage and crypto for each collection.
* remove engine_wipeServer, as it's unused

Comment 9

[Mike Connor [:mconnor]]

http://hg.mozilla.org/labs/weave/rev/56aa3c5e3724