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

Specification Path expressions for operations with no OperationI ID #144

Merged
merged 6 commits into from
Feb 15, 2024

Conversation

ndenny
Copy link
Collaborator

@ndenny ndenny commented Feb 7, 2024

As discussed in #130 - a slight modification on @handrews suggestion - I'm using the # to separate the $sourceDescriptions.<name> from the JSON Pointer. This disambiguates it from the operationId syntax that uses $sourceDescriptions.<name>.<operationId>

Note sure if I needed to create a completely separate ABNF - let me know.

@handrews
Copy link
Member

handrews commented Feb 7, 2024

@ndenny I see what you mean about the syntactic ambiguity, but if you're going to use # then you should use the URI fragment-encoded syntax of JSON Pointers. Because if it looks like a URI fragment, it should parse like a URI fragment.

I'm also not sure that the # really disambiguates it. The OAS does not define syntax for operationId, it is just RECOMMENDED to follow "common programming language naming conventions". That doesn't forbid /, but it doesn't forbid # either. Unless people have been starting operationIds with / then it should be just as distinctive. Although you do have to remember that the empty string is a valid JSON pointer, and technically also not forbidden as an operationId...

I see you have the # followed by plain-text JSON Pointer elsewhere as well. I'm concerned that's going to cause very obscure bugs when someone assumes they should URI-encode the pointer, as the result is also a valid JSON Pointer... just not the same JSON Pointer if you interpret it as plain text. That error can be hard to spot and debug.

If you can't do something with the runtime expression syntax to disambiguate it, I think it either needs to be URI-encoded or you need a different delimiter.

@ndenny
Copy link
Collaborator Author

ndenny commented Feb 8, 2024

@handrews - perhaps it'd be cleaner if we had a pair of properties:
operationSourceDescription and operationPointer so we don't have to 'encode' what we mean into one thing

@handrews
Copy link
Member

handrews commented Feb 8, 2024

@ndenny yeah... same applies to requestBody, I think. Either way, if we're not uri-encoding we shouldn't use # as it just looks like a URI-fragment and folks will expect it to use that encoding.

@frankkilcommins
Copy link
Collaborator

sourceDescriptions is already covered by the ABNF syntax specified for runtime expressions. No need to create another section.

I don't have a strong opinion on the property naming. The obvious choices are operationRef (understanding it deviates from OAS), operationReference, or operationPath (as suggested by @handrews )

How I envisaged the URI reference to be created was to leverage the runtime expression to specify the base and then to use JSON Pointer to resolve against the appropriate path operation.

Given:

sourceDescriptions:
- name: petStoreDescription
  url: https://github.com/swagger-api/swagger-petstore/blob/master/src/main/resources/openapi.yaml
  type: openapi

The we could reference an operation in the file via:
'{$sourceDescriptions.petStoreDescription.url}#/paths/~1pet~1findByStatus/get'

Copy link
Collaborator

@frankkilcommins frankkilcommins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment #144 (comment)

@handrews
Copy link
Member

handrews commented Feb 10, 2024

[EDIT: See next comment, this is a problem in OAS 3.x, so Workflows is better off matching it than trying to fix it.]

How I envisaged the URI reference to be created was to leverage the runtime expression to specify the base and then to use JSON Pointer to resolve against the appropriate path operation.

My concern about the # usage is that given this JSON:

{
  "cat": {
    "Schrödinger": "maybe"
  }
}

We should either see "/cat/Schrödinger" (plain string) or "#/cat/Schr%C3%B6dinger" (urlencoded). We should not mix the URI-fragment separator # with the plain string JSON Pointer text. Anything that looks like a fragment should parse like a fragment.

But I'd argue that in runtime expressions, it's better to use a non-fragment syntax. Particularly for things like requestBody that might get applied to a plain application/json payload. Technically you can only evaluate a fragment based on the rules of the media type, and application/json specifically does not define JSON Pointer as a fragment syntax (and the JSON Pointer RFC specifically mentions that it is not the fragment syntax for application/json). So it's actually more correct to define runtime expression-specific rules using plain text JSON Pointers rather than URI fragments.

@handrews
Copy link
Member

.... and now I'm realizing that # + JSON Pointer (string form) is already in the runtime expression syntax of OAS 3.x :-/ So I guess we can file that under "things Workflows is not expected to fix." We can fix it with Moonwalk and I guess Workflows 2. Workflows 1 should match OAS 3 where possible.

@frankkilcommins frankkilcommins added the implementor-draft In scope for first version label Feb 13, 2024
@frankkilcommins
Copy link
Collaborator

We've decided on operationPath. @ndenny to remove the redundant ABNF section that was added

@ndenny
Copy link
Collaborator Author

ndenny commented Feb 14, 2024

Question - if we're going for the expected syntax to be '{$sourceDescriptions.petStoreDescription.url}#/paths/~1pet~1findByStatus/get' ? Do we need to then explain the curly-brackets syntax?

@frankkilcommins frankkilcommins merged commit 9164da1 into main Feb 15, 2024
2 checks passed
@frankkilcommins frankkilcommins deleted the operation-path-instead-of-ref branch January 20, 2025 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
implementor-draft In scope for first version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants