Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wildcard JSONPath example in spec text seems incorrect #88

Open
micovery opened this issue Oct 23, 2024 · 2 comments
Open

Wildcard JSONPath example in spec text seems incorrect #88

micovery opened this issue Oct 23, 2024 · 2 comments
Labels
bug Something isn't working
Milestone

Comments

@micovery
Copy link
Contributor

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[[email protected]=='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 ralfhandl added this to the Release 1.0 milestone Oct 23, 2024
@ralfhandl ralfhandl added the bug Something isn't working label Oct 23, 2024
ralfhandl added a commit to ralfhandl/Overlay-Specification that referenced this issue Oct 23, 2024
@ralfhandl
Copy link
Contributor

ralfhandl commented Oct 23, 2024

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
Copy link
Contributor

gregsdennis commented Oct 24, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants