stoplightio / spectral

A flexible JSON/YAML linter for creating automated style guides, with baked in support for OpenAPI (v3.1, v3.0, and v2.0), Arazzo v1.0, as well as AsyncAPI v2.x.
https://stoplight.io/spectral
Apache License 2.0
2.51k stars 239 forks source link

YAML Merge Key not implemented to Spec #2054

Open tarquin-the-brave opened 2 years ago

tarquin-the-brave commented 2 years ago

Describe the bug

The merge key spec states:

If the value associated with the key is a single mapping node, each of its key/value pairs is inserted into the current mapping, unless the key already exists in it.

The merge key implementation in Spectral will always include key/value pairs even if the key already exists.

As later repeated keys override the earlier key in the mapping we get a situation where if you include the merge key at the beginning of a mapping, later key/value pairs in the mapping can override keys that are imported from the anchor. Whereas if the merge key is at the end of the mapping, it imports key/value pairs that override key that already exist in the mapping.

To Reproduce

Given this ruleset, ruleset.yaml:

rules:
  my-rule:
    given: $.bar
    severity: error
    then:
      field: baz
      function: length
      functionOptions:
        max: 3

(single rule that says "bar.baz must have a length less than or equal to 3").

And this document to lint, test.yaml:

foo: &foo
  baz: two

bar:
  <<: *foo
  baz: three

we see:

$ spectral lint --ruleset ruleset.yaml test.yaml

/src/test.yaml
 6:8  error  my-rule  "baz" property must be shorter than 3  bar.baz

✖ 1 problem (1 error, 0 warnings, 0 infos, 0 hints)

$ echo $?
1

However if we change that document to, test2.yaml:

foo: &foo
  baz: two

bar:
  baz: three
  <<: *foo

we see the error disappear:

$ spectral lint --ruleset ruleset.yaml test2.yaml
No results with a severity of 'error' or higher found!

$ echo $?
0

There is an issue of interpretation with "already exists in the mapping". I can see this meaning "exists as an earlier key" or "exists anywhere in the mapping when the merge key syntax is resolved" - this is in reality elided by the fact that later repeated keys override their previous counter parts so with the former interpretation, keys appearing after the merge key will override any instance of the same key being imported from the anchor to get the same result.

But by either interpretation of "already exists", Spectral is not getting this right. Here the baz key already exists, and yet it is being overwritten by the key in the *foo anchor that the merge key is importing.

Expected behaviour

The expected behaviour would be to not overwrite existing keys in a mapping with keys imported from merging in the mapping referenced in the merge key.

I.e: both test.yaml and test2.yaml above should resolve to:

foo:
  baz: two
bar:
  baz: three

and thus would both fail linting by the ruleset.yaml above.

Environment:

Additional context

While this is a minimal example, this issue makes the use of merge keys in much larger YAML documents tricky, such as an Open API Spec. Those used to using merge keys now how to remember to put them at the top of mappings to get the desired parsing from Spectral. If they don't Spectral can give errors that don't appear to relate to the data. If they checked their merge key and anchor usage by using a tool like yq to parse their document, the rendered form they see won't include the values which the Spectral lint is erroring on, i.e. values wrongly merged in from anchors.

P0lip commented 2 years ago

Yup, you're right. This is indeed not implemented correctly. This will need to be fixed in @stoplight/yaml, but we can keep the Spectral issue open for the tracking purposes.

raleigh04 commented 2 years ago

@mnaumanali94 tag!