Feature request: support for filter/condition that is always false (ie. where false)

[john-kelly]

First and foremost, thank you very much for your time

Summary

I've discussed this issue in the past on gitter, however, I thought I'd make a more formal request for this functionality. In short, I'm in need of support for a filter/condition that always fails (ie. where false). This request is a matter of elegance/beauty + handling edge cases rather than desiring new functionality.

In sql terms, what I'm requesting is something along the lines of:

... where false

We already have support for:

... where true

through implicit means (specifying no filters).

Context

I'm the author of elm-postgrest, and I'm under the impression that this functionality would allow my API to be more elegant + simpler (and I assume the same will be true for other client libraries). Let me try to demonstrate (examples will be in elm, however, those familiar with haskell should feel at home).

Let's start by looking at the current/desired API for a readMany request in elm-postgrest:

readMany :
    Schema id attributes
    ->
        { select : Selection attributes a
        , where_ : Condition attributes
        , order : List (Order attributes)
        , limit : Maybe Int
        , offset : Maybe Int
        }
    -> Request (List a)

and now some example usage:

getArticles : Request List
getArticles =
    PG.readMany schema
        { select = selection
        , where_ = PG.true
        , order = []
        , offset = Nothing
        , limit = Nothing
        }

as you can see PG.true has the type:

PG.true : Condition attributes

In addition to PG.true, elm-postgrest also supports PG.not. This function has the type:

not : Condition attributes -> Condition attributes

And, by extension of having both PG.true and PG.not, in order for things to remain consistent, we must also have PG.false.

Now, to be clear, I understand that I could represent the PG.true case with something other than a "true" Condition attributes. For example, readMany could have the type:

readMany :
    Schema id attributes
    ->
        { select : Selection attributes a
        , where_ : Maybe (Condition attributes) <----- NOTE: api change
        , order : List (Order attributes)
        , limit : Maybe Int
        , offset : Maybe Int
        }
    -> Request (List a)

and example usage being:

getArticles : Request List
getArticles =
    PG.readMany schema
        { select = selection
        , where_ = Nothing <------ NOTE: api change
        , order = []
        , offset = Nothing
        , limit = Nothing
        }

and if that were the only case where this fudged the api, I may feel differently regarding the need for this feature, however, there is in fact one other place where the API suffers and that's with the 2 functions PG.all and PG.any (representing postgrest and + or respectively). Those functions have the types:

all : List (Condition attributes) -> Condition attributes

any : List (Condition attributes) -> Condition attributes

In order for these functions to work as expected, they need to have the behavior:

all [] --> true
any [] --> false

Once again, I'm aware of the API change to all and any in order to avoid this issue:

all : (Condition attributes, List (Condition attributes)) -> Condition attributes

any : (Condition attributes, List (Condition attributes)) -> Condition attributes

however, it is at this point that I began to question its elegance.

Workarounds

If support for limit 0 were added, I may be able simulate the behavior that I need. We can discuss/explore this point further if need be.

[steve-chavez]

It makes sense to allow LIMIT 0 from the PostgreSQL side, however from the HTTP side we're mapping the usage of limit to the Range header semantics defined in RFC 7233, my understanding of the RFC is that requesting an empty range should be a 416(client error) and even if we allow this don't see how to send a valid Content-Range that describes the empty response.

Saw a discussion regarding this, seems we can ignore the Content-Range for this case and just return a 200, this could be an option for implementing this.

Another option is to allow a true/false inside the and/or operators, so you could have a /items?and=(false).

[john-kelly]

The suggested

Another option is to allow a true/false inside the and/or operators, so you could have a /items?and=(false).

would work well for my needs, however, I don't know that I have enough postgrest specific context to weigh in on whether or not this change makes sense for the project as a whole.

With that being said, I am under the impression that the surface level client API issues (presented in this issue) will occur in other clients (assuming they create a Condition abstraction)

[steve-chavez]

It makes sense to allow LIMIT 0 from the PostgreSQL side, however from the HTTP side we're mapping the usage of limit to the Range header semantics defined in RFC 7233

It's not really required that we map our ?limit=0 to the Range header logic(which btw needs a revision as noted in https://github.com/PostgREST/postgrest/issues/1089#issuecomment-515184613), the main reason we did that was because we could reuse the code in RangeQuery.hs.

I think we can make an exception(for ?limit) in RangeQuery for allowing a 0 row response.

[gautam1168]

@steve-chavez can I sum up this thread/feature request as, the following GET request should not fail?

http://localhost:3000/table?limit=0

Its returning 416 now and it should return a blank array right?

[steve-chavez]

@gautam1168 Yes, correct!

[gautam1168]

@steve-chavez I have figured out what I need to do to make this work. I am a bit conflicted about one change that I will have to make though. I will have to change the rangeOffset function in RangeQuery.hs to remove the line that panics. Like this:

rangeOffset :: NonnegRange -> Integer
rangeOffset range =
  case rangeLower range of
    BoundaryBelow lower -> lower
    _                   -> 0
    -- _                   -> panic "range without lower bound" -- should never happen

Do you think this is a safe change to make? Or is there a reason for this function to panic if Range without lower boundary is coming in from somewhere?

Alternatively I can modify the implementation of limitParams in the ApiRequests.hs file so that the following code does not create a range without a lower boundary when limit=0 is passed

limitParams :: M.HashMap ByteString NonnegRange
  limitParams  = M.fromList [(toS (replaceLast "limit" k), restrictRange (readMaybe . toS =<< v) allRange) | (k,v) <- qParams, isJust v, endingIn ["limit"] k]

Can you point me to which one of these would be the preferred change? I am guessing the second one right?

[steve-chavez]

Alternatively I can modify the implementation of limitParams in the ApiRequests.hs file so that the following code does not create a range without a lower boundary when limit=0 is passed

@gautam1168 Yeah, that seems simpler and better(the RangeQuery.hs module has been pretty stable/focused, changes have been too rare). Go for it!

[gautam1168]

@steve-chavez I have been trying to make it work by modifying the code in ApiRequests.hs but it looks like it won't be possible to achieve this without making a little change in RangeQuery.hs. This is because, this feature boils down to supporting emptyRange throughout the project. So, when a request comes with a limit=0 the ApiRequest.hs module must not give up just when it sees an emptyRange and return a

topLevelRange == emptyRange = Left InvalidRange

But also, SqlFragment.hs must construct a valid limitOffsetF fragment when it gets an emptyRange i.e. one with LIMIT and OFFSET set to 0 and rangeStatusHeader must create a valid status header with the emptyRange.

I can see that changing the behaviour of rangeOffset is not the cleanest thing to do. But I think the best solution here would be to make sure that whereever rangeOffset is called a check should be added to make sure that the range is not an emptyRange.

I know this looks like a dangerous change and perhaps not a beginner one. But Ill try to open a PR and then see how it goes. If it can't be merged, then it can't be merged.

I have checked that postgres supports queries like

SELECT * FROM xyz LIMIT 0 OFFSET 0

so, this is possible, but its a slightly more impactful change than perhaps was anticipated.