Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Empty response if result is null #998

Closed
damianobarbati opened this issue Jun 9, 2017 · 20 comments
Closed

Empty response if result is null #998

damianobarbati opened this issue Jun 9, 2017 · 20 comments

Comments

@damianobarbati
Copy link

damianobarbati commented Jun 9, 2017

Why the following returns an empty response? null is a valid json value.

const config = require('./package.json');

const http = require('http');
const koa = require('koa');
const cors = require('kcors');
const compress = require('koa-compress')
const noTrailingSlash = require('koa-no-trailing-slash');
const json = require('koa-json');
const body = require('koa-body');
const send = require('koa-send');
const router = require('koa-router')();

const app = new koa();

app.use(cors());
app.use(compress());
app.use(noTrailingSlash());
app.use(json({ pretty: true, spaces: 4 }));
app.use(body({ formLimit: '5mb', jsonLimit: '5mb', strict: false, multipart: true }));

app.use(async (ctx, next) => {
    try {
        await next();
    }
    catch(error) {
        ctx.status = 400;
        ctx.body = error.message || error;
    }
});

router.all('/ciao', async ctx => {
    ctx.body = null;
});

app.use(router.routes());
http.createServer(app.callback()).listen(8080);

Reponse is empty:

macbook:react-app damiano$ curl -v localhost:8080/ciao
*   Trying ::1...
* TCP_NODELAY set
* Connected to localhost (::1) port 8080 (#0)
> GET /ciao HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.51.0
> Accept: */*
> 
< HTTP/1.1 204 No Content
< Vary: Origin, Accept-Encoding
< Date: Fri, 09 Jun 2017 14:17:57 GMT
< Connection: keep-alive
< 
* Curl_http_done: called premature == 0
* Connection #0 to host localhost left intact
macbook:react-app damiano$ 
@damianobarbati
Copy link
Author

Nobody experiencing this?! :|

@fengmk2
Copy link
Member

fengmk2 commented Jun 13, 2017

null body meaning no content on koa https://github.com/koajs/koa/blob/master/lib/response.js#L139

@damianobarbati
Copy link
Author

damianobarbati commented Jun 13, 2017

@fengmk2 that's not good: what about an API endpoint returning "null" as data?
Shouldn't koa allow some kind of config about it? @tj @jonathanong
null is a valid json value but returning a 204 will trigger problems front-end side.
We cannot assume everybody is enclosing result in an object.

@tj
Copy link
Member

tj commented Jun 13, 2017

Just my opinion but personally I most people expect an object or array with json these days, apis returning primitives is a little weird, even if it's technically fine. At very least something like { "user": null } so that the null is self-describing

@damianobarbati
Copy link
Author

@tj I agree, but in some cases it may be needed and returning a 204 could be unexpected.
Do you think koa team will consider this or will this be ignored assuming everybody must envelop
the data?
Because in the latter I'll just close the issue (even if I don't agree much) :)

@tj
Copy link
Member

tj commented Jun 13, 2017

I don't have a super strong opinion there, I guess maybe ideally the client requesting stuff treats 204 as null but it's definitely easier to assume it's always JSON, though that's sort of the same weirdness as doing { "error": MESSAGE } instead of a 4xx or 5xx with MESSAGE as the body which is maybe a bit more correct

@jonathanong
Copy link
Member

it makes way more sense to me to return 204 or an error instead of null. i find anything but an array or object being returned from an API weird.

is there absolutely no way you can change the FE code? this would be a breaking change

@damianobarbati
Copy link
Author

damianobarbati commented Jun 14, 2017

@jonathanong if this would cause a breaking change then let's just leave it like this but I'm not sure pretending "always enveloped results" is right.

My 2 cent: could koa check if request header is application/json and if so just encode null instead of returning 204 empty content? I think it's the correct behaviour.

@damianobarbati
Copy link
Author

what do you think about it @tj? Could koa check if request header is application/json and if so just encode null instead of returning 204 empty content? I think it's the correct behaviour

@tj
Copy link
Member

tj commented Jun 15, 2017

Sounds reasonable, my main fear there is that it gets (even more) magical when it starts doing multiple things. I kind of agree that array/objects are the way to go, even if technically less correct

@damianobarbati
Copy link
Author

@tj @jonathanong please let me know on this: if you think this is never going to be implemented I'll just close the issue.
Thanks guys for your efforts!

@damianobarbati
Copy link
Author

@tj @jonathanong time to close this? Did you take time to think if it's worthful or not?

@jonathanong
Copy link
Member

Could koa check if request header is application/json and if so just encode null instead of returning 204 empty content? I think it's the correct behaviour

that sounds like a good behavior to have. if a content type is set, then the developer intends to set a body.

my only concern is whether this would be a breaking change, which it probably will be for a few people

@lichangwei
Copy link

Below is my case:

request(options, function(error, response, body){
    //...
    ctx.status = response.statusCode; //500
    ctx.body = body; //undefined
    console.log(ctx.status); //204, it confused me
    //...
});

if ctx.body == null and ctx.status is 2XX or not set, we can set ctx.status to 204, otherwise it's better do nothing.

@damianobarbati
Copy link
Author

After more than 2 years here I am again xD
I'm trying to return in the json body null but I see koa returning 204.
@lichangwei how did you solve this?

@pke
Copy link

pke commented Nov 21, 2019

I'm willing to provide a PR. I saw this in the 3.0 roadmap initial issue #1114 as a breaking change.
Instead of breaking the current behaviour I'd suggest that to be a config option one can set when creating the koa app. That way it would not break old clients and clients that need it can enabled it via emptyBodyAs204=false (default is true). Coming up with a good config name was difficult (as naming often is) then I thought maybe one could register

@samtsai15
Copy link

OK , If ctx.body = null equals 204

middleware: 

( async (ctx) => {

ctx.body = null;
ctx.body = 'have result'
return ctx;

})

it still return 204 , anyone have this issue ?

@antixrist
Copy link

antixrist commented Dec 22, 2019

@samtsai15 I handle it something like this:

const statuses = require('statuses');

// ...

app.use(async (ctx, next) => {
  await next();

  if (ctx.fresh) {
    ctx.status = 304;
    ctx.body = null;
  } else if (ctx.length && statuses.empty[ctx.status]) {
    ctx.status = 200;
  }
});

@maapteh
Copy link

maapteh commented Feb 24, 2021

@tj are you willing to solve this issue? We have some microservices in GraphQL and null is a perfect return value when it fits the contract. Im now wrapping Koa results back myself, so not a real dealbreaker but it's weird to see when explicitly returning null it magically disappears, and even worse gets another statusCode because of it.

Also contractually now it doesn't fit, normally with service i do <Foo | null> and the contract is then consumed in the other mesh/gateway which now also requires manual work :(. I really liked the performance setup many years ago, but this really surprises me.

@damianobarbati
Copy link
Author

@maapteh after 4 years I gave up and came up with a https://www.npmjs.com/package/koa-better-json

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants