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

Swagger: GET record from table with uuid primary key: failed to parse filter #1970

Open
artsra opened this issue Sep 30, 2021 · 16 comments
Open
Labels
enhancement a feature, ready for implementation http http compliance OpenAPI

Comments

@artsra
Copy link

artsra commented Sep 30, 2021

Environment

  • PostgreSQL version: postgres:13
  • PostgREST version: postgrest/postgrest
  • Swagger image: swaggerapi/swagger-ui
  • Operating system: MacOS Big Sur 11.6

(Using this: https://github.com/johnnylambada/docker-postgrest-swagger-sample)

Description of issue

On swagger I try to GET a record from a table where the primary key is a UUID.
As a result I get:

{
  "details": "unexpected \"3\" expecting \"not\" or operator (eq, gt, ...)",
  "message": "\"failed to parse filter (3922488c-18bc-45d8-baa2-bdb10cd90e37)\" (line 1, column 1)"
}

I think this error originates from postgrest.

On the other hand if I try a filter like 'eq.3922488c-18bc-45d8-baa2-bdb10cd90e37' the action is refused by swagger, because the string with filter does not follow the uuid format.

How do I resolve this?

@artsra artsra changed the title On swagger with uuid id: failed to parse filter On swagger GET a record from a table with uuid primary key: failed to parse filter Sep 30, 2021
@artsra artsra changed the title On swagger GET a record from a table with uuid primary key: failed to parse filter Swagger: GET a record from table with uuid primary key: failed to parse filter Sep 30, 2021
@artsra artsra changed the title Swagger: GET a record from table with uuid primary key: failed to parse filter Swagger: GET record from table with uuid primary key: failed to parse filter Sep 30, 2021
@wolfgangwalther
Copy link
Member

This will not work right now. We had some ideas in #1949 (comment) and #1949 (comment) that would make this work - but those are far from being implemented, yet.

@steve-chavez
Copy link
Member

I see it now. So as Wolfgang mentioned, our current syntax is a problem for swagger-ui. Because it's like:

id=eq.2e1183bf-123b-4a48-982f-d9c78584ce08

The value being eq.2e1183bf-123b-4a48-982f-d9c78584ce08, which is an invalid uuid. If we were to switch our operators to the left side:

id.eq=2e1183bf-123b-4a48-982f-d9c78584ce08

Then the value would be valid.

Function calls should work fine with swagger-uid though, because they use the required form of arg=val.


Seems that or(or=(age.gte.14,age.lte.18)) will always be a problem, even if we switched the syntax. Complex boolean logic for swagger-ui would have to be done via functions.

@steve-chavez
Copy link
Member

steve-chavez commented Dec 2, 2021

Seems that or(or=(age.gte.14,age.lte.18)) will always be a problem, even if we switched the syntax. Complex boolean logic for swagger-ui would have to be done via functions.

I think the above problem with or could be fixed as well.

We could change this:

GET /people?or=(grade.gte.90,student.is.true)

To:

GET /people?grade.gte=90&student.is=true&or=grade.gte,student.is

Or as a shorthand:

GET /people?grade.gte=90&student.is=true&or=*

This works because on the new syntax we can make the filter at the left side unique, unlike the current syntax where ?grade=.. can have multiple filters.

Besides swagger-ui, other advantage I see is that with this new or client libraries wouldn't need to escape or quote values inside or. It seems less error prone for manual requests as well.

Grouping could still be done as:

GET /people?age.gte=11&age.lte=17&age.eq=14&or=age.eq,not.and(age.gte,age.lte)

@wolfgangwalther
Copy link
Member

My initial reaction to this was: Oh noo. Separating logic from the filters will just create additional complexity. But the more I think about it, the more I see the upside of this, too.

This works because on the new syntax we can make the filter at the left side unique, unlike the current syntax where ?grade=.. can have multiple filters.

Do we already support multiple filters with the same name before the =? I wasn't sure in #2063 (comment).

If that's the case, I wouldn't want to give up on that. Otherwise a age.eq=15&age.eq=17&or=* would not be possible.

Maybe we can add aliasing to it? So basically make it unique, but allow this:

GET /people?age15:age.eq=15&age17:age.eq=17&or=age15,age17

Besides swagger-ui, other advantage I see is that with this new or client libraries wouldn't need to escape or quote values inside or. It seems less error prone for manual requests as well.

❤️

Grouping could still be done as:

 GET /people?age.gte=11&age.lte=17&age.eq=14&or=age.eq,not.and(age.gte,age.lte)

Instead of making the top-level an or, how about logic? Otherwise, we'd need to support things like or.not= etc. - it seems simpler to just parse the value of logic=... as our logic tree - done.

GET /people?age.gte=11&age.lte=17&age.eq=14&logic=or(age.eq,not.and(age.gte,age.lte))

or with aliases:

GET /people?min:age.gte=11&max:age.lte=17&age.eq=14&logic=or(age.eq,not.and(min,max))

@steve-chavez
Copy link
Member

steve-chavez commented Dec 3, 2021

Do we already support multiple filters with the same name before the =?

Just tried that, we do :/ - it's undocumented, would like to keep it that way.

Otherwise a age.eq=15&age.eq=17&or=* would not be possible.

That could be changed to an in or eq.any though, right?

or with aliases:

IMHO it seems too complex, and we'd only need for that for a URL workaround(no sense in aliasing filters in SQL). For a REST syntax v2, I think it'd be better to just make each query param unique. That way the syntax would be kept simpler.

@steve-chavez
Copy link
Member

steve-chavez commented Dec 3, 2021

I think it'd be better to just make each query param unique

Though as I see it on #2063 (comment), or is already used on duplicate (id.neq, name.neq) query params.

Hm, I think fixing that for a v2 would be a matter of adding the any variant to all our operators and guide the users with docs. Doesn't seem that bad, users now have to consult our docs for grasping complex boolean logic anyway.

@steve-chavez
Copy link
Member

Instead of making the top-level an or, how about logic?

One problem is that logic seems like a common keyword that could be used as a column name. boolean_logic seems too long and boolean seems like a type. or/and are safer in that regard.

@steve-chavez
Copy link
Member

steve-chavez commented Dec 11, 2021

For a REST syntax v2, I think it'd be better to just make each query param unique

I gave it a second thought and making each param unique seems too breaking, many users are already used to doing or with the same filter.

Maybe we can add aliasing to it? So basically make it unique, but allow this:
GET /people?age15:age.eq=15&age17:age.eq=17&or=age15,age17

Yes, aliasing looks like the way forward. We could still offer the any alternatives and suggest in the docs that the URL can be kept simpler if they're used.

GET /people?min:age.gte=11&max:age.lte=17&age.eq=14&logic=or(age.eq,not.and(min,max))

Thinking about the logic operator, I think we could add it without needing a breaking change if we do the filter aliases right?

So currently it could be like:

GET /people?min:age=gte.11&max:age=lte.17&age=eq.14&logic=or(age.eq,not.and(min,max))

@steve-chavez
Copy link
Member

GET /people?age15:age.eq=15&age17:age.eq=17&or=age15,age17

Another thing, if no aliasing is added like above, then the or could be applied to both filters.

GET /people?age.eq=15&age.eq=17&or=age.eq

Which would also help to keep the url shorter, in cases where or is wanted for all filters.

@wolfgangwalther
Copy link
Member

Thinking about the logic operator, I think we could add it without needing a breaking change if we do the filter aliases right?

Adding the logic parameter would always be a breaking change, because logic= was not a reserved keyword for function calls earlier - so somebody could use that as an argument name.

Using or=(...) could be a non-breaking change. Basically, whenever we encounter a <col>.<op> part without value, interpret it as a reference to a filter on the highest level. And if there are multiple, expand them to a list, so all of those are the same:

  • GET /people?age.eq=15&age.eq=17&or=age.eq
  • GET /people?age.eq=15&or=(age.eq,age.eq.17)
  • GET /people?or=(age.eq.15,age.eq.17)

@steve-chavez
Copy link
Member

Though this one would be fixed by moving the operators to the left side(as proposed on #2066), it would also be possible to have at least the eq filter working with swagger UI and HTML forms by defaulting to eq for a filter.

So /tbl?id=a would be the same as /tbl?id=eq.a. This is similar to how RPC works with arguments, id=a is id=arg.a(this was the original design).

This would at least solve this issue since it's related to eq. For other operators we would need to do #2066.

@ronnyTodgers
Copy link

Big thumbs up for defaulting to an eq in the absence of an operator where one is assumed! And yes longer term #2066 looks like a really neat way to allow the operators to extend basic functionality rather than trip noobs up when they try and use the basics

@ronnyTodgers
Copy link

And great timing I was just starting to write an nginx rewriter to mangle the eqs into requests from swagger :)

@wolfgangwalther
Copy link
Member

[...] it would also be possible to have at least the eq filter working with swagger UI and HTML forms by defaulting to eq for a filter.

So /tbl?id=a would be the same as /tbl?id=eq.a. This is similar to how RPC works with arguments, id=a is id=arg.a(this was the original design).

This would at least solve this issue since it's related to eq.

I don't think we should default to eq without operator. This makes it really hard to implement other things.

Instead, the correct solution seems to me to add variants for each operator to the OpenAPI parameters output.

So right now, we have something like this:

    "/users": {
      "get": {
        "parameters": [
          {
            "$ref": "#/parameters/rowFilter.users.lastname"
          },
          {
            "$ref": "#/parameters/rowFilter.users.firstname"
          },

but we should have something like:

    "/users": {
      "get": {
        "parameters": [
          {
            "$ref": "#/parameters/rowFilter.users.lastname.eq"
          },
          {
            "$ref": "#/parameters/rowFilter.users.lastname.like"
          },
          [...]
          {
            "$ref": "#/parameters/rowFilter.users.firstname.eq"
          },
          {
            "$ref": "#/parameters/rowFilter.users.firstname.like"
          },
          [...]

@laurenceisla
Copy link
Member

but we should have something like:

    "/users": {
      "get": {
        "parameters": [
          {
            "$ref": "#/parameters/rowFilter.users.lastname.eq"
          },
          {
            "$ref": "#/parameters/rowFilter.users.lastname.like"
          },
          ...

This should be done only after we move the operator from col=op.val to col.op=val in the query string, right? Swagger would show lastname.eq as a parameter and translate it to lastname.eq=value, which does not work right now.

@wolfgangwalther
Copy link
Member

This should be done only after we move the operator from col=op.val to col.op=val in the query string, right? Swagger would show lastname.eq as a parameter and translate it to lastname.eq=value, which does not work right now.

Correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement a feature, ready for implementation http http compliance OpenAPI
Development

No branches or pull requests

5 participants