Don't send X-Confirm-Delete header

RESOLVED FIXED in Firefox 57

Status

()

defect
P3
normal
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: rfkelly, Assigned: kimsaehun, Mentored)

Tracking

(Blocks 1 bug)

unspecified
Firefox 57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [qa-][good first bug][lang=java])

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
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.
Whiteboard: [qa?]
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.
Whiteboard: [qa?] → [qa-]
tracking-fennec: --- → 30+
Priority: -- → P2
tracking-fennec: 30+ → 31+
This is not a big deal.
tracking-fennec: 31+ → ---
Priority: P2 → P3
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.
Mentor: gkruglov
Summary: Don't send X-Confirm-Delete header to sync1.5 storage servers → Don't send X-Confirm-Delete header
Whiteboard: [qa-] → [qa-][good first bug][lang=java]
(Assignee)

Comment 4

2 years ago
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?
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.
(Assignee)

Comment 7

2 years ago
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?
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!
Assignee: nobody → kimsaehun
Comment hidden (mozreview-request)
(Assignee)

Comment 10

2 years ago
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?
(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

2 years ago
mozreview-review
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
Attachment #8893619 - Flags: review?(gkruglov) → review+

Comment 13

2 years ago
Pushed by gkruglov@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/01ea7fe459c1
Don't send X-Confirm-Delete header with Android Sync requests. r=Grisha
(Assignee)

Comment 14

2 years ago
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

2 years ago
(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.

Updated

2 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 16

2 years ago
Awesome, this wasn't as daunting as I thought it would be. Thanks again for the help Mark and Grisha.
https://hg.mozilla.org/mozilla-central/rev/01ea7fe459c1
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57

Updated

2 years ago
Product: Android Background Services → Firefox for Android
You need to log in before you can comment on or make changes to this bug.