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

fix: [WIP] mixing offset and limit with Range header #3578

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

taimoorzaeem
Copy link
Collaborator

Fixes #3007.

@steve-chavez @laurenceisla @wolfgangwalther This is an attempt to fix this issue. There is still a lot of refactoring and testing left in this PR. For now, I would like you to review this redesign. Am I going in the right direction with this?

Copy link
Member

@laurenceisla laurenceisla left a comment

Choose a reason for hiding this comment

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

All in all, It's looking good! Separating Ranges and treating limit and offset as query strings should be the right choice. I'd say 👍 on continuing with this PR.

Some considerations below:

-- Replace .offset ending with .limit to be able to match those params later in a map
offsets = first (replaceLast "limit") <$> filter (endingIn ["offset"] . fst) nonemptyParams
offset = lookupParam "offset"
limit = lookupParam "limit"
lookupParam :: Text -> Maybe Text
Copy link
Member

@laurenceisla laurenceisla Jun 10, 2024

Choose a reason for hiding this comment

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

This should still allow doing something like

/clients?select=*,projects(*)&projects.limit=1

I think this change breaks that.

Comment on lines 143 to 145
toJSON LimitNoOrderError = toJsonPgrstError
ApiRequestErrorCode09 "A 'limit' was applied without an explicit 'order'" Nothing (Just "Apply an 'order' using unique column(s)")

Copy link
Member

Choose a reason for hiding this comment

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

This also may be related. It breaks limited updates/deletes which need order to be present in the query string.

@taimoorzaeem
Copy link
Collaborator Author

taimoorzaeem commented Jun 11, 2024

@laurenceisla @steve-chavez How should this change now reflect in Content-Range header in below tests? According to mdn the range start and end are zero indexed and inclusive. Shouldn't this mean that if the result has only 1 row, it should return Content-Range: 0-0/* instead of 1-1/*?

it "using the Range header" $
          request methodGet "/rpc/getitemrange?min=2&max=4"
                  (rangeHdrs (ByteRangeFromTo 1 1)) mempty
             `shouldRespondWith` [json| [{"id":4}] |]
              { matchStatus = 200
              , matchHeaders = ["Content-Range" <:> "1-1/*"]
              }

        it "using limit and offset" $ do
          post "/rpc/getitemrange?limit=1&offset=1" [json| { "min": 2, "max": 4 } |]
             `shouldRespondWith` [json| [{"id":4}] |]
              { matchStatus = 200
              , matchHeaders = ["Content-Range" <:> "1-1/*"]
              }
          get "/rpc/getitemrange?min=2&max=4&limit=1&offset=1"
             `shouldRespondWith` [json| [{"id":4}] |]
              { matchStatus = 200
              , matchHeaders = ["Content-Range" <:> "1-1/*"]

Edit: Also, now that limit and offset are separated from range header, how about we also separate their test cases? RangeSpec.hs for range header and a new LimitOffsetSpec.hs for testing limit and offset query params? WDYT?

@taimoorzaeem taimoorzaeem force-pushed the fix/limit-offset branch 2 times, most recently from f2a00dc to cde8eb8 Compare June 12, 2024 10:39
@taimoorzaeem taimoorzaeem marked this pull request as ready for review June 12, 2024 11:09
@laurenceisla
Copy link
Member

Shouldn't this mean that if the result has only 1 row, it should return Content-Range: 0-0/* instead of 1-1/*?

Not necessarily, remember that the Range can have an offset, e.g. Range: 1-1, which starts on the second item and ends in the same second item.
Then the response header is valid: Content-Range: 1-1/*. This is happening in the first example (rangeHdrs (ByteRangeFromTo 1 1) = Range: 1-1).

        it "using the Range header" $
          request methodGet "/rpc/getitemrange?min=2&max=4"
                  (rangeHdrs (ByteRangeFromTo 1 1)) mempty
             `shouldRespondWith` [json| [{"id":4}] |]
              { matchStatus = 200
              , matchHeaders = ["Content-Range" <:> "1-1/*"]
              }

The entire resource (without the Range) is {"id": 3, "id": 4}, then Range: 1-1 takes {"id": 4}.

The other requests with limit=...&offset=... without the Range header, must not return Content-Range (mixing both needs to be tested too, there's an example in the original issue on what it should return).

Edit: Also, now that limit and offset are separated from range header, how about we also separate their test cases? RangeSpec.hs for range header and a new LimitOffsetSpec.hs for testing limit and offset query params? WDYT?

I agree, they must be separated now.

@taimoorzaeem
Copy link
Collaborator Author

The other requests with limit=...&offset=... without the Range header, must not return Content-Range (mixing both needs to be tested too, there's an example in the original issue on what it should return).

@laurenceisla Alright, now with this, I am also assuming that Prefer: count=exact/planned/estimated will only be applied when Range: .. was sent in the request. Is my assumption correct?

@laurenceisla
Copy link
Member

laurenceisla commented Jun 13, 2024

Alright, now with this, I am also assuming that Prefer: count=exact/planned/estimated will only be applied when Range: .. was sent in the request. Is my assumption correct?

Yes that would be correct. This will make PostgREST a bit more HTTP compliant, so this PR is also partially related to #1089. There, it mentions:

[...] in RFC 7233 Content-Range is only a feature of special partial responses, which are indicated by status code 206 (Partial Content), which in turn can’t happen unless the request has a Range. RFC 7233 § 4.2

@taimoorzaeem
Copy link
Collaborator Author

it "updates with limit/offset using table default values(field-with_sep) when json keys are undefined" $ do
          request methodPatch "/complex_items?select=id,name&columns=name,field-with_sep&limit=1&offset=2&order=id"
            [("Prefer", "return=representation"), ("Prefer", "missing=default")]
            [json|{"name": "Tres"}|]
            `shouldRespondWith`
            [json|[
              {"id":3,"name":"Tres"}
            ]|]
            { matchStatus  = 200
            , matchHeaders = ["Preference-Applied" <:> "missing=default, return=representation"]
            }

I am having trouble figuring out what this one test is NOW supposed to return and what could be the problem here? @laurenceisla WDYT?

Failures:

  test/spec/Feature/Query/UpdateSpec.hs:356:13: 
  1) Feature.Query.UpdateSpec, Patching record, PATCH with ?columns parameter, apply defaults on missing values, updates with limit/offset using table default values(field-with_sep) when json keys are undefined
       body mismatch:
         expected: [{"id":3,"name":"Tres"}]
         but got:  []


@laurenceisla
Copy link
Member

laurenceisla commented Jun 14, 2024

Alright, now with this, I am also assuming that Prefer: count=exact/planned/estimated will only be applied when Range: .. was sent in the request. Is my assumption correct?

Yes that would be correct. This will make PostgREST a bit more HTTP compliant, so this PR is also partially related to #1089.

Wait, I think I made a mistake here. I mean, this is correct and needs to be changed, but it would be better to do so in a different PR. I didn't take in consideration that the Content-Range was returned on ALL requests right now.

Sorry about this, you made considerable changes to the tests and the process that generates the Content-Range. You have these options here:

  1. Revert the changes in Content-Range (that is, make all the requests return the header like before).
  2. Keep the changes, but they definitely need to be on a different PR. The change is "Do not return Content-Range header unless Range is sent", which is a standalone issue, and it may need a confirmation for the rest of the team.

@taimoorzaeem taimoorzaeem marked this pull request as draft July 20, 2024 14:32
@taimoorzaeem taimoorzaeem changed the title fix: mixing offset and limit with Range header [WIP] fix: mixing offset and limit with Range header Jul 20, 2024
@taimoorzaeem taimoorzaeem marked this pull request as ready for review July 27, 2024 07:22
@taimoorzaeem taimoorzaeem changed the title [WIP] fix: mixing offset and limit with Range header fix: [WIP] mixing offset and limit with Range header Jul 27, 2024
@taimoorzaeem
Copy link
Collaborator Author

@laurenceisla @steve-chavez @wolfgangwalther One test case that is remaining to be fixed is:

test/spec/Feature/Query/UpdateSpec.hs:368:13: 
  1) Feature.Query.UpdateSpec, Patching record, PATCH with ?columns parameter, apply defaults on missing values, updates with limit/offset using table default values(field-with_sep) when json keys are undefined
       body mismatch:
         expected: [{"id":3,"name":"Tres"}]
         but got:  []

Any idea how is this test giving this result?

@wolfgangwalther
Copy link
Member

test/spec/Feature/Query/UpdateSpec.hs:368:13: 
  1) Feature.Query.UpdateSpec, Patching record, PATCH with ?columns parameter, apply defaults on missing values, updates with limit/offset using table default values(field-with_sep) when json keys are undefined
       body mismatch:
         expected: [{"id":3,"name":"Tres"}]
         but got:  []

Any idea how is this test giving this result?

This PR is too big for me to look at - I don't even know where to start. Is there any chance to somehow split this one commit into multiple pieces, where each piece can stand alone, make sense and have passing tests?

@taimoorzaeem
Copy link
Collaborator Author

This PR is too big for me to look at - I don't even know where to start. Is there any chance to somehow split this one commit into multiple pieces, where each piece can stand alone, make sense and have passing tests?

I think not. The range header and limit and offset were too intermingled that it's almost impossible (at least not for me). Also, I wrote this patch quite a while ago, so it would be kinda hard for me to even understand the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Proposal to stop mixing limit and offset with the Range header
3 participants