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

applyFilter: handle empty payloads #338

Merged
merged 3 commits into from Apr 27, 2015
Merged

applyFilter: handle empty payloads #338

merged 3 commits into from Apr 27, 2015

Conversation

@dvic
Copy link
Contributor

dvic commented Apr 25, 2015

Fixes #337. This handles the case when a payload is null (e.g., not posted with a POST method or masked by Hapi in case of GET method).

By the way: is it correct that Hapi passes null for the payload when a GET method is performed?

@dvic dvic force-pushed the dvic:patch-1 branch from 69a829f to bc7503e Apr 25, 2015
@@ -87,6 +87,8 @@ exports.GreatResponse = function (request, options, filterRules) {

var applyFilter = function (data) {

if(!data || Object.keys(data).length === 0) return;

This comment has been minimized.

Copy link
@arb

arb Apr 26, 2015

Contributor

I think the check should be for !(data && typeof data === 'object'). While null or undefined cause an issue, plain string responses or payloads cause the problem too.

Also, please check the style guide. If statements must have brackets around the body.

This comment has been minimized.

Copy link
@dvic

dvic Apr 26, 2015

Author Contributor

I added also the condition Object.keys(data).length > 0, as empty objects also seem to cause this bug.


it('handles empty request payloads', function (done) {

generateGreatResponse(null, {message: 'test'});

This comment has been minimized.

Copy link
@arb

arb Apr 26, 2015

Contributor

Please add tests for plain strings as well.

it('handles empty request payloads', function (done) {

generateGreatResponse(null, {message: 'test'});
generateGreatResponse({}, {message: 'test'});

This comment has been minimized.

Copy link
@arb

arb Apr 26, 2015

Contributor

Style guide -> { message: 'test' }

@arb arb added this to the 6.1.2 milestone Apr 26, 2015
@arb arb self-assigned this Apr 26, 2015
@arb arb added the bug label Apr 26, 2015
describe('GreatResponse()', function () {

var generateGreatResponse = function (requestPayload, responsePayload) {
var filterRules = {

This comment has been minimized.

Copy link
@arb

arb Apr 27, 2015

Contributor

New line after function

@arb

This comment has been minimized.

Copy link
Contributor

arb commented Apr 27, 2015

After some experimenting, I found a better fix for this: change the first few lines of the forEach loop to check for the root node:

if (this.isRoot) {
   return;
}

Then you won't need any of the other checks. I'd like to avoid anything Object.keys lookup which is why it seems like I'm obsessing over this.

@arb

This comment has been minimized.

Copy link
Contributor

arb commented Apr 27, 2015

Good find, thanks for the PR!

arb added a commit that referenced this pull request Apr 27, 2015
applyFilter: handle empty payloads
@arb arb merged commit 9693e40 into hapijs:master Apr 27, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.