postgrest icon indicating copy to clipboard operation
postgrest copied to clipboard

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

Open artsra opened this issue 3 years ago • 10 comments

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 avatar Sep 30 '21 12:09 artsra

This will not work right now. We had some ideas in https://github.com/PostgREST/postgrest/pull/1949#issuecomment-922875327 and https://github.com/PostgREST/postgrest/pull/1949#issuecomment-923376956 that would make this work - but those are far from being implemented, yet.

wolfgangwalther avatar Oct 05 '21 08:10 wolfgangwalther

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 avatar Nov 05 '21 22:11 steve-chavez

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)

steve-chavez avatar Dec 02 '21 19:12 steve-chavez

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 https://github.com/PostgREST/postgrest/discussions/2063#discussioncomment-1737031.

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.

:heart:

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))

wolfgangwalther avatar Dec 03 '21 06:12 wolfgangwalther

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 avatar Dec 03 '21 23:12 steve-chavez

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

Though as I see it on https://github.com/PostgREST/postgrest/discussions/2063#discussioncomment-1743152, 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 avatar Dec 03 '21 23:12 steve-chavez

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 avatar Dec 06 '21 18:12 steve-chavez

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 avatar Dec 11 '21 06:12 steve-chavez

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.

steve-chavez avatar Dec 13 '21 18:12 steve-chavez

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)

wolfgangwalther avatar Dec 16 '21 20:12 wolfgangwalther

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 https://github.com/PostgREST/postgrest/issues/2066.

steve-chavez avatar Jan 14 '23 00:01 steve-chavez

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 avatar Feb 04 '23 01:02 ronnyTodgers

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

ronnyTodgers avatar Feb 04 '23 01:02 ronnyTodgers

[...] 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"
          },
          [...]

wolfgangwalther avatar Jun 09 '23 07:06 wolfgangwalther

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.

laurenceisla avatar Jun 09 '23 18:06 laurenceisla

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.

wolfgangwalther avatar Jun 10 '23 12:06 wolfgangwalther