search

apigee-127 / swagger-node-runner

The heart of Swagger-Node
MIT License
102 stars 123 forks source link

204 No Content #79

Open osher opened 7 years ago

osher commented 7 years ago

There's a codepath I'm not comfortable with, I need your take on this.

from W3 spec:

The 204 response MUST NOT include a message-body, and thus is always terminated by the first empty line after the header fields.

So, in contrast, IMHO - the case slips our code here: https://github.com/theganyo/swagger-node-runner/blob/master/lib/connect_middleware.js#L87

the problem is that if ctx.output is falsful - including zero (0), empty string (""), null, undefined, and obviously false - the respondse.end() is not called, where the last in chain-of-responsibility is usually the framework, wich ends with it's default response, usually 404 in the form of cannot GET /the-path.

All of the above (except undefined maybe) are legit replies the server may be required to provide, regardless to 204 status code, where there should be cases where the server should emit no body.

Am doing something wrong or missing anything?

If I'm not missing anything - LMK, and I'll start a PR to fix this...

theganyo commented 7 years ago

Yes, I agree. It shouldn't be relying on a falsy condition... it should be more specific than that.

osher commented 7 years ago

IMHO, when a request is mapped to a swagger-operation - it should not leave the pipe without emitting a response.

Am I missing something? Can you think of a case where this assumption fails?

If so - then there should not be such a condition. I mean - we can argue what translate(body,mime) does with nulls and undefineds, but the more I consider it - I'm convinced: The 404 > Cannot PUT /my-svc/path is not a correct reply from the web-server

I believe that having the request mapped to an operation - response.end() must be called and a response must be provided, be it empty or not Edited: and the next callback should be called

Can we agree on this?

theganyo commented 7 years ago

Hmm. Well, the theory was that we're still trying to operate as a good citizen within the framework that exists and not completely take control... so it was following the "do no harm" doctrine and only writing when it was told to. I'm also pretty certain that there are developers that expect to hook in regular connect middleware after the pipe runs.

That said, you're completely right... an error such as you have above makes no sense at all. So the question is: Is it safe to assume that the response can be ended? And.. honestly, I'm not sure. Would it solve the issue to just call write on the response instead of end?