prevent body format errors and wasted json parsing on correct but "non json" repsonses #95
Conversation
There was a problem hiding this comment.
Thanks for submitting this PR!
At a high-level the comments boil down to:
- Does this still do response model validation for users who are sending JSON but haven't explicitly set the
Content-Typeresponse header? - Let's rename some symbols for clarity
- Let's not include the yarn.lock file as part of this PR
Then, one last request:
- let's add a test that shows the current bad behavior and proves we've corrected it
handlers/_swagger.js
Outdated
| logger.warn('Invalid content type specified: %s. Expecting one of %s', type, allowedTypes); | ||
| } | ||
| // Wrap the set function, which is responsible for setting headers | ||
| var type = ''; |
There was a problem hiding this comment.
for clarity, I think it would be good to call the variable something like responseContentType
handlers/_swagger.js
Outdated
| }); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| if (responseModelValidationLevel) { | ||
| var responseSender = res.send; | ||
| res.send = function (body) { | ||
| var isBodyValid = isValidDataType(body); |
There was a problem hiding this comment.
it looks like it might be good to refactor this function's name, maybe to isValidJsonData
With that change, it may make sense to move the check for 'application/json' to the outermost extent to protect the subsequent code. However, I am not sure how that will affect users who just call res.send without explicitly setting the Content-Type ...
handlers/_swagger.js
Outdated
| res.send = responseSender; | ||
| responseSender.call(res, body); | ||
| return; | ||
| if ( type === 'application/json') { |
There was a problem hiding this comment.
There's an extra space between the bracket and type ...
Also, how does this affect users who are not explicitly setting their response content type?
Has express already set that header by this time?
Above, should the default value for type be 'application/json'?
Or should this check be more like: if (!type || type === 'application/json')?
yarn.lock
Outdated
| @@ -0,0 +1,2212 @@ | |||
| # THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY. | |||
There was a problem hiding this comment.
let's not add this file as part of this PR - please add it to the .gitignore file for now.
|
@seanpk i agree on the changes, before i make the fixes we should agree on behaviour on unset content-type: |
|
@lucidNTR - yes, I think by treating unset response content-type as application/json, the change will be backward compatible for existing code that doesn't set it explicitly. Thanks again. |
|
i made the first set of changes as you had in mind.
|
|
@seanpk ok i found the problem: _swagger.js : line 354 What is your recommended next step? |
|
@lucidNTR - can you provide more details on the problem? Looking at that code in the context of this PR, are you looking to add a check, and subsequent warning, if the If that is what you're asking about, after doing the check, we could call |
|
@seanpk that was exactly my idea, but i have to admit the error handling is a bit confusing to me with 3 levels of indirection and hirarchical validation errors im struggling to find the right way to add the new synthetic subError at the moment. |
|
i start to get the feeling this is a whole new PR with its own caveats. to finish this PR in a timely manner, would you be OK if i just add the possibility to check for console log errors in integration tests and make sure the tests fail first and then succeed with this PR? |
|
@lucidNTR - yes, I can support that - I think there are other tests that check the log for errors now already (or if not in this repo, I've seen the pattern before) Thanks again for your diligence and contributions! |
currently non json responses will break the server and always tried to be parsed as json, even when the content type says they are not in json format.
this PR fixes that behaviour