belgif / rest-guide

REST Guidelines of Belgian government institutions
https://www.belgif.be/specification/rest/api-guide/
Apache License 2.0
24 stars 4 forks source link

How to represent input validation issues that relate to multiple parameters? #108

Open jpraet opened 1 year ago

jpraet commented 1 year ago

In the deprecated InvalidParamProblem, it was possible to reference multiple invalidParams.

In the new InputValidationProblem you can reference multiple issues, but each issue can only reference a single parameter (in, name, value).

Suppose that we want to report an issue about query parameters "id" and "name" that are mutually exclusive.

A possible option would be to place the parameters as key/value pairs in the "value" object as follows:

{
  "type": "urn:problem-type:belgif:badRequest",
  "href": "https://www.belgif.be/specification/rest/api-guide/problems/badRequest.html",
  "title": "Bad Request",
  "status": 400,
  "detail": "The input message is incorrect",
  "issues": [
    {
      "type": "urn:problem-type:cbss:input-validation:mutuallyExclusiveParameters",
      "title": "these parameters are mutually exclusive",
      "detail": "id and name are mutually exclusive",
      "in": "query",
      "value": {
        "id": "12345",
        "name": "test" 
      }
    }
  ]
}

That would work, but suppose that we want to report an issue about path parameter "id" that must match the "id" property in the body?

One parameter is "in": "path", and the other is "in": "body" so you cannot apply the same trick.

For these use cases, it would be useful if InputValidationIssue had a params array of {"in": "...", "name": "...", "value": "..."} objects instead of the current top-level "in", "name", "value" properties that only allow for a single param.

pvdbosch commented 1 year ago

The current InputValidationIssue defines a simple structure for the most common use case; but its fields aren't mandatory and additional properties are allowed so you can still choose an entirely different structure if the API requires it.

Different types of input validation validation issues may require different fields, so we shouldn't attempt to model all possible use cases. That said, the issue types you mentioned might be common enough in order to standardize them further. To discuss in next WG.

jpraet commented 1 year ago

At CBSS we think it would be beneficial if the belgif problem standard would support validation issues referencing multiple parameters as follows:

{
  "type": "urn:problem-type:belgif:badRequest",
  "href": "https://www.belgif.be/specification/rest/api-guide/problems/badRequest.html",
  "title": "Bad Request",
  "status": 400,
  "detail": "The input message is incorrect",
  "issues": [
    {
      "type": "urn:problem-type:cbss:input-validation:incompleteParameterGroup",
      "title": "Incomplete parameter group",
      "detail": "The parameter group [alternateId.type, alternateId.value] is incomplete",
      "params": [
        {
          "in": "query",
          "name": "alternateId.type",
          "value": null
        },
        {
          "in": "query",
          "name": "alternateId.value",
          "value": "DK13801134"
        }
      ]
    }
  ]
}
{
  "type": "urn:problem-type:belgif:badRequest",
  "href": "https://www.belgif.be/specification/rest/api-guide/problems/badRequest.html",
  "title": "Bad Request",
  "status": 400,
  "detail": "The input message is incorrect",
  "issues": [
    {
      "type": "urn:problem-type:cbss:input-validation:mismatchedParameters",
      "title": "Mismatched parameters",
      "detail": "The employerId path parameter should match the employerId body parameter",
      "params": [
        {
          "in": "path",
          "name": "employerId",
          "value": 93017373
        },
        {
          "in": "body",
          "name": "employerId",
          "value": 99999999
        }
      ]
    }
  ]
}
pvdbosch commented 1 year ago

discussed on WG, but no conclusion yet.

How frequent do this type of input validation issues occur?

Cases that are common enough, should be standardized. Others can remain API-specific.

Possible solutions for standard solution:

Examples of possible solutions:

A) using oneOf - at the problem level

InputValidationProblem:
  type: object
  allOf:
  - $ref: "#/components/schemas/Problem"
  properties:
    issues:
      type: array
      items:
        oneOf:
          - $ref: "#/components/schemas/InputValidationIssue"
          - $ref: "#/components/schemas/MultiParamInputValidationIssue"

B) using oneOf - at the InputValidationIssue level

InputValidationIssue:
  type: object
  allOf:
    - $ref: "#/components/schemas/Problem"
  properties:
    oneOf:
    - "$ref": "#/components/schemas/ParameterReference"
    - params: 
        type: array
        items:
          "$ref": "#/components/schemas/ParameterReference"

ParameterReference:
  in:
    type: string
    enum:
    - body
    - header
    - path
    - query
  name:
  type: string
  value: {} # any type allowed

C) input validation issue for PUT "full update" case with mismatching identifier

  type: urn:problem-type:belgif:input-validation:paramsShouldBeIdentical
  title: "..."
  detail: enterpriseNumber in payload should match the path parameter in the URL
  in: body
  name: enterprise.enterpriseNumber
  value: abc
  shouldMatch:
    in: path
    name: enterpriseNumber
    value: xyz
pvdbosch commented 1 year ago

With openapi-generator oneOf can generate some weird model classes; it doesn't even work for jax-rs currently https://github.com/OpenAPITools/openapi-generator/issues/5565 , so maybe avoid it...

Other possibilities:

jpraet commented 1 year ago

I prefer the approach of deprecating the current in/name/value properties in favor of the params array.

    InputValidationIssue:
      type: object
      allOf:
        - $ref: "#/components/schemas/Problem"
        # status property of Problem is usually not used for input validation issues
      properties:
        in:
          type: string
          enum:
            - body
            - header
            - path
            - query
          deprecated: true # deprecated for removal in 2.0, use params[0].in
        name:
          type: string
          deprecated: true # deprecated for removal in 2.0, use params[0].name
        value:
          # any type allowed
          deprecated: true # deprecated for removal in 2.0, use params[0].value
        params:
          type: array
          items:
            $ref: "#/components/schemas/ParameterReference"
    ParameterReference:
      type: object
      properties:
        in:
          type: string
          enum:
            - body
            - header
            - path
            - query
        name:
          type: string
        value: {} # any type allowed
pvdbosch commented 1 year ago

Discussion of concrete use cases is continued in #113, to analyze how such problems would be best represented.

If choosing to change the existing type, we'll have to consider impact. API Gateways often perform schema validation, currently creating a bad request problem (with InputValidationProblem structure) in logic that's applied to all APIs. To keep backwards compatibility, it may become necessary to make the problem structure configurable per API ("backwards-compatible mode" or "new structure")

It will also increase complexity of the "schemaViolation" issue type somewhat: the invalid parameter (there's always only one per issue) is put in an array, which is not guaranteed by the schema to be of length one.

jpraet commented 1 year ago

In this article https://medium.com/isa-group/inter-parameter-dependencies-in-rest-apis-4664e901c124 a study was done where they observed that 85% of REST API's have some kind of inter-parameter dependencies. They also identified 7 common usage patterns of such inter-parameter dependencies, which could be an interesting starting point for designing the standardized belgif common issue types in #113.

jpraet commented 1 year ago

I wonder how much business impact this suggested evolution of the InputValidationProblem type would really have in practice.

It seems like a trivial change when compared to earlier major changes in the problem responses:

Do we have any idea, for the institutions using the belgif REST guidelines, what amount of API's have already fully caught up with these earlier major changes? Only API's already using the new InputValidationProblem are impacted.

At CBSS we are caught up with these changes, but we only have 2 REST API's in production at the moment.

For eHealth, I don't find any references to the InputValidationProblem in the contracts published on https://portal.api.ehealth.fgov.be/?tag=swagger?

For SMALS, I only have personal experience with their Foleen API, and that one does not follow the REST guidelines. I believe SMALS's approach is also to not retroactively update existing API's to the latest REST guidelines?

JDMKSZ commented 1 year ago

Note that there's a long running issue in the OAS Spec github on this. So they're thinking about it too, I guess. Only, I think we can't wait for a solution from that side, as it will take another X years before it's actually addressed in the spec and implemented in the tooling.

pvdbosch commented 1 year ago

At CBSS we are caught up with these changes, but we only have 2 REST API's in production at the moment.

We were able to perform the changes, but had to discuss with each of the clients and coordinate with their updates. Took a bit of coordination effort, also because the changes had to be done both on API Gateway and the backend implementations.

For SMALS, I only have personal experience with their Foleen API, and that one does not follow the REST guidelines. I believe SMALS's approach is also to not retroactively update existing API's to the latest REST guidelines?

There are currently three problem formats supported by the SocSec API Gateway and each API deployed on the API Gateway has to be configured (in APIDEF) with one of these. The default config is the oldest format because of backwards compatibility. They have to be maintained as long as at least one API still uses them, so probably still for many years considering the number of REST APIs.

jpraet commented 1 year ago

Do you have any numbers on how many API's on the SocSec API Gateway already using the latest InputValidationProblem structure?

pvdbosch commented 1 year ago

Do you have any numbers on how many API's on the SocSec API Gateway already using the latest InputValidationProblem structure?

I don't have exact numbers, but there are about 15-20 new SocSec APIs since start of 2023.

pvdbosch commented 1 year ago

We can continue standardizing the issue types for these use cases, but there's no agreement to change the Problem structure

jpraet commented 1 year ago

With openapi-generator oneOf can generate some weird model classes; it doesn't even work for jax-rs currently OpenAPITools/openapi-generator#5565 , so maybe avoid it...

The approach B you described in https://github.com/belgif/rest-guide/issues/108#issuecomment-1470416903 (oneOf between a ParameterReference and a ParameterReference array on InputValidationIssue level) seems to work fine for me with jaxrs-spec generator in openapi-generator 6.6.0. For approach A (oneOf between InputValidationIssue and MultiParamInputValidationIssue in the InputValidationProblem issues array) it is indeed broken.

See sample project openapi-problem-sandbox.zip