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

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

Open
jpraet opened this issue Feb 6, 2023 · 14 comments
Open

Comments

@jpraet
Copy link
Contributor

jpraet commented Feb 6, 2023

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
Copy link
Contributor

pvdbosch commented Feb 6, 2023

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.

@pvdbosch pvdbosch transferred this issue from belgif/openapi-problem Feb 14, 2023
@jpraet
Copy link
Contributor Author

jpraet commented Mar 10, 2023

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
Copy link
Contributor

discussed on WG, but no conclusion yet.

How frequent do this type of input validation issues occur?

  • can we list examples of APIs that mandate a group of query params, or have mutually exclusive query params?
  • examples of URI and body parameters that should be equal:
    • every PUT "full update" operation that allows identifier in the payload
    • other examples?

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

Possible solutions for standard solution:

  • add a oneOf between multi param and single param validation issue
  • set one of the parameters in the InputValidationIssue structure, and add other parameter as additional property
    • could make sense for the PUT "full update" case to consider the identifier in the JSON payload as "wrong"

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
Copy link
Contributor

pvdbosch commented Apr 3, 2023

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

Other possibilities:

@jpraet
Copy link
Contributor Author

jpraet commented Apr 5, 2023

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
Copy link
Contributor

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
Copy link
Contributor Author

jpraet commented May 10, 2023

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
Copy link
Contributor Author

jpraet commented May 11, 2023

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:

  • change of problem "type" identifiers from http link to urn syntax (2021-06-24)
  • replacing InvalidParamProblem by InputValidationProblem (2022-04-30)

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
Copy link
Contributor

JDMKSZ commented May 12, 2023

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
Copy link
Contributor

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
Copy link
Contributor Author

jpraet commented May 12, 2023

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

@pvdbosch
Copy link
Contributor

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
Copy link
Contributor

  • having a textual "detail" description is probably sufficient for most users (i.e. they don't use the parameter references)
    • concern that breaking change isn't worthwhile
  • conceptually, multiple parameters should be referenced for these type of validation issues

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

@jpraet
Copy link
Contributor Author

jpraet commented Jun 22, 2023

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 #108 (comment) (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

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

No branches or pull requests

3 participants