llfbandit / dio_cache_interceptor

Dio HTTP cache interceptor with multiple stores respecting HTTP directives (ETag, Last-Modified, Cache-Control) with options.
https://pub.dev/packages/dio_cache_interceptor
120 stars 70 forks source link

Responses from cache shouldn't always return 304 ? #121

Closed jo11yn closed 11 months ago

jo11yn commented 1 year ago

Current behaviour :

When reading from store, the response is return with the method toResponse from CacheResponse. It always set statusCode to 304.

Proposal:

To my understanding of Http cache, when reading from store it should also read stored http status code and return it as it was send by the server. 304 status code should only be used by server to tell client data hasn't changed.

Example :

-> request /hello to server -> response is 200 with data "hello" and cache headers -> we save it to store -> request /hello to server again -> response is 200 with data "hello" from store

It would solve the problem of Dio considering 304 responses with not null body as error ( which is http standard ) and maybe this use case as well https://github.com/llfbandit/dio_cache_interceptor/pull/119

What do you think ? I can take a look at how to implement it if needed !

llfbandit commented 1 year ago

Thanks for the proposal, but I disagree with this.

As far as I know there's only on case that would fit to change the status code of the response. This is when a cache recipient (here this package) decides to revalidate its own entries by itself under certain conditions. This will never happen here (too light and the purpose is about private cache, not shared).

I may be wrong, but I don't get your point to change the status code. The package catches the error and resolves the request as valid.

Finally, your intention is unrelated to #119. This is about considering 304 status codes as valid status (not done by default by Dio). This specific setup, breaks the workflow of the interceptor and makes it useless.

jo11yn commented 1 year ago

Thanks for your response and i agree this is not the same use case as https://github.com/llfbandit/dio_cache_interceptor/pull/119, my bad.

To clarify my point here is a response from the favicon from https://www.google.fr in your browser. You can see on this image that browser cache has been hit and then the response status code is 200 ( from disk cache ) which is different from a 304 response from server. To me when cache is hit you can't have a 304 in http standards.

Capture d’écran 2023-05-03 à 16 19 56

This would ensure that without overriding validateStatus in Dio, responses from cache are handled properly by Dio instead of throwing an error because 304 shoudn't have a body.

DioError [bad response]: The request returned an invalid status code of 304

Hope i made it more clear, thanks for your time!

llfbandit commented 1 year ago

There's no wrong or right answer to this, because there's no rule about providing a 200 status code (more precicely the initial status code) when response is coming from cache. Still, providing the initial status code as a result could help.

This is a breaking change if we go this way.