959033 - Don't send X-Confirm-Delete header

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Android Sync, defect, P3)

Description

[Ryan Kelly [:rfkelly]]

Per the proposed sync1.5 protocol docs in Bug 951986, the sync storage servers will no longer require an "X-Confirm-Delete" header on delete requests.  Android sync client should avoid sending this header if it knows it's talking to a sync1.5 server.

Note that it's safe to send the header anyway if you need to, as it will be ignored.

Comment 1

[Richard Newman [:rnewman]]

N.B., given that the backend client code is shared, this removal either needs to be conditional or to only occur when Sync 1.1 is deprecated.

Marking qa-, 'cos this'll have unit tests and won't be user-facing.

Comment 2

[Nick Alexander :nalexander [he/him]]

This is not a big deal.

Comment 3

[Richard Newman [:rnewman]]

This is a great first bug -- delete a line and fix the test.

android/services/src/main/java/org/mozilla/gecko/sync/net/SyncStorageRequest.java
162:        request.addHeader("x-confirm-delete", "1");

android/tests/background/junit4/src/org/mozilla/android/sync/net/test/TestSyncStorageRequest.java
253:      assertNotNull(request.getValue("x-confirm-delete"));
254:      assertEquals("1", request.getValue("x-confirm-delete"));

We only support Sync 1.5 now, so we can just remove this functionality.

Comment 4

[kimsaehun]

Hi, I wanted to start contributing to open source projects and found this bug from BugsAhoy(https://www.joshmatthews.net/bugsahoy/?java=1&unowned=1&simple=1). May I take this task and also receive some guidance on how I can complete this task?

Comment 5

[Mark Hammond [:markh] [:mhammond]]

Hi kimsaehun,
  Thanks for taking an interest in this bug. You should follow the instructions at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction (and in this case you will be following the instructions for Android), and get a build of Firefox for Android working. Once you have that working, see comment 3 for the specific changes that need to be made - you then rebuild Firefox with your changes, run the tests to ensure they work, then put your patch up for review.

Please let us know if you run into problems, and consider joining the #introduction channel on IRC where you can ask all kinds of introductory questions.

Comment 6

[kimsaehun]

Attachment: Bug 959033 - Don't send X-Confirm-Delete header with Android Sync requests.

Review commit: https://reviewboard.mozilla.org/r/164704/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/164704/

Comment 7

[kimsaehun]

Hi Mark, Thanks for the guidance. I believe I followed the instructions correctly and have submitted a patch to this bug. It seems the next step is to ask for a review. To whom should I request this review to? Bugzilla recommends Grisha Kruglov. Would he be the correct person to request this to?

Comment 8

[Mark Hammond [:markh] [:mhammond]]

Thanks for the patch! Grisha is the right person to request review from, and the easiest way to do that is to re-push your patch with a different commit message. If you make it, say, "Bug 959033 - Don't send X-Confirm-Delete header with Android Sync requests. r?grisha", you should find it automatically flags him for review, and if he gives r+, will also be ready to land.

Thanks!

Comment 9

[kimsaehun]

Comment on attachment 8893619 [details]
Bug 959033 - Don't send X-Confirm-Delete header with Android Sync requests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164704/diff/1-2/

Comment 10

[kimsaehun]

Looks like it worked! Thanks for your help Mark. Is it safe to presume that this is how I can keep on contributing from now on? Find a bug, ask for it to be assigned to me, work on it, and push a patch?

Comment 11

[Mark Hammond [:markh] [:mhammond]]

(In reply to kimsaehun from comment #10)
> Looks like it worked! Thanks for your help Mark. Is it safe to presume that
> this is how I can keep on contributing from now on? Find a bug, ask for it
> to be assigned to me, work on it, and push a patch?

Yep, that sounds great! In general though, instead of immediately asking for it to be assigned to you, you should probably start with either a comment indicating how you intend tackling the bug, or just by uploading a patch - or if you aren't sure, just asking for guidance as you did here. As you have already seen, bugsahoy is a great way to find bugs.

Let me know if I can be more help, and thanks for contributing to Firefox!

Comment 12

[:Grisha Kruglov]

Comment on attachment 8893619 [details]
Bug 959033 - Don't send X-Confirm-Delete header with Android Sync requests.

https://reviewboard.mozilla.org/r/164704/#review170186

Looks good! Thanks for the patch, kimsaehun! I've pushed it to our test servers, once everything passes there it should be good to land.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b36b0c31e1e

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/01ea7fe459c1
Don't send X-Confirm-Delete header with Android Sync requests. r=Grisha

Comment 14

[kimsaehun]

Thanks for the review Grisha. Unless something goes wrong in the test server, is it correct to say I'm done with this bug?

Comment 15

[:Grisha Kruglov]

(In reply to kimsaehun from comment #14)
> Thanks for the review Grisha. Unless something goes wrong in the test
> server, is it correct to say I'm done with this bug?

Indeed, all is good. Tests looked good, and I've "landed" your patch yesterday. Comment 13 indicates that it's been pushed into our integration branch. It should land on main mozilla-central branch sometime later today, at which point this bug will be officially marked as FIXED.

Comment 16

[kimsaehun]

Awesome, this wasn't as daunting as I thought it would be. Thanks again for the help Mark and Grisha.

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/01ea7fe459c1