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

bugfix/missing quotes templates #143

Merged
merged 6 commits into from
Dec 16, 2021
Merged

Conversation

baywet
Copy link
Member

@baywet baywet commented Nov 23, 2021

@baywet baywet self-assigned this Nov 23, 2021
@baywet baywet added this to the 1.0.10 milestone Nov 23, 2021
@baywet baywet enabled auto-merge November 23, 2021 19:56
@baywet baywet disabled auto-merge November 23, 2021 19:57
@baywet baywet force-pushed the bugfix/missing-quotes-templates branch from af03c51 to f0cf61b Compare November 23, 2021 20:01
@baywet baywet enabled auto-merge November 23, 2021 20:01
@baywet baywet disabled auto-merge November 23, 2021 21:58
@xuzhg
Copy link
Contributor

xuzhg commented Nov 24, 2021

MyFunction(param={param}) can match

  • ~ MyFunction(param='abc') // if the type of param is string
  • ~ MyFunction(param=true) // if the type of param is boolean
  • ~ MyFunction(param=8) // if the type of param is int32
  • ...

MyFunction(param='{param}') can matches ?

  • ~ MyFunction(param=''abc'') // if the type of param is string
  • ~ MyFunction(param='true') // if the type of param is boolean
  • ~ MyFunction(param='8') // if the type of param is int32

@baywet baywet force-pushed the bugfix/missing-quotes-templates branch from f0cf61b to faa8df1 Compare November 24, 2021 17:54
@baywet baywet requested review from zengin and xuzhg November 24, 2021 17:54
@baywet baywet enabled auto-merge (squash) November 24, 2021 18:00
zengin
zengin previously approved these changes Nov 24, 2021
Signed-off-by: Vincent Biret <[email protected]>
@baywet baywet requested a review from xuzhg December 2, 2021 17:25
@darrelmiller
Copy link
Member

The OpenAPI that we need to generate should look like this,

openapi: 3.0.0
info:
  title: example of string path parameter that must be quoted
  version: 1.0.0
paths:
  get:
    /MyFunction(param='{funcParam}'):
      parameters:
        - name: funcParam
          in: path
          required: true
          schema:
             type: string
      responses: 
        200:
          description: OK

JSON Schema has no notion of a quoted string, so it would be difficult to describe a parameter value that needs to be surrounded by quotes before inserting into the URL. The fact that quotes, single vs double are required are an OData requirement and therefore should be encoded in URL, just like all other OData conventions.

You can see in the simple string expansion examples in the URI Template specification that quotes are not inserted for string values.

     Example Template     Expansion

       {var}              value
       {hello}            Hello%20World%21
       {half}             50%25
       O{empty}X          OX
       O{undef}X          OX
       {x,y}              1024,768
       {x,hello,y}        1024,Hello%20World%21,768
       ?{x,empty}         ?1024,
       ?{x,undef}         ?1024
       ?{undef,y}         ?768
       {var:3}            val
       {var:30}           value
       {list}             red,green,blue
       {list*}            red,green,blue
       {keys}             semi,%3B,dot,.,comma,%2C
       {keys*}            semi=%3B,dot=.,comma=%2C

https://datatracker.ietf.org/doc/html/rfc6570#section-3.2.2

@baywet
Copy link
Member Author

baywet commented Dec 13, 2021

collecting notes from the meeting we've just had:

  • clients rely on this conversion library to encapsulate away OData conventions and therefore have no knowledge of encoding requirements (quotes)
  • the information (quotes) cannot be carried through JSON schema/OpenAPI structures
  • this should work in 99.999% of the times, except maybe for Edm.Primitive types which are not allowed in url templates
  • the tradeoff of this solution is making the template a little more static in nature
  • an alternative could be considered to use the format property, and use an open formatter (odatavalue, odatastring...) but this brings other concerns like collisions with numbers which already use the format property.

Overall the group agreed to proceed with this change.

@xuzhg for final review and merge.

@xuzhg
Copy link
Contributor

xuzhg commented Dec 15, 2021

:shipit:

@baywet baywet disabled auto-merge December 16, 2021 12:30
@baywet baywet merged commit b339855 into master Dec 16, 2021
@baywet baywet deleted the bugfix/missing-quotes-templates branch December 16, 2021 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

URL templates missing quotes around enum types
4 participants