oval icon indicating copy to clipboard operation
oval copied to clipboard

Support multiple locations or wildcard?

Open josephzidell opened this issue 2 years ago • 6 comments

The format of the command $ oval validate LOCATION means oval can validate one file at a time. In many modern applications, there can be several, if many, swagger files. Can we support multiple paths or a pattern?

Example of multiple paths:

$ oval validate LOCATION1 LOCATION2 LOCATION3 ...

Example of using a pattern:

$ oval validate **/swagger.json

In either case, the returned warnings or errors would indicate the specific file path the warning/error occurred in.

I'd b happy to take a crack at a PR for this.

josephzidell avatar Oct 10 '21 03:10 josephzidell

This is a cool idea. There are a few things that need to be answered before I knock this out.

  1. Are you cool with glob matching?
  2. Which option would you like for the standard output (detailed below)?
  3. Which option would you like for the JSON output (detailed below)?

Standard Output

Option 1

We will take single-file mode and update the header to include the problematic file names. Example below:

OpenAPI Specification document (path/to/file/1.json) is not valid!

ERRORS
  #/paths/~1pets~1{id2}: Equivalent path already exists: /pets/{id2}

WARNINGS
  #/definitions/Unused: Definition is not used: #/definitions/Unused

OpenAPI Specification document (path/to/file/2.json) is not valid!

ERRORS
  #/paths/~1pets~1{id2}: Equivalent path already exists: /pets/{id2}

WARNINGS
  #/definitions/Unused: Definition is not used: #/definitions/Unused

Option 2

We will collect all errors and prepend the JSON Pointer for each error/warning with the filename. Example below

OpenAPI Specification documents are not valid!

ERRORS
  path/to/file/1.json#/paths/~1pets~1{id2}: Equivalent path already exists: /pets/{id2}
  path/to/file/2.json#/paths/~1pets~1{id2}: Equivalent path already exists: /pets/{id2}

WARNINGS
  path/to/file/1.json#/definitions/Unused: Definition is not used: #/definitions/Unused
  path/to/file/2.json#/definitions/Unused: Definition is not used: #/definitions/Unused

JSON Output

Option 1

We will add a files property to the root of the output, which is an array of the single-file output but with the addition of the location property that points to the file location. Example below:

{
  "files": [
    {
      "location": "path/to/file/1.json",
      "message": "OpenAPI Specification document is not valid!",
      "errors": [
        {
          "code": "EQUIVALENT_PATH",
          "message": "Equivalent path already exists: /pets/{id2}",
          "path": [
            "paths",
            "/pets/{id2}"
          ]
        }
      ],
      "warnings": []
    },
    {
      "location": "path/to/file/2.json",
      "message": "OpenAPI Specification document is not valid!",
      "errors": [
        {
          "code": "EQUIVALENT_PATH",
          "message": "Equivalent path already exists: /pets/{id2}",
          "path": [
            "paths",
            "/pets/{id2}"
          ]
        }
      ],
      "warnings": []
    }
  ]
}

Option 2

We will add a files property to the root of the output, and each property of files is the path to the file location and its value is the current single-file output. Example below:

{
  "files": {
    "path/to/file/1.json": {
      "message": "OpenAPI Specification document is not valid!",
      "errors": [
        {
          "code": "EQUIVALENT_PATH",
          "message": "Equivalent path already exists: /pets/{id2}",
          "path": [
            "paths",
            "/pets/{id2}"
          ]
        }
      ],
      "warnings": []
    },
    "path/to/file/2.json": {
      "message": "OpenAPI Specification document is not valid!",
      "errors": [
        {
          "code": "EQUIVALENT_PATH",
          "message": "Equivalent path already exists: /pets/{id2}",
          "path": [
            "paths",
            "/pets/{id2}"
          ]
        }
      ],
      "warnings": []
    }
  ]
}

Option 3

We collect all errors/warnings and each error/warning has a location property that points to the file location. Example below:

{
  "message": "OpenAPI Specification documents are not valid!",
  "errors": [
    {
      "code": "EQUIVALENT_PATH",
      "message": "Equivalent path already exists: /pets/{id2}",
      "location": "path/to/file/1.json",
      "path": [
        "paths",
        "/pets/{id2}"
      ]
    },
    {
      "code": "EQUIVALENT_PATH",
      "message": "Equivalent path already exists: /pets/{id2}",
      "location": "path/to/file/2.json",
      "path": [
        "paths",
        "/pets/{id2}"
      ]
    }
  ],
  "warnings": []
}

Option 4

We use an array to output per-file output, with each array entry being as it is now but with the addition of the location property. Example below:

[
  {
    "message": "OpenAPI Specification documents are not valid!",
    "location": "path/to/file/1.json",
    "errors": [
      {
        "code": "EQUIVALENT_PATH",
        "message": "Equivalent path already exists: /pets/{id2}",
        "path": [
          "paths",
          "/pets/{id2}"
        ]
      },
    "warnings": []
  },
  {
    "message": "OpenAPI Specification documents are not valid!",
    "location": "path/to/file/2.json",
    "errors": [
      {
        "code": "EQUIVALENT_PATH",
        "message": "Equivalent path already exists: /pets/{id2}",
        "path": [
          "paths",
          "/pets/{id2}"
        ]
      }
    ],
    "warnings": []
  }
]

I can't think of any other things to firm up right now but if we can get these answered, I can knock out a PR for your review.

whitlockjc avatar Oct 13 '21 00:10 whitlockjc

Thanks @whitlockjc

For glob matching, that's perfectly fine.

For standard output, how about something like option 1 with indentation:

OpenAPI Specification document (path/to/file/1.json) is not valid!

	ERRORS
	#/paths/~1pets~1{id2}: Equivalent path already exists: /pets/{id2}

	WARNINGS
	#/definitions/Unused: Definition is not used: #/definitions/Unused

OpenAPI Specification document (path/to/file/2.json) is not valid!

	ERRORS
	#/paths/~1pets~1{id2}: Equivalent path already exists: /pets/{id2}

	WARNINGS
	#/definitions/Unused: Definition is not used: #/definitions/Unused

For json output, it feels like option 3 allows the greatest interopability with other software that may read the json output. Since everything is on the same level, it's easy to for example, see the total number of issues.

josephzidell avatar Oct 19 '21 03:10 josephzidell

@josephzidell With option 3 for JSON output, does it concern you that you'd need to reduce() the JSON output to get per-file accumulations of the validation errors/warnings?

whitlockjc avatar Oct 19 '21 16:10 whitlockjc

@whitlockjc Not at all. This will used in one of two ways:

  1. Human consumption, in which case the user will fix each infraction in turn, and organizing by file is irrelevant
  2. Computer processing, which will almost always have access to a reduce function (available in most environments)

josephzidell avatar Oct 28 '21 16:10 josephzidell

The more I think about it, I do like the file-based collection of errors and warnings. If the input is a collection of files, the output should be as a collection of files. And since your ideal output for Standard output is file-based as well, it would create a consistency. If I had to implement anything right now, it would be Option 2. I could see a simpler output where files was removed and the top-level properties of the JSON output would be the the input file paths, whose values would be the per-file errors/warnings.

Unless you completely hate this, I can knock it out pretty quickly. Sorry for the delay.

whitlockjc avatar Dec 01 '21 19:12 whitlockjc

@whitlockjc That sounds reasonable and user-friendly. Thanks for jumping onto this. 🙏

josephzidell avatar Dec 01 '21 22:12 josephzidell