Sync: client includes default Accept header in API requests

RESOLVED FIXED in Firefox 62

Status

()

P3
normal
RESOLVED FIXED
8 years ago
6 months ago

People

(Reporter: rnewman, Assigned: carol.ng, Mentored)

Tracking

unspecified
Firefox 62
Points:
---

Firefox Tracking Flags

(firefox62 fixed)

Details

(Whiteboard: [good first bug][lang=js][sync:scale])

Attachments

(2 attachments)

(Reporter)

Description

8 years ago
Last I checked:

   Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8

That's ridiculous when we're really expecting JSON or application/newlines. HTTP ain't that hard.
(Reporter)

Updated

6 years ago
Whiteboard: [good first bug] → [good first bug][mentor=rnewman][lang=js]
(Reporter)

Updated

6 years ago
Whiteboard: [good first bug][mentor=rnewman][lang=js] → [good first bug][mentor=rnewman][lang=js][sync:scale]
Mentor: rnewman
Whiteboard: [good first bug][mentor=rnewman][lang=js][sync:scale] → [good first bug][lang=js][sync:scale]
I'd like to be assigned to this bug, could someone assign me please.
(Reporter)

Comment 2

4 years ago
No need to be assigned. Just go ahead and get your development environment set up, and let us know if you hit snags.

See https://developer.mozilla.org/en-US/docs/Developer_Guide/Build_Instructions#Getting_started
Hi Richard, I have set up my dev environment. I've build Firefox. But, the bug's product label did not explitly state Firefox, it's just Mozilla Services. So, I'm thinking whether building Firefox is what is needed to work with this bug?
(Reporter)

Comment 4

4 years ago
Firefox is the right place -- that's the product that ships this service code.

You should be able to write a small unit test (look in services/sync/tests/unit) that checks the Accept header, and makes sure it's asking for JSON... then that test will fail, and you can move on to making it work!
I tried and pieced together this codes for a unit test. 

'use strict'

Cu.import("resource://services-sync/util.js");
Cu.import("resource://services-common/rest.js");

function run_test() {
  // initTestLogging("Trace");
  run_next_test();
}

function localRequest(server, path) {
  _("localRequest: " + path);
  let url = server.baseURI.substr(0, server.baseURI.length - 1) + path;
  _("url: " + url);
  return new RESTRequest(url);
}

add_test(function test_header(){
    let server = new SyncServer();
    server.registerUser("abc","password");
    do_check_true(server.userExists("abc"));
    server.start(null, function () {
    _("Started on " + server.port);
     let req = localRequest(server, "/1.1/abc/info/collections");
     _("req is " + req);
      req.get(function (response) {
        _('res is ' +response);
        Utils.nextTick(function () {
          server.stop(run_next_test);
        });
      });
  });
});

Referred test_httpd_sync_server.js. But I am not sure how to get the response header as the response I am getting is always null. 

Could it the problem of the endpoint I am using or problem in the unit test I wrote as a whole?
I slightly altered the add_test function

add_test(function test_header(){
    let server = new SyncServer();
    server.registerUser("abc","password");
    do_check_true(server.userExists("abc"));

    server.start(null, function () {
    _("Started on " + server.port);
     let req = localRequest(server, "/1.1/abc/info/collections");

      req.get(function (response) {
      do_check_eq(this.response.headers["Accept"], "application/json");

        Utils.nextTick(function () {
          server.stop(run_next_test);
        });
      });
  });
});

The test fails but this.response.headers['Accept'] is 'undefined'. Not sure whether I am testing the right thing.
(Reporter)

Comment 7

4 years ago
I think you're making two errors on top of each other: you're using an attribute when you should be accessing an argument, and when accessing that attribute you're referring to the wrong object.

Here's some JS code that should explain the second:

  let object = {
    foo: 42,

    // A method.
    bar: function () {
      console.log("Foo is " + this.foo);

      let f = function () {
        console.log("Foo is " + this.foo);
      };

      let g = () => {
        console.log("Foo is " + this.foo);
      };

      f();
      g();
    }
  };

> object.bar();
"Foo is 42"
"Foo is undefined"
"Foo is 42"


The first is simple: in your line

      do_check_eq(this.response.headers["Accept"], "application/json");

you're accessing the 'response' property *of the function* (which is the value of 'this'), which isn't defined.

Try using the response argument to that function instead.
I don't quite understand what you mean by "response argument to that function instead".

Is it like

function(response){
  do_check_eq(response.headers['Accept'], "application/json");
}
(Reporter)

Comment 9

4 years ago
> Is it like…

Yes.


> function(response){
           ^^^^^^^^
>   do_check_eq(response.headers['Accept'], "application/json");
                ^^^^^^^^

Comment 10

2 years ago
I'd like to work on this, has anyone looked in to it recently?

Comment 11

2 years ago
It seems to me the issue stems from RESTRequest in services/common/rest.js, rather than in sync. I've managed to reproduce the bug by adding the following test case to services/common/tests/unit/test_restrequest.js

add_test(function test_default_request_accept_headers() {
    let handler = httpd_handler(200, "OK");
    let server = httpd_setup({"/resource": handler});

    new RESTRequest(server.baseURI + "/resource").get(function(error) {
        do_check_eq(error, null);

        do_check_true(handler.request.hasHeader("accept"));

        let accept_header = handler.request.getHeader("accept");

        do_check_eq(accept_header.indexOf("text/html"), -1);
        do_check_eq(accept_header.indexOf("application/xhtml+xml"), -1);
        do_check_eq(accept_header.indexOf("application/xml"), -1);

        do_check_true(accept_header.indexOf("application/json") > -1 || accept_header.indexOf("application/newlines") > -1);

        server.stop(run_next_test);
    });
});

Running a do_print(accept_header) in that test case shows the same incorrect header from the OP. Does anyone know at-a-glance if that original accept header is required by a service other than sync, or if fixing it in common is the better approach?
(Reporter)

Comment 12

2 years ago
Kit, want to mentor this?
Mentor: rnewman → kit
(In reply to g4chris from comment #11)
> Running a do_print(accept_header) in that test case shows the same incorrect
> header from the OP. Does anyone know at-a-glance if that original accept
> header is required by a service other than sync, or if fixing it in common
> is the better approach?

Thanks for picking this up! I think you can go ahead and fix it in common/, yes. With the exception of the records handler, I'm not sure any of our servers do proper content negotiation, anyway.

Comment 14

2 years ago
I've attached a potential patch for this. I figured "application/json;q=0.9,*/*;q=0.2" would make a sensible default so that a server that does do strict content negotiation would choose json first, but fall back to something that works instead of giving up with a 406.
Attachment #8841258 - Flags: review?(kit)
Comment on attachment 8841258 [details] [diff] [review]
Proposed fix and test case for Bug 694735

LGTM, thanks for the patch! Please update services/sync/modules/resource.js, too; sadly, there's a lot of duplication between RESTRequest and AsyncResource. Please also include a commit message per https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Committing_Rules_and_Responsibilities#Commit_message_restrictions.
Attachment #8841258 - Flags: review?(kit) → feedback+
Assignee: nobody → g4chris
Priority: -- → P1
Hi! Would you like to fix up the nits and submit an updated patch, or should I take care of that? Either is OK!
Flags: needinfo?(g4chris)

Comment 17

2 years ago
I'll work on it and upload a new patch, should be done by the end of the week. I'm in my last year of CS undergrad so it's a pretty crazy time with a bunch of big deadlines at the beginning of April.
Flags: needinfo?(g4chris)
No worries. There's no rush at all; this bug just came up in our weekly triage.
Hi g4chris, thanks for the contribution, but there are a couple of comments that need to be addressed before we can land this. Are you still interested in helping us with this?
Flags: needinfo?(g4chris)
Priority: P1 → P3
Assignee: g4chris → nobody
Flags: needinfo?(g4chris)

Comment 20

2 years ago
Hello, I'm a student at Seneca college learning open source, and I was hoping to work on this bug for my course. If no one else is currently working on it, I'd like to give it a try.
(In reply to Hans van den Pol from comment #20)
> Hello, I'm a student at Seneca college learning open source, and I was
> hoping to work on this bug for my course. If no one else is currently
> working on it, I'd like to give it a try.

Please do! All the comments above in this bug should be enough to get you started, and feel free to ask any questions you have.
Comment hidden (mozreview-request)

Updated

10 months ago
Assignee: nobody → carol.ng

Comment 23

10 months ago
mozreview-review
Comment on attachment 8980399 [details]
Bug 694735 - Change default Accept header to json in Sync API requests

https://reviewboard.mozilla.org/r/246564/#review252690

Looking good! Thank you.

::: services/common/tests/unit/test_restrequest.js:566
(Diff revision 1)
> +  Assert.equal(request.response.status, 200);
> +  Assert.equal(request.response.body, "");
> +
> +  let accept_header = handler.request.getHeader("accept");
> +
> +  Assert.equal(accept_header.indexOf("text/html"), -1);

Nit: can we use |String.prototype.includes()| instead?
Attachment #8980399 - Flags: review?(eoger) → review+
Comment hidden (mozreview-request)

Comment 25

10 months ago
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6ad743c0dca0
Change default Accept header to json in Sync API requests r=eoger

Comment 26

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6ad743c0dca0
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.