Open taimoorzaeem opened 2 weeks ago
@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?
Shouldn't this mean that if the result has only 1 row, it should return
Content-Range: 0-0/*
instead of1-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 newLimitOffsetSpec.hs
for testing limit and offset query params? WDYT?
I agree, they must be separated now.
The other requests with
limit=...&offset=...
without theRange
header, must not returnContent-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?
Alright, now with this, I am also assuming that
Prefer: count=exact/planned/estimated
will only be applied whenRange: ..
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 https://github.com/PostgREST/postgrest/issues/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 aRange
. RFC 7233 § 4.2
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: []
Alright, now with this, I am also assuming that
Prefer: count=exact/planned/estimated
will only be applied whenRange: ..
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:
Content-Range
(that is, make all the requests return the header like before).Content-Range
header unless Range
is sent", which is a standalone issue, and it may need a confirmation for the rest of the team.
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?