694735 - Sync: client includes default Accept header in API requests

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P3)

Description

[Richard Newman [:rnewman]]

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.

Comment 1

[Jack [:jack_jagan]]

I'd like to be assigned to this bug, could someone assign me please.

Comment 2

[Richard Newman [:rnewman]]

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

Comment 3

[Jack [:jack_jagan]]

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?

Comment 4

[Richard Newman [:rnewman]]

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!

Comment 5

[Jack [:jack_jagan]]

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?

Comment 6

[Jack [:jack_jagan]]

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.

Comment 7

[Richard Newman [:rnewman]]

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.

Comment 8

[Jack [:jack_jagan]]

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");
}

Comment 9

[Richard Newman [:rnewman]]

> Is it like…

Yes.


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

Comment 10

[g4chris]

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

Comment 11

[g4chris]

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?

Comment 12

[Richard Newman [:rnewman]]

Kit, want to mentor this?

Comment 13

[Lina Butler [:lina]]

(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

[g4chris]

Attachment: Proposed fix and test case for Bug 694735

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.

Comment 15

[Lina Butler [:lina]]

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.

Comment 16

[Lina Butler [:lina]]

Hi! Would you like to fix up the nits and submit an updated patch, or should I take care of that? Either is OK!

Comment 17

[g4chris]

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.

Comment 18

[Lina Butler [:lina]]

No worries. There's no rush at all; this bug just came up in our weekly triage.

Comment 19

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

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?

Comment 20

[Hans van den Pol]

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.

Comment 21

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

(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 22

[Carol Ng [:cng]]

Attachment: Bug 694735 - Change default Accept header to json in Sync API requests

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

Comment 23

[Edouard Oger [:eoger]]

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?

Comment 24

[Carol Ng [:cng]]

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

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

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6ad743c0dca0
Change default Accept header to json in Sync API requests r=eoger

Comment 26

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/6ad743c0dca0