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.

Sign up for GitHub

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

Jump to bottom

Proposal to stop mixing limit and offset with the Range header #3007

Open
laurenceisla opened this issue Oct 13, 2023 · 6 comments · May be fixed by #3578
Open

Proposal to stop mixing limit and offset with the Range header #3007

laurenceisla opened this issue Oct 13, 2023 · 6 comments · May be fixed by #3578
Assignees
@taimoorzaeem
Labels
enhancement a feature, ready for implementation http http compliance

Comments

@laurenceisla
Copy link
Member

laurenceisla commented Oct 13, 2023 •
edited
Loading

Problem

Proposal

  • Convert limit and offset as-is to SQL, e.g. ?offset=1&limit=3 to OFFSET 1 AND LIMIT 3 without taking into consideration the values in Range. The query builder generates an aggregate query that should apply the value of the Range, and the sub-query that should apply the limit and offset.
  • Limits and offsets will return the status 200 instead of 206 if it has no Range header, complying with RFC 7233 section-4.1.

Caveats

The selected resource will now include limits and offsets, so the Range values will apply over that selected resource, e.g. this query:

curl "http://localhost:3000/projects?select=id,name&offset=1" -H "Range: 1-"

Will no longer return

[{"id":2,"name":"Windows 10"},
 {"id":3,"name":"IOS"},
 {"id":4,"name":"OSX"},
 {"id":5,"name":"Orphan"}]

Instead, it will show:

[{"id":3,"name":"IOS"},
 {"id":4,"name":"OSX"},
 {"id":5,"name":"Orphan"}]

It does not mix offset=1 with Range: 1-, it applies the
offset first and then the first position range value over the already offset query.
@wolfgangwalther
Copy link
Member

This sounds like a great idea. It also has very nice semantics: Changing the URL actually changes the requested resource ("a list of 3" is a different thing from "a list of 5"), but using Range headers just "browses through the same resource".

@wolfgangwalther wolfgangwalther added the http http compliance label Oct 17, 2023
@steve-chavez steve-chavez added the enhancement a feature, ready for implementation label Oct 18, 2023
@steve-chavez steve-chavez mentioned this issue Oct 19, 2023
Merged
@steve-chavez
Copy link
Member

@laurenceisla Great idea! This will also make fixing the Range header #2204 less breaking.

@taimoorzaeem taimoorzaeem self-assigned this Jun 2, 2024
@taimoorzaeem
Copy link
Collaborator

Hmm, this would take a lot of change since limit and range is currently too mixed up. For starters, how would you like the following data structures to change:

-- data ApiRequest
...
, iRange               :: HM.HashMap Text NonnegRange    -- ^ Requested range of rows within response
, iTopLevelRange       :: NonnegRange                    -- ^ Requested range of rows from the top level
...

I think we can change them to two ranges, so one would represent the offset and limit range and other would represent the range header. So it may be like:

-- data ApiRequest
...
, iOffsetLimit               :: NonnegRange      -- ^ Requested range of rows within the selected resource
, iHeaderRange               :: NonnegRange      -- ^ Requested range of rows from the selected resource
...

@laurenceisla @steve-chavez @wolfgangwalther WDYT?

@taimoorzaeem
Copy link
Collaborator

@steve-chavez @laurenceisla

or this hack to allow LIMIT=0: Allow limit=0 in query params to return an empty array #2269

Also, does this change mean we need to remove the limit=0 feature?

@laurenceisla
Copy link
Member Author

laurenceisla commented Jun 3, 2024 •
edited
Loading

Hmm, this would take a lot of change since limit and range is currently too mixed up.

Yes, it may need a fair amount of refactoring and/or redesign to be done.

I think we can change them to two ranges

It's a valid approach, although I remembered I wanted to send limit and offset to the DB as-is and let it return the error.

Also, does this change mean we need to remove the limit=0 feature?

According to what I described above then limit=0 is LIMIT 0 in PostgreSQL, so that still should be valid. Edit: Ah, I see the confusion, I mistakenly put LIMIT=0 when it should be LIMIT 0 in the original post. It's fixed now.

@taimoorzaeem taimoorzaeem linked a pull request Jun 10, 2024 that will close this issue
Open
@wolfgangwalther
Copy link
Member

One important consideration for the example in the opening post:

What will the Content-Range header return for the total number of rows, i.e. the count?

Currently it would return 5 total rows, but if we clean this up, it should return 4 total rows. I.e. the row counting should only do so on the result of &limit=x&offset=y.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@taimoorzaeem taimoorzaeem

Labels
enhancement a feature, ready for implementation http http compliance
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: [WIP] mixing offset and limit with Range header taimoorzaeem/postgrest
4 participants
@steve-chavez @wolfgangwalther @laurenceisla @taimoorzaeem