Spectral doesn't seem to properly honor %YAML directive

[nulltoken]

Describe the bug
YAML 1.2 spec recommends using the Core schema unless instructed otherwise.

This Core Schema doesn't specify any timestamp data type. As such, an unquoted value such as 2012-10-12 should be resolved as a string.

Linting a openapi 3.0 spec described as a YAML 1.2 document makes the "valid-example" rule fail when the parsed implicit stringed example look like a date.

To Reproduce

1. Given this OpenAPI document

random.1.2.openapi.yaml

%YAML 1.2
---
openapi: 3.0.0
info:
  title: Random title
  description: Random description
  version: 0.0.1
  contact:
    email: [email protected]
paths: {}
servers:
  - url: https://www.random.com
components:
  schemas:
    RandomRequest:
      description: A random request
      type: object
      properties:
        implicit_string_date:
          description: Some YAML 1.2 implicit string.
          type: string
          example: 2012-10-12
        another_implicit_string_date:
          description: Some string.
          type: string
          example: x20121012
        explicit_string_date:
          description: Explicit string.
          type: string
          example: "2012-10-12"

2. Run this CLI command '....'

$ yarn spectral lint ./random.1.2.openapi.yaml

3. See error

Adding OpenAPI 3.x functions
OpenAPI 3.x detected

random.1.2.openapi.yaml
 19:30  warning  valid-example  "implicit_string_date" property type should be string

✖ 1 problem (0 errors, 1 warning, 0 infos)

Expected behavior
By default:

• Honoring the %YAML directive (when defined) and using the spec recommended schema to parse it
• Documenting the default YAML version used to parse a document when the document doesn't specify a %YAML directive

Bonus features:

• Displaying in the CLI output the YAML document format version (either specified in the document or fallen-back default)
• Allowing the user to specify through an argument the YAML version that should be used for documents without a %YAML directive

Environment (remove any that are not applicable):
@stoplight/spectral/4.0.2 win32-x64 node-v10.14.2

Additional context
You guys rock!

[nulltoken]

Another approach (or maybe a first step) might be to warn the user:

• Document that only YAML 1.1 is supported at this moment
• Fail the processing when a file with a YAML 1.2 directive is processed

[philsturgeon]

Double-checked OAS 3.0 and yep:

In order to preserve the ability to round-trip between YAML and JSON formats, YAML version 1.2 is RECOMMENDED along with some additional constraints:

We can get away with documenting that we only support YAML v1.1 and throw an error if directives are supported short term, but if we do this we need to add a new issue for supporting YAML 1.2 properly.

This is up to you @P0lip, but lets do one of these two things in this hardening sprint.

[P0lip]

@philsturgeon @nulltoken
I believe we should support YAML 1.2 by default - at least when it comes to scalar values.
To be frank with you all, I think that supporting more than one spec will be quite of burden, so I'd suggest sticking to YAML 1.2 instead of YAML 1.1 if 1.2 is the recommended format.

What do you think?
I can implement YAML 1.2 spec compliant scalar value handling this week.

[P0lip]

A working implementation can be found stoplightio/yaml#23
If we all agree on going with 1.2 we can just merge the above and bump yaml dependency in Spectral.

[philsturgeon]

We only need to support a single YAML version especially because I’m sure there’s nothing in 1.1 which won’t work in 1.2. Your approach looks good to me.

[nulltoken]

I’m sure there’s nothing in 1.1 which won’t work in 1.2.

FWIW, merge keys aren't officially part of the YAML 1.2 spec.

Would Spectral support them in YAML 1.2 documents would be a neat feature (Because of the ton of refactoring opportunities they offer) but should be documented in a clear and explicit manner as a freebie.

Otherwise you might have to deal with issues such as "My supposedly valid Spectral YAML 1.2 docs refused to be parsed by Yaml library X or Y".

[P0lip]

@nulltoken
Merge keys are already supported and enabled by default.
#415

[philsturgeon]

@nulltoken yeah so merge tokens were not officially part of 1.1 either I don't think, they seemed to be an extension. I think we are moving from supporting "1.1 with merge tokens" to "1.2 with merge tokens"

@P0lip could you add a note somewhere in docs about this as part of the fix, beyond bumping the stoplight/yaml library?

[P0lip]

@nulltoken yeah so merge tokens were not officially part of 1.1 either I don't think, they seemed to be an extension. I think we are moving from supporting "1.1 with merge tokens" to "1.2 with merge tokens"

Yeah, they are an extension .
As Phil said, we are going with "1.2 with merge tokens".

I'll leave a note on that somewhere in docs.

[nulltoken]

Merge keys are already supported and enabled by default.
#415

@P0lip I know! And I'm so thankful for that <3

I think we are moving from supporting "1.1 with merge tokens" to "1.2 with merge tokens"

@philsturgeon Awesome!