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

allow form parameters and missing body is a string #19

Closed
wants to merge 2 commits into from

Conversation

hans-d
Copy link

@hans-d hans-d commented Sep 5, 2014

see #18

  • req.body defaults to {} when no body is present. Results in 'invalid type'. A {} is now converted to '' (empty string)
  • parameter types not a path or query where all mapped to req.body. In case of form, it now reads from the body object (eg when using app.use(bodyParser.urlencoded({ extended: true })))

@hans-d hans-d mentioned this pull request Sep 5, 2014
@tlivings
Copy link
Contributor

tlivings commented Sep 5, 2014

What type are you defining it as? If it is required, and it is empty, then it will be an error. If it is not empty, and it doesn't fit the model, it will be an error as well.

Form data will map to an object using urlencoded, in which case you do not want to specify the type as string. Instead, you want to specify it as a model which can be validated.

For example:

"parameters": [
    {
        "name": "body",
        "description": "some form data",
        "required": true,
        "type": "Form",
        "paramType": "body"
    }
]

and then...

"Form": {
    "id": "Form",
    "properties": {
        "param1": {
            "type": "string"
        },
        "param2": {
            "type": "string"
        }
    }
}

@tlivings
Copy link
Contributor

tlivings commented Sep 5, 2014

I think the proper fix here is to do the following:

var value, isPath;

isPath = parameter.paramType === 'path' || parameter.paramType === 'query';

switch (parameter.paramType) {
    case 'path':
    case 'query':
        value = req.param(parameter.name);
        break;
    case 'header':
        value = req.header(parameter.name);
        break;
    case 'body':
    case 'form':
        value = req.body;
        //If the type is string and we found an empty object, convert to empty string.
        //This is a bug in express's body-parser: https://github.com/expressjs/body-parser/issues/44
        if (parameter.type === 'string') {
            if (thing.isObject(value) && !Object.keys(value).length) {
                value = '';
                break;
            }
            value = JSON.stringify(value);
        }
}

You can make this change or I can. But I'd also like to add tests for each of those cases.

@hans-d
Copy link
Author

hans-d commented Sep 5, 2014

As I'm not yet familiar with your testing framework, and how you've setup the tests it might take some time for me to write the tests. Switching to switch is indeed a better idea and less dirty.

As for the paramType with form that is not yest covered with your fix. The example you provide is still with paramType body, and will indeed work. For form to work (it then also needs a app.use(bodyParser.urlencoded({ extended: true }))):

                    case 'body':
                        value = req.body;
                        //If the type is string and we found an empty object, convert to empty string.
                        //This is a bug in express's body-parser: https://github.com/expressjs/body-parser/issues/44
                        if (parameter.type === 'string') {
                            if (thing.isObject(value) && !Object.keys(value).length) {
                                value = '';
                                break;
                            }
                            value = JSON.stringify(value);
                        }
                        break;
                    case 'form':
                        value = req.body[parameter.name]
                        break;

@tlivings
Copy link
Contributor

tlivings commented Sep 5, 2014

If you don't mind I will close this PR and submit my own which contains the tests as well as other fixes.

@tlivings
Copy link
Contributor

tlivings commented Sep 5, 2014

Added #20

@hans-d
Copy link
Author

hans-d commented Sep 5, 2014

don't mind, as long as it gets fixed :-)

@hans-d
Copy link
Author

hans-d commented Sep 5, 2014

(or I might misunderstand the form paramtype)

@tlivings tlivings closed this Sep 5, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants