TM-185013 - fix multilevel error response schema check

[jadebeer]

When the error response schema uses a oneOf to point to other schema objects the validator would fail because it expected a single object only. This now prevents that failure - most noticeably on the missing "properties" keyword

[jbend]

@jadebeer Does this relate back to a rule in the API Standards? I am trying to understand the why behind this.

[jadebeer]

@jadebeer Does this relate back to a rule in the API Standards? I am trying to understand the why behind this.

Hi @jbend well you guys tell me who wrote the rules 😊

I don't see anything in here: https://developer.trimble.com/docs/api-standard/ about that so I couldn't tell you who came up with the scripts but it is testing that the error schema has the keyword "properties" in it. And then from there it tests certain aspects of things that the properties should contain. This site may have been used: https://swagger.io/specification/ When I search for oneOf I find this as description of what a schema object such as an error response could hold and it is what TM-Cloudhub openapi does in this instance.

"oneOf": [{
          "properties": {
            "dataType": {"const": "string"},
            "data": {"$ref": "array_of_strings"}
          }
        }, {
          "properties": {
            "dataType": {"const": "number"},
            "data": {"$ref": "array_of_numbers"}
          }
        }]

As background as to what the issue is that I'm addressing: in TM-Cloudhub openapi for each endpoint we have the typical error response schemas defined for 400, 403, 500 etc as a "StandardErrorResponse". The schema definition for StandardErrorResponse contains the "properties" keyword, so that's all good. The exception to this is the 401 (unauthorized error) This has an error response schema definition that is an array of oneOf either the StandardErrorResponse OR the TrimbleCloudErrorResponse. Why 2? Because of the TTC layer that can return a response looking like this

"TrimbleCloudErrorResponse": {
        "type": "object",
        **"properties"**: {
          "fault": {
            "type": "object",
            "properties": {
              "status": {
                "type": "string"
              },
              "code": {
                "type": "string"
              },
              "message": {
                "type": "string"
              },
              "description": {
                "type": "string"
              }
            }
          }
        }
      }

  OR the StandardErrorResponse looking like this:

"StandardErrorResponse": {
        "type": "object",
        **"properties"**: {
          "type": {
            "type": "string"
          },
          "title": {
            "type": "string"
          },
          "status": {
            "type": "integer",
            "enum": [
              400,
              401,
              403,
              404,
              405,
              409,
              429,
              500
            ]
          },
          "errors": {
            "type": "array",
            "items": {
              "$ref": "#/components/schemas/Error"
            },
            "nullable": true
          }
        }
      }

TTC can create a 401 if unauthorized before it even hits TM-Cloudhub code or ART can return the StandardErrorResponse once Cloudhub forwards the request.

So this fix is to not dig into multi levels of objects if the schema definition is an array of oneOf but to return valid at this stage. It's a simplistic fix but a deep diving on nested object validation would be rather complex that for instance would have to account for possible recursive self referencing nested objects leading to an infinite loop while validating the error response schema. I wasn't comfortable making that kind of change on this commit. If the councils that be want to take that work on to enhance this functionality it would be best done under their own expertise.

[rhernandez2]

@jbend I was wondering if you had a chance to review @jadebeer explanation regarding the reasoning behind the change. Please let us know if you have any additional questions or would like to have a quick discussion to review?