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

Problem response for (non-schema) input validation #74

Closed
pvdbosch opened this issue Dec 21, 2020 · 6 comments · Fixed by #87
Closed

Problem response for (non-schema) input validation #74

pvdbosch opened this issue Dec 21, 2020 · 6 comments · Fixed by #87
Assignees
Milestone

Comments

@pvdbosch
Copy link
Contributor

pvdbosch commented Dec 21, 2020

The BadRequest problem type is now specific to input schema validation in the REST guide, i.e. used when "The consumer request is not conform to the Swagger specification of the API."

There is also a need to standardize on application-level input validation errors; e.g.:

  • input query parameter refers to an unknown or otherwise invalid resource (e.g. SSIN is unknown, replaced or canceled)
  • end date of a period is before begin date

proposal

  • Enlarge the scope of the badRequest problem to include any type of input validation error
  • add a new required property "issueType" to each invalidParam value containing an identifier of the validation error

Example:

        type:  https://www.gcloud.belgium.be/rest/problems/badRequest 
        title: Bad Request
        status: 400
        detail:  "The input message is incorrect"
        instance: urn:uuid:123456-1234-1235-4567489798
        invalidParams:
        - in: path
          name: enterpriseNumber
          reason: should be numeric
          value: abc
          issueType: schemaViolation
        - in: body
          name: parent[0].ssin
          value: "12345678901" 
          issueType: replacedSsin
          replacedBy: "23456789012" 
        - in: body
          name: parent[1].ssin
          value: "12345678901" 
          issueType: replacedSsin
          replacedBy: "23456789012" 
        - in: body
          name: period
          value:
             startDate: "2020-12-31" 
             endDate: "2020-01-01" 
          reason: endDate should be after startDate
          issueType: invalidPeriod
        - in: path
          name: ssin
          value: "12345678911" 
          reason: ssin is invalid
          issueType: invalidSsin

rationale

  • consistent handling of input problems by client; if client doesn't pre-validate his input in the same way the schema does
    • Can be confusing to have new validation errors across multiple calls after fixing first input errors, but no easy way around this
  • having cause of validation error visible in issueType is useful for client and for operational purposes
    • e.g. issueType: schemaViolation can still indicate message validation error against schema
  • "400 Bad Request" is http status code that should be used for app-level validation errors

Difference with resourceNotFound

When a resource id is not found, resourceNotFound problem should be used instead of BadRequestProblem.
This also means that the invalid parameters in a resourceNotFound problem should refer to path params only . We could explicitly document this difference.

E.g.

  • /countries/{countryId}/cities/{cityId} => resourceNotFound if countryId or cityId don't exist
  • /cities?countryId={countryId}&includeFormerCities={includeFormerCities} => BadRequest if countryId doesn't exist or includeFormerCities isn't a boolean value
@pvdbosch
Copy link
Contributor Author

pvdbosch commented Feb 4, 2021

Related: also found that "422 Unprocessable Entity" is sometimes used for OpenAPI validation errors:

422 is defined in the WebDAV spec as:

   The 422 (Unprocessable Entity) status code means the server
   understands the content type of the request entity (hence a
   415(Unsupported Media Type) status code is inappropriate), and the
   syntax of the request entity is correct (thus a 400 (Bad Request)
   status code is inappropriate) but was unable to process the contained
   instructions.  For example, this error condition may occur if an XML
   request body contains well-formed (i.e., syntactically correct), but
   semantically erroneous, XML instructions.

@pvdbosch pvdbosch added this to the in progress milestone Feb 23, 2021
@pvdbosch pvdbosch self-assigned this Feb 23, 2021
@pvdbosch
Copy link
Contributor Author

pvdbosch commented Apr 28, 2021

Flanders is using a nested problem structure to represent this case, see
https://test.data.vlaanderen.be/doc/richtlijnen/iv-rest-api-richtlijnen/#api_design (pt 9 Validatie error payload)

Because the same requirements for input validation errors exist as generic problems, this would be interesting.
Note that our guide already supports a nested problem for long running tasks.

The example from the issue description would then become:

type:  urn:problem-type:belgif:badRequest 
href: https://belgif.be/problems/badRequest
title: Bad Request
status: 400
detail:  "The input message is incorrect"
instance: urn:uuid:123456-1234-1235-4567489798
issues: #naming? currently invalidParams. Reuse as warnings in normal response?
- type: urn:problem-type:belgif:input-validation:schemaViolation #mandatory
  href: https://belgif.be/problems/schemaViolation #dereferencable
  title: "Input isn't valid with respect to schema" #mandatory?
  # no status property for these nested problems
  detail: enterpriseNumber abc should be numeric #recommended, if more detail can be provided
  # following additional properties are standardized but optional
  in: path
  name: enterpriseNumber
  value: abc
- type: urn:problem-type:cbss:input-validation:replacedSsin
  href: https://api.ksz-bcss.fgov.be/problems/replacedSsin
  title: "SSIN has been replaced. Use new ssin."
  detail: "SSIN 12345678901 has been replaced by 23456789012"
  in: body
  name: parent[0].ssin
  value: "12345678901" 
  replacedBy: "23456789012" 
- type: urn:problem-type:belgif:input-validation:invalidPeriod
  href: https://belgif.be/problems/invalidPeriod
  title: period is invalid
  detail: endDate should be after startDate
  in: body
  name: period
  value:
     startDate: "2020-12-31" 
     endDate: "2020-01-01" 
- type: urn:problem-type:cbss:input-validation:invalidSsin
  href: https://api.ksz-bcss.fgov.be/problems/invalidSsin
  title: "There is no person with the given SSIN"
  in: path
  name: ssin
  value: "12345678911" 

Open questions:

  • should schemaViolation issues be split from other validation issues?
    • they're often verified separately before other validations (e.g. by APIGW)
      • does this depend on provider implementation? E.g. use of APIGW that validates, use of Bean Validation in Java (w/o additional bean validation constraints). Some providers might execute most validation programmatically
      • schema validation is still not well supported in many languages and not implemented in the same way
      • other input validations might not be done in one go as well (e.g. first simple validations, then only existence of Ssin and EnterpriseNumber)
      • would clients want to treat schemaValidation issues in the same way as other input validation issues?
  • should we define a part of the URN to distinguish these input validation problems, as hint that they shouldn't be used as top level problems e.g. the "input-validation" infix above?
  • define an "issues" type that can also be used as warnings in a 200 response, using same problem types ?
  • to discuss: difference between 404 ResourceNotFound (see issue description). Is distinction with 409 Conflict (conflict with current resource state) clear enough?

@pvdbosch
Copy link
Contributor Author

pvdbosch commented Oct 20, 2021

REST WG decided to:

  • use single badRequest problem type for all input validation issues, problem format like in previous comment
  • recommend use of input-validation infix as part of the URN of nested types
  • use of issues list for use case warnings in a 200 response isn't standardized, requires further real world examples. Warnings may be broader than input validation.
  • difference with 404 ResourceNotFound will be clarified in the REST guide

@pvdbosch
Copy link
Contributor Author

pvdbosch commented Jan 11, 2022

Added InputValidationProblem and InputValidationIssue in PRs to openapi-problem (belgif/openapi-problem#4 - OpenAPI v3 only, I'll add Swagger once reviewed) and in the guide (#87)

Notes:

  • deprecated InvalidParamProblem and InvalidParam; to avoid creating a v2 (which would cascade to openapi-common)
  • schema object names are InputValidationProblem/InputValidationIssue, nested problems have infix "input-validation" but the problem type is "urn:problem-type:belgif:badRequest". Should we rename the problem type to "urn:problem-type:belgif:inputValidation"?
  • examples of 404 of problem type resourceNotFound follow the now-deprecated InvalidParamProblem type. Should this be modified to use InputValidationProblem or get its own type with only a single parameter referenced (normally only a single path param in this case)

@pvdbosch
Copy link
Contributor Author

pvdbosch commented Jan 12, 2022

conclusions REST WG January 2022:

  • to specify invalid parameter name in case of body param, it can be difficult for implementations to construct JSON Path (like in example). This isn't currently a requirement in the guide however; so we can leave it open to the API provider.
  • use same InputValidationProblem in case of 404 resource not found; nested problem type is left up to API provider. The problem types are left as is (badRequestProblem and resourceNotFoundProblem URNs)

@pvdbosch
Copy link
Contributor Author

PRs have been updated and are ready for review:

@pvdbosch pvdbosch linked a pull request Mar 15, 2022 that will close this issue
pvdbosch added a commit to belgif/openapi-problem that referenced this issue Mar 30, 2022
* add type for input validation problem (belgif/rest-guide#74),
* replace and deprecate InvalidParam/InvalidParamProblem
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

Successfully merging a pull request may close this issue.

1 participant