1400999 - [tc-lib-api] Do not set application/json for empty responses or DELETE methods

Status: RESOLVED FIXED

(Taskcluster :: Services, defect)

Description

[Dustin J. Mitchell [:dustin] (he/him)]

In particular, it responds with 204's with empty bodies, which tc-client does not like.  Better to reply with an empty JSON body.  And follow the other best-practices.

Comment 1

[Jonas Finnemann Jensen (:jonasfj)]

Sounds like a taskcluster-client-web bug

Comment 2

[:Eli Perelman]

The problem with this isn't that the client libraries don't handle the data correctly, it's that the response says the content-type is appication/json, but returns an empty body which is not parseable as JSON.

Comment 4

[Jonas Finnemann Jensen (:jonasfj)]

Probably we should fix this.

Comment 5

[Dustin J. Mitchell [:dustin] (he/him)]

The work here is to make sure that every API method that returns nothing either returns an empty JSON document (`{}`) or does not set content-type: application/json.  Ideally tc-lib-api can fix this automatically, but it would be OK for it to check this on every request, and we will get tests failing where it's not the case.

Comment 6

[Dustin J. Mitchell [:dustin] (he/him)]

This is still quite relevant -- hopefully someone will hack on it soon!

Comment 7

[Arshad Kazmi [:arshadkazmi42]]

I am interested to take this up.
Is it still available?

Comment 8

[Dustin J. Mitchell [:dustin] (he/him)]

Sure is!

Comment 9

[Arshad Kazmi [:arshadkazmi42]]

Cool. Where can I find the code for this? On Github? Any suggestions on where can I start working on it?

Comment 10

[Dustin J. Mitchell [:dustin] (he/him)]

The code is in https://github.com/taskcluster/taskcluster-lib-api.  Comment 5 contains some ideas for how to fix this.

Comment 11

[Arshad Kazmi [:arshadkazmi42]]

Dustin, I went through the code base, but couldn't find `204` status code anywhere in the code.
I tried debugging route_test.js, all the tests are returning `{}` body response except the api which is giving 400 bad requests.

Can you guide, where exactly I need to be looking for this? Or is there a way to reproduce the issue?

Comment 12

[Dustin J. Mitchell [:dustin] (he/him)]

Hm, this is the line that originally caused the complaints:

https://github.com/taskcluster/taskcluster-purge-cache/blob/master/src/api.js#L83
 res.status(204).send();

The tc-lib-api docs say "Note: use of res.status(4..).json(..) to return error statuses is an anti-pattern. While you may see older code that still follows this pattern, do not repeat it!" (https://github.com/taskcluster/taskcluster-lib-api#request-handlers) and in general I think API methods shouldn't be using res.status(..) at all, preferring res.reply.

I tested this out in the tools site, which was the original complaint.  I added a new purge-cache, and watched in the Firefox network inspector to see the response.  It's a 204, without a content-type header, and with content-length 0.  So, I think this is in fact 100% fine already.  So it seems this was accidentally fixed somewhere along the way?

But I think we can make this a little better.  Let's allow

  res.reply();

to reply with a 204.  Then callers won't need to use `res.status(204)` anymore.  So, can you add some support for that in tc-lib-api, including a test?  Once that's done, we can upgrade the services that currently use `res.status(204)` to use it.

Comment 13

[Arshad Kazmi [:arshadkazmi42]]

So, for allowing 204 in res.reply,  I am thinking of adding a condition here

https://github.com/taskcluster/taskcluster-lib-api/blob/master/src/middleware/schema.js#L85


```
      // If JSON was valid or validation was skipped then reply with 200 OK
      if (res.statusCode === 204) {
        res.status(204).json(json)
      } else {
        res.status(200).json(json);
      }
```

Any thoughts on this?

Comment 14

[Dustin J. Mitchell [:dustin] (he/him)]

How would statusCode get set to 204?  I think the condition to check for is that the argument to `res.reply` is undefined.

Comment 15

[Arshad Kazmi [:arshadkazmi42]]

You mean adding something like this, I am new to this repo so got alot of doubts :P

```
if (!json) {
  return res.status(204).json(json);
}
// If JSON was valid or validation was skipped then reply with 200 OK
res.status(200).json(json);
```

Comment 16

[Dustin J. Mitchell [:dustin] (he/him)]

The `!json` part is right, but I think you will want a simple `.send()` after that rather than `.json(json)` (which will probably fail).

Comment 17

[Arshad Kazmi [:arshadkazmi42]]

I was working on the tests, I don't see any test suite in which this can be done. 
Is there any existing suite which can support this test? Or I need to create a new test suite?

Comment 18

[Dustin J. Mitchell [:dustin] (he/him)]

https://github.com/taskcluster/taskcluster-lib-api/blob/master/test/validate_test.js is probably a good choice (since in a way this is "validating" a non-JSON response)

Comment 19

[Arshad Kazmi [:arshadkazmi42]]

Created a PR here

https://github.com/taskcluster/taskcluster-lib-api/pull/145

Comment 20

[Dustin J. Mitchell [:dustin] (he/him)]

OK, I've released tc-lib-api as version 12.7.0.

Next up is to upgrade tc-lib-api in a few applications and use this new feature.  From grepping in my repositories:

p/taskcluster-auth/src/v1.js:  return res.status(204).send();
p/taskcluster-auth/src/v1.js:  return res.status(204).send();
p/taskcluster-purge-cache/src/api.js:  res.status(204).send();

I think we need to do this for taskcluster/taskcluster-purge-cache and taskcluster/taskcluster-auth.  Do you want to make up some quick PR's for those two repositories?

Comment 21

[Arshad Kazmi [:arshadkazmi42]]

Yes. I would love to work on it.

Do we need to add a new issue for it in here? Or we are going to make those changes as part of this bug itself?

Comment 22

[Dustin J. Mitchell [:dustin] (he/him)]

We can keep the same bug (this one).

Comment 23

[Arshad Kazmi [:arshadkazmi42]]

Created PR in both the repos here

https://github.com/taskcluster/taskcluster-auth/pull/187

https://github.com/taskcluster/taskcluster-purge-cache/pull/36

Comment 24

[Arshad Kazmi [:arshadkazmi42]]

Above to PR's are merged. 
Does this bugs status needs to be updated?

Comment 25

[Dustin J. Mitchell [:dustin] (he/him)]

Yep, I think we're done here.  Thanks!

I noticed you have Joi-related repo in your github org.  If schemas are of interest to you, there are some issues on
  https://github.com/taskcluster/react-schema-viewer
that you might have a look at.  Otherwise, please let me know if I can help you find a next step!

Comment 26

[Arshad Kazmi [:arshadkazmi42]]

Thanks Dustin.
I will look into the react-schema-viewer repo issues.

I am interested to work in other repos also.
It will be great, if I can get something for next step.