fix(ext/http): Throwing Error if the return value of Deno.serve handler is not a Response class #21099
Conversation
|
|
ext/http/00_serve.js
Outdated
| @@ -449,6 +450,11 @@ function mapToCallback(context, callback, onError) { | |||
| fromInnerRequest(innerRequest, signal, "immutable"), | |||
| new ServeHandlerInfo(innerRequest), | |||
| ); | |||
|
|
|||
| // Throwing Error if the handler return value is not a Response class | |||
| if(!(response && ObjectPrototypeIsPrototypeOf(ResponsePrototype, response))) { | |||
There was a problem hiding this comment.
This doesn't need a null check as ObjectPrototypeIsPrototypeOf will return false here:
| if(!(response && ObjectPrototypeIsPrototypeOf(ResponsePrototype, response))) { | |
| if (!ObjectPrototypeIsPrototypeOf(ResponsePrototype, response)) { |
There was a problem hiding this comment.
I checked it throwed another error, isnt it? Seems ObjectPrototypeIsPrototypeOf throw error if the input is undefined instead of returning false..
There was a problem hiding this comment.
Seems ObjectPrototypeIsPrototypeOf throw error if the input is undefined instead of returning false..
I believe that's only if the first parameter is null or undefined. The second parameter may be null or undefined:
> WebSocket.prototype.isPrototypeOf(a)
true
> WebSocket.prototype.isPrototypeOf(null)
false
> WebSocket.prototype.isPrototypeOf(undefined)
false
|
Changes look good, but we should add a test to |
Okay its night here, I would write a test tmrw and will let u know @mmastrac. Thanks |
| } catch (error) { | ||
| try { | ||
| response = await onError(error); | ||
| if (!ObjectPrototypeIsPrototypeOf(ResponsePrototype, response)) { |
There was a problem hiding this comment.
Added same logic to restrict to return only Response class in onError callback
|
Looks great to me. If you mark it as ready for review we can give it a run with CI! |
|
Just doing linting and formatting (got some problem with that, now it was fixed), and am running cargo test. Then i will mark this as ready for review. |
|
Hey @mmastrac, when i run cargo test some were failing the log is so big, but i mostly saw integration_test were failing. Is that something i need to do since i dint do any breaking change i believe |
|
@codesculpture Sometimes we get test flakes -- let's see what the CI says. |
|
Seems it was unexpectedly failed. Isnt it? |
|
I can take a look! I'll try to merge it today EDIT: Github is acting poorly right now, so I'll just restart the builds until we get a proper result. |
|
/bench http[minimal,realistic] |
http[minimal]
http[realistic]
start: id: server: |
|
Thank you @mmastrac |
Fixing this #20947 and #20013
We are not checking the return value of handler passed through Deno.serve and directly passing it to toInnerResponse but thats expecting a Response class . So throwing error if handler return other than Response class. (Btw, its my first PR correct me if im wrong on anything :) )