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

Make 'Bad Request' body consistent and easy to understand. #40

Closed
samuelimoisili opened this issue Feb 24, 2023 · 6 comments · Fixed by #44
Closed

Make 'Bad Request' body consistent and easy to understand. #40

samuelimoisili opened this issue Feb 24, 2023 · 6 comments · Fixed by #44
Assignees

Comments

@samuelimoisili
Copy link
Contributor

samuelimoisili commented Feb 24, 2023

When I tried with all the required fields I got the following response:
Request:
POST http://localhost:5001/mediator/patient
{ "_id": "bbca4173-b914-43b6-9fcb-437aa8773f38", "sex":"female", "name":"Lorena"}
Response code: 200 OK
Response Body:

{
    "status": 400,
    "patient": {
        "message": "Invalid 'date_of_birth' range: received undefined"
    }
}

Originally posted by @lorerod in #19 (comment)

@samuelimoisili
Copy link
Contributor Author

samuelimoisili commented Mar 2, 2023

@njogz I'm considering using yup over joi. yup seems to be more flexible, it gives you the option to configure the error message.

const joi = require("joi");
const yup = require("yup");
const fs = require("fs/promises");

const test = {
  age: "wrong_age",
  // name is not present.
};

function validateAndSaveYupSchema() {
  const schema = yup.object({
    name: yup.string().required("'name' is required"),
    age: yup.number().required("'age' is required"),
  });

  try {
    schema.validateSync(test, );
  } catch (error) {
    fs.writeFile("yup.json", JSON.stringify(error.errors, null, 4));
  }
}

function validateAndSaveJoiSchema() {
  const schema = joi.object({
    name: joi.string().required("'name' is required"),
    age: joi.number().required("'age' is required"),
  });

  const error = schema.validate(test, );
  fs.writeFile("joi.json", JSON.stringify(error.error.details, null, 4));
}

validateAndSaveJoiSchema();
validateAndSaveYupSchema();
// yup.json
[
    "'name' is required",
    "age must be a `number` type, but the final value was: `NaN` (cast from the value `\"wrong_age\"`)."
]
// joi.json

[
    {
        "message": "\"name\" is required",
        "path": [
            "name"
        ],
        "type": "any.required",
        "context": {
            "label": "name",
            "key": "name"
        }
    },
    {
        "message": "\"age\" must be a number",
        "path": [
            "age"
        ],
        "type": "number.base",
        "context": {
            "label": "age",
            "value": "wrong_age",
            "key": "age"
        }
    }
]

@njogz
Copy link
Contributor

njogz commented Mar 2, 2023

Even with Joi we can pass in custom error messages e.g.

const schema = joi.object({
    name: joi.string().required("'name' is required").error(new Error("name is not valid"),
    age: joi.number().required("'age' is required").error(new Error("age is not valid"),
  });

Joi seems to have a richer api and is more popular for backend applications so I prefer to keep it over yup. Zod looks interesting since it's typescript first and can work with Joi so that may be worth a look.

Looking at the comment from Lorena it seems the main issue is the confusing status code which is a 200 but the body indicates there is an error. So not necessarily the error message from Joi. Let's send back the correct status code if there is an error in validation.

@samuelimoisili
Copy link
Contributor Author

@njogz Thanks for the Joi error suggestion.

Looking at the comment from Lorena it seems the main issue is the confusing status code which is a 200 but the body indicates there is an error.

Yeah, but I wanted to standardise the error response along with the request body. I'll use the error syntax you shared for Joi.

@ngaruko ngaruko self-assigned this Mar 5, 2023
@lorerod lorerod self-assigned this Mar 7, 2023
@lorerod
Copy link
Contributor

lorerod commented Mar 7, 2023

Environment: MacOS 13.1 (22C65); Docker desktop 4.15.0 (93002)l; Docker engine: 20.10.21
CHT 4.1.0: Local using docker compose files cht-core.yml and cht-couchdb.yml
interoperability branch: 40-bad-request

Trying to create a patient without required field date of birth ✅

Request body:

{ "_id": "bbca4173-b914-43b6-9fcb-437aa8773f38", "sex":"female", "name":"Lorena"}

Response body:

{
    "message": "\"date_of_birth\" is required"
}
Trying to create a patient with an incorrect value for sex field ✅

Request body:

{ "_id":"0123", "sex":"lorena", "name":"123"}

Response body:

{
    "message": "\"sex\" must be one of [male, female, other, unkown]"
}
Trying to create a patient with numeric _id field ✅

Request body:

{ "_id": 123, "sex":"male", "name":"123"}

Response body:

{
    "message": "\"_id\" must be a string"
}
Trying to create a patient with bad format of required field date of birth ⚠️

Request body:

{ "_id": "bbca4173-b914-43b6-9fcb-437aa8773f38", "sex":"female", "name":"Lorena", "date_of_birth":"1983/06/11"}

Response body:

{
    "message": "\"date_of_birth\" with value \"02-08-1983\" fails to match the required pattern: /^d{4}-(0[1-9]|1[012])-(0[1-9]|[12][0-9]|3[01])$/"
}

Observation: I suggest changing the message to be clearer, not using the regular expression pattern.

Create a patient with all required field ❌

Request body:

{ "_id": "bbca4173-b914-43b6-9fcb-437aa8773f38", "sex":"female", "name":"Lorena", "date_of_birth":"1983-06-11"}

Response body:

{
    "message": "\"date_of_birth\" with value \"1983-06-11\" fails to match the required pattern: /^d{4}-(0[1-9]|1[012])-(0[1-9]|[12][0-9]|3[01])$/"
}

@samuelimoisili I understood the desired format of the date was YYYY-MM-DD, and you confirmed.
If you agree, I will send this issue back to In progress for you to look, and you can also change the message to a clearer one, please.

@samuelimoisili
Copy link
Contributor Author

@lorerod I've updated the error message and regex pattern.

@lorerod
Copy link
Contributor

lorerod commented Mar 7, 2023

interoperability branch: 40-bad-request

Trying to create a patient without required field date of birth ✅

Request body:

{ "_id": "bbca4173-b914-43b6-9fcb-437aa8773f38", "sex":"female", "name":"Lorena"}

Response body:

{
    "message": "\"date_of_birth\" is required"
}
Trying to create a patient with an incorrect value for sex field ✅

Request body:

{ "_id":"0123", "sex":"lorena", "name":"123"}

Response body:

{
    "message": "\"sex\" must be one of [male, female, other, unkown]"
}
Trying to create a patient with numeric _id field ✅

Request body:

{ "_id": 123, "sex":"male", "name":"123"}

Response body:

{
    "message": "\"_id\" must be a string"
}
Trying to create a patient with bad format of required field date of birth ✅

Request body:

{ "_id": "bbca4173-b914-43b6-9fcb-437aa8773f38", "sex":"female", "name":"Lorena", "date_of_birth":"1983/06/11"}

Response body:

{
    "message": "Invalid date expecting YYYY-MM-DD"
}
Create a patient with all required field ✅

Request body:

{ "_id": "bbca4173-b914-43b6-9fcb-437aa8773f38", "sex":"female", "name":"Lorena", "date_of_birth":"1983-06-11"}

Response code: 200 ok
Response body:

{
    "resourceType": "Patient",
    "id": "1",
    "meta": {
        "versionId": "1",
        "lastUpdated": "2023-03-07T16:46:31.151+00:00"
    },
    "text": {
        "status": "generated",
        "div": "<div xmlns=\"http://www.w3.org/1999/xhtml\"><div class=\"hapiHeaderText\">Lorena <b>LORENA </b></div><table class=\"hapiPropertyTable\"><tbody><tr><td>Identifier</td><td>bbca4173-b914-43b6-9fcb-437aa8773f38</td></tr><tr><td>Date of birth</td><td><span>11 June 1983</span></td></tr></tbody></table></div>"
    },
    "identifier": [
        {
            "system": "cht",
            "value": "bbca4173-b914-43b6-9fcb-437aa8773f38"
        }
    ],
    "name": [
        {
            "use": "official",
            "family": "Lorena",
            "given": [
                "Lorena"
            ]
        }
    ],
    "gender": "female",
    "birthDate": "1983-06-11T00:00:00.000Z"
}

This looks good @samuelimoisili thanks!

@ngaruko ngaruko removed their assignment Mar 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants