Skip to content

Do not respond with Content-Range header unless Range is requested #3921

New issue

Have a question about this project? No Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “No 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? No Sign in to your account

Open
taimoorzaeem opened this issue Feb 25, 2025 · 9 comments
Open
Labels
breaking change A bug fix or enhancement that would cause a breaking change http http compliance idea Needs of discussion to become an enhancement, not ready for implementation

Comments

@taimoorzaeem
Copy link
Collaborator

Problem

Currently we are returning Content-Range header on every request.

$ curl -i localhost:3000/items
HTTP/1.1 200 OK
Content-Range: 0-14/*

RFC says

The Content-Range header field has no meaning for status codes that do not explicitly describe its semantic. For this specification, only the 206 (Partial Content) and 416 (Range Not Satisfiable) status codes describe a meaning for Content-Range.

The Content-Range should only be returned as a response to a Range request.

Solution

A more appropriate header for returning response length should be X-Total-Count as mentioned in this Stackoverflow question. It is a custom header so we can also tweak it.

$ curl -i localhost:3000/items
HTTP/1.1 200 OK
X-Total-Count: 15

This might help fix other related issues as well:

@taimoorzaeem taimoorzaeem added http http compliance idea Needs of discussion to become an enhancement, not ready for implementation labels Feb 25, 2025
@steve-chavez
Copy link
Member

Would doing this imply a breaking change?

X-Total-Count: 15

Another option instead of another header could be to put it as part of a custom content type: Content-Type: application/vnd.pgrst.json+array;length=15

Additionally, with aggregates and spreads maybe there could be a way to return both the root count and the data in the same response.

@wolfgangwalther
Copy link
Member

The Content-Range header field has no meaning for status codes that do not explicitly describe its semantic. For this specification, only the 206 (Partial Content) and 416 (Range Not Satisfiable) status codes describe a meaning for Content-Range.

The Content-Range should only be returned as a response to a Range request.

I don't quite understand why we would need to return it for other status codes, can you elaborate?

This might help fix other related issues as well:

The issue explicitly mentions cutting off embedded results - which this proposal does not help at all with.

Any header-based solution is going to be problematic for embeddings. We can only solve the embedding problem by putting the count in the body, imho.

I don't see how a custom header can help here. The requirements of this issue are very specific - the Content-Range header should be returned correctly - which we don't do, yet. Returning.. another header instead, is not going to fix that.

I don't think we will improve HTTP compliance by introducing a custom header :D

I don't quite see, yet, how adding a third way of counting to the mix helps the situation. We already have Range and limit/offset, with potentially different semantics. Adding even more will only make it even more complex, right?


I don't see the upside of this proposal, yet.

@taimoorzaeem
Copy link
Collaborator Author

taimoorzaeem commented Feb 26, 2025

Would doing this imply a breaking change?

Yes.

I don't think we will improve HTTP compliance by introducing a custom header

HTTP compliance would be improved by removing Content-Range on every request. Not by introducing a new header.

I don't quite understand why we would need to return it for other status codes, can you elaborate?

I think you mean "why wouldn't we", because we are returning other status codes. The RFC said:

The Content-Range header field has no meaning for status codes that do not explicitly describe its semantic. For this specification, only the 206 (Partial Content) and 416 (Range Not Satisfiable) status codes describe a meaning for Content-Range.

It has no meaning for responses other than 206 and 416. Then why return it?

This proposal is more about removing Content-Range on every request than introducing a new header. Maybe I should edit the title. I just thought that PostgREST users might still need a total count in absence of Content-Range so a new header should replace this.

I also think it would easier to finish my work on #3578 with this change.

@taimoorzaeem taimoorzaeem added the breaking change A bug fix or enhancement that would cause a breaking change label Feb 26, 2025
@wolfgangwalther
Copy link
Member

The Content-Range header field has no meaning for status codes that do not explicitly describe its semantic. For this specification, only the 206 (Partial Content) and 416 (Range Not Satisfiable) status codes describe a meaning for Content-Range.

It has no meaning for responses other than 206 and 416. Then why return it?

Well, in terms of REST specs, the X-Total-Count has no meaning, too. So why return the same information in two different headers? This is about as bad UX as Accept-Profile and Content-Profile which is really confusing...

The RFC just says that the headers don't carry any meaning in terms of their RFC. But that doesn't mean it's forbidden to return them, right? So what is really bad about the fact that we currently return them everywhere? I don't get it, yet.

@taimoorzaeem
Copy link
Collaborator Author

taimoorzaeem commented Feb 26, 2025

The RFC just says that the headers don't carry any meaning in terms of their RFC. But that doesn't mean it's forbidden to return them, right? So what is really bad about the fact that we currently return them everywhere? I don't get it, yet

While fixing #3007, in #3578 it changes the result of Content-Range for almost all test cases (120+ cases). So, from a development and testing perspective it is making it more laborious. So it isn't bad but kind of a blocker for important fixes.

@taimoorzaeem taimoorzaeem changed the title X-Total-Count header Do not respond with Content-Range header unless Range is requested Feb 26, 2025
@wolfgangwalther
Copy link
Member

While fixing #3007, in #3578 it changes the result of Content-Range for almost all test cases (120+ cases). So, from a development and testing perspective it is making it more laborious. So it isn't bad but kind of a blocker for important fixes.

This seems like either a tooling problem or a problem with how we write the tests, to me.

But realistically, we should not be changing something around this header ever so often, that this becomes a problem to adjust in as many tests. If we do #3007, then yes we have a bigger change. But we'd have exactly the same kind of change just by doing this issue here. So it doesn't really matter where we have the big effort, if we only have it once.

@laurenceisla
Copy link
Member

laurenceisla commented Feb 26, 2025

The RFC just says that the headers don't carry any meaning in terms of their RFC. But that doesn't mean it's forbidden to return them, right? So what is really bad about the fact that we currently return them everywhere? I don't get it, yet.

Right. It doesn't seem to forbid the use of Content-Range outside of 206 and 416 responses, it just isn't defined (has no meaning). Somehow I interpreted it as it was only available in those cases (it was also interpreted that way in this issue).

Still, my original idea was to not return any header (remove Content-Range altogether) for requests without Range. If we wanted to know the Content-Range, then we'd need to send e.g. a Range: items=0- header to know. So yeah, I also disagree with creating a new header for this. (Not fully advocating for removing Content-Range anymore, since the RFC doesn't seem to forbid it as mentioned before).

@wolfgangwalther
Copy link
Member

If we wanted to know the Content-Range, then we'd need to send e.g. a Range: items=0- header to know.

I don't think this would satisfy the requirements of the RFC cited above, assuming that we'd still return with 200 here and not 206. I.e. if we return the full response even though a Range was requested. So if we were strict, we'd return 200, but no Content-Range, even in the case when this Range is explicitly requested.

@alexanderkjeldaas
Copy link

It would be great to have this fixed to avoid the requirement of a reverse proxy in front. For example react native and expo mobile apps refuse to work with postgrest.

No Sign up for free to join this conversation on GitHub. Already have an account? No Sign in to comment
Labels
breaking change A bug fix or enhancement that would cause a breaking change http http compliance idea Needs of discussion to become an enhancement, not ready for implementation
Development

No branches or pull requests

5 participants