medic / cht-interoperability

CHT - eCHIS interoperability project
GNU Affero General Public License v3.0
2 stars 3 forks source link

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

Closed samuelimoisili closed 1 year ago

samuelimoisili commented 1 year ago

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 https://github.com/medic/interoperability/issues/19#issuecomment-1443700671

samuelimoisili commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

@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.

lorerod commented 1 year ago

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 :x: 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 commented 1 year ago

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

lorerod commented 1 year ago

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": "
Lorena LORENA
Identifierbbca4173-b914-43b6-9fcb-437aa8773f38
Date of birth11 June 1983
" }, "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!