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 default value for Expires header #1137

Merged
merged 1 commit into from
Mar 24, 2019

Conversation

thornbill
Copy link
Member

Changes
Changes the default value of the Expires HTTP header to be "0" instead of "-1". The RFC specifically mentions using "0" to represent values that have already expired.

The Java apiclient uses Gson for serializing the response and it logs warnings for the use of "-1".

Issues
N/A

Copy link
Contributor

@LogicalPhallacy LogicalPhallacy left a comment

Choose a reason for hiding this comment

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

Seems like it should have raised a problem before. Fix looks good.

Copy link
Member

@cvium cvium left a comment

Choose a reason for hiding this comment

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

The spec and what's actually implemented can be two different things. Eg. https://support.microsoft.com/en-us/help/234067/how-to-prevent-caching-in-internet-explorer and https://stackoverflow.com/a/11367872

And if you read the spec:

A cache recipient MUST interpret invalid date formats, especially the
value "0", as representing a time in the past (i.e., "already
expired").

It simply states that any invalid date value must be interpreted as "already expired".

If we want to be truly "correct", the default value should be changed to a date in the past.

@JustAMan
Copy link
Contributor

the default value should be changed to a date in the past.

Can we just set the date to "Unix start" (01-01-1970)?

@joshuaboniface
Copy link
Member

I disagree, the spec seems to indicate that 0 is the preferred way to specify the invalid date. Which makes sense, it's Unix start minus 1. I'm not in favour of hacks to support Microsoft's abomination known as IE. It's dead anyways.

@cvium
Copy link
Member

cvium commented Mar 22, 2019

If you want to be really pedantic about the wording in the spec, you're actually supposed to set the Expires header to the same value as the Date header.

@joshuaboniface
Copy link
Member

I don't really care either way, 0 is an invalid date in the past so it fits what's required. But if the change works it works, isn't a hack, and seems to follow the spec. Putting 1970-01-01 00:00:00 is a hack to me.

@joshuaboniface
Copy link
Member

So how do we want to handle this @thornbill @cvium?

@thornbill
Copy link
Member Author

I don’t have a strong opinion either way, but anything beyond a basic string replacement will be exceeding my C# abilities. 😉

@joshuaboniface
Copy link
Member

Let's just make it work however works best for you @cvium I'd like this to be in the release but don't want to delay it too much longer.

Copy link
Member

@cvium cvium left a comment

Choose a reason for hiding this comment

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

It's fine. I just don't agree that -1 is a hack as it's valid according to the spec. I haven't been able to find any information on whether this is still an issue in Edge, so let's just give it a shot...

@Bond-009 Bond-009 merged commit e971b62 into jellyfin:master Mar 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants