hapijs / joi

The most powerful data validation library for JS
Other
20.75k stars 1.51k forks source link

fix: do not override existing labels of underlying schemas in alternatives #3001

Closed hajekjiri closed 5 months ago

hajekjiri commented 8 months ago

When using labels on underlying schemas of labeled Joi.alternatives, the Joi.alternatives' label overwrites the labels of the underlying schemas. I think this is a bug and therefore propose the changes in this PR.

Based on reading existing tests, I assume passing labels from Joi.alternatives to underlying schemas is wanted, though I do not believe this should be the case when the underlying schemas already have their own labels. The fact that I am not breaking any existing tests with these changes further reinforces my belief that this may be a bug. I only added a few new tests to cover the behavior introduced in my changes.

Background:

In my project, I am using joi-to-swagger to convert Joi schemas to OpenAPI-compatible schemas. In case of Joi.alternatives, this leads to anyof or oneOf (meaning this property can have any/one of the following schemas) , where the schema labels are useful to be able to differentiate the specific alternatives at first glance instead of having a default generic label for each one (object, string, number, ...). When I use nested Joi.alternatives, the nested alternatives all take the parent's label, which results in me being unable to tell the difference at first glance when reading the generated OpenAPI documentation (see screenshots below).

Take the following example:

const Joi = require('./lib/index.js');

const valueSchema = Joi.object().keys({
  value: Joi.string().required(),
});

const notFoundErrorSchema = Joi.object().keys({
  code: Joi.string().required(),
  resource: Joi.string().required(),
});

const forbiddenErrorSchema = Joi.object().keys({
  code: Joi.string().required(),
  user: Joi.string().required(),
});

const errorSchema = Joi.alternatives().try(
  notFoundErrorSchema.label('NOT FOUND ERROR'),
  forbiddenErrorSchema.label('FORBIDDEN ERROR'),
);

const responseSchema = Joi.alternatives().try(
  valueSchema.label('VALUE'),
  errorSchema.label('ERROR'),
);

console.log(JSON.stringify(responseSchema.describe(), null, 2));
Current output
{
  "type": "alternatives",
  "matches": [
    {
      "schema": {
        "type": "object",
        "flags": {
          "label": "VALUE"
        },
        "keys": {
          "value": {
            "type": "string",
            "flags": {
              "presence": "required"
            }
          }
        }
      }
    },
    {
      "schema": {
        "type": "alternatives",
        "flags": {
          "label": "ERROR"
        },
        "matches": [
          {
            "schema": {
              "type": "object",
              "flags": {
                "label": "ERROR"  // <----------------------------------------
              },
              "keys": {
                "code": {
                  "type": "string",
                  "flags": {
                    "presence": "required"
                  }
                },
                "resource": {
                  "type": "string",
                  "flags": {
                    "presence": "required"
                  }
                }
              }
            }
          },
          {
            "schema": {
              "type": "object",
              "flags": {
                "label": "ERROR"  // <----------------------------------------
              },
              "keys": {
                "code": {
                  "type": "string",
                  "flags": {
                    "presence": "required"
                  }
                },
                "user": {
                  "type": "string",
                  "flags": {
                    "presence": "required"
                  }
                }
              }
            }
          }
        ]
      }
    }
  ]
}
Expected output
{
  "type": "alternatives",
  "matches": [
    {
      "schema": {
        "type": "object",
        "flags": {
          "label": "VALUE"
        },
        "keys": {
          "value": {
            "type": "string",
            "flags": {
              "presence": "required"
            }
          }
        }
      }
    },
    {
      "schema": {
        "type": "alternatives",
        "flags": {
          "label": "ERROR"
        },
        "matches": [
          {
            "schema": {
              "type": "object",
              "flags": {
                "label": "NOT FOUND ERROR"  // <----------------------------------------
              },
              "keys": {
                "code": {
                  "type": "string",
                  "flags": {
                    "presence": "required"
                  }
                },
                "resource": {
                  "type": "string",
                  "flags": {
                    "presence": "required"
                  }
                }
              }
            }
          },
          {
            "schema": {
              "type": "object",
              "flags": {
                "label": "FORBIDDEN ERROR"  // <----------------------------------------
              },
              "keys": {
                "code": {
                  "type": "string",
                  "flags": {
                    "presence": "required"
                  }
                },
                "user": {
                  "type": "string",
                  "flags": {
                    "presence": "required"
                  }
                }
              }
            }
          }
        ]
      }
    }
  ]
}
Resulting OpenAPI documentation
Current behavior

image

Expected behavior

image

Marsup commented 5 months ago

I agree that it's probably what anyone would expect. Thanks for the PR!