OAI / Overlay-Specification

The OAI Overlay Specification
Apache License 2.0
57 stars 15 forks source link

Wildcard JSONPath example in spec text seems incorrect #88

Open micovery opened 1 month ago

micovery commented 1 month ago

I was trying to implement the spec as part of Apigee's apigee-go-gen tool, and ran into this issue while creating some test cases.

The example JSON Path expression from this example: https://github.com/OAI/Overlay-Specification/blob/main/versions/1.0.0.md?plain=1#L197 could not be parsed successfully by library at from: https://github.com/vmware-labs/yaml-jsonpath

$.paths.*.get.parameters[?@.name=='filter' && @.in=='query']

I had to modify it by adding grouping parenthesis, and then it worked:

$.paths.*.get.parameters[?(@.name=='filter' && @.in=='query)']

I also tried with various other online tools and it seems like they all need parenthesis. I am not a JSONPath expert, so I am opening this issue. Please clarify or fix the example.

As a separate nitpick ... if I understand correctly, the example is attempting to change the schema property to use a $ref instead of it being inlined. However, that's not what's it's actually doing, it's adding the $ref field to the schema object. So you end up with a schema object that is both in-lined, and also $ref. Should not the example first remove the schema object, and then update the operation with the schema using $ref, ... e.g.

overlay: 1.0.0
info:
  title: Update many objects at once
  version: 1.0.0
actions:
  - target: $.paths.*.get
    update:
      x-safe: true
  - target: $.components.schemas
    update:
      filterSchema:
        type: string
        default: available
        enum:
          - available
          - pending
          - sold
  - target: $.paths.*.get.parameters[?(@.name=='filter' && @.in=='query')].schema
    remove: true
  - target: $.paths.*.get.parameters[?(@.name=='filter' && @.in=='query')]
    update:
      schema:
        $ref: '#/components/schemas/filterSchema'
ralfhandl commented 1 month ago

Hello @micovery,

Thanks for reporting this!

According to RFC9535 no parentheses are needed, and Table 12 shows several examples using AND conditions without parentheses.

Seems the parentheses are a tool issue and not a specification bug.

I ran the example with both Bump and SpeakEasy, and both tools complained without the parentheses 😞

Bump produced the result you describe above with $ref next to existing schema keywords, and your overlay produced the expected result.

SpeakEasy only applied the x-safe update and completely ignored the schema update 😮

So we need to update the example and add a remove action before the $ref update action.

This can only be done in the next patch version 1.0.1 of the specification. The infrastructure for the next release(s) hasn't been set up yet, so a corresponding pull request will have to wait a while.

If you find more bugs, please don't hesitate to create issues.

Thanks in advance!

gregsdennis commented 4 weeks ago

It should be noted that adoption on RFC9535 is expected to be rather slow since many tools support syntaxes which aren't in the spec.

Hopefully that spec's inclusion in this one drives that support.

Also, my tool at https://json-everything.net/json-path was developed alongside the spec, so it supports all of the new syntax.