conditions: datetime headers supports new TZs #4087
Conversation
Some headers of type date time, like If-Unmodified-Since, If-Modified-Since and other can support timezones other than GMT, like UTC, PST, CEST etc.. Though the S3 spec explicitly requires GMT, AWS S3 in practice accepts time.RFC1123 date format, so this PR follows the same behavior.
|
@vadmeste, thanks for your PR! By analyzing the history of the files in this pull request, we identified @harshavardhana, @hackintoshrao and @krisis to be potential reviewers. |
| "UTC": 0, | ||
|
|
||
| "CET": 1 * 3600, | ||
| "CEST": 2 * 3600, |
There was a problem hiding this comment.
Why only these timezone? what is 3600? why can't we use time.Hour constant?
| // minioTZ, call only the standard golang time parsing. | ||
| func parseTime(layout, value string) (time.Time, error) { | ||
| // Standard golang time parsing | ||
| t, err := time.Parse(layout, value) |
There was a problem hiding this comment.
Why can we use https://golang.org/pkg/time/#ParseInLocation?
There was a problem hiding this comment.
Why can we use https://golang.org/pkg/time/#ParseInLocation?
We can't, ParseInLocation needs location as parameter and the purpose of this PR is to guess the location from the date string.
For example, if we have this date Fri, 03 Apr 2015 17:10:00 PST, golang can parse this string but it doesn't consider PST because it doesn't know it. The parsed date will automatically have UTC timezone which is wrong.
There was a problem hiding this comment.
My observation on time/timezone in Go is below
- Go has an issue supporting abbreviated timezone.
time.Parse()sets abbreviated timezone text, not correct value to know time difference with UTC.- However
time.ParseInLocation()works properly when timezone is passed as argument. - As RFC1123 carries abbreviated timezone we have problem with name collision eg. CST refers to multiple timezone.
Based on the above we could do
- No support other than GMT as per RFC.
- If we support other than GMT, we would need to support all of them (Note: there is no solution to abbreviated timezone name collision).
If we go with option 2, the right fix would just have a map of abbreviated name to full name and use time.ParseInLocation() like below
var NameLocationMap = map[string]string{"PST":"US/Pacific", ...}
t, err = time.Parse(time.RFC1123, timeString)
l, err = t.Location()
location, ok = NameLocationMap[l.String()]
if ok {
l, err = time.LoadLocation(location)
t, err = time.ParseInLocation(time.RFC1123, timeString, l)
}
fmt.Println("local time:", t.Local())
fmt.Println("UTC time:", t.UTC())There was a problem hiding this comment.
@balamurugana, the problem with locations is that they are taking care of summer time shift, here US/Pacific can be PST or PDT, that's why I opted the time offset solution (-8, -7, ..)
I knew about timezone collision, but AWS S3 is already doing that..
Codecov Report
@@ Coverage Diff @@
## master #4087 +/- ##
==========================================
+ Coverage 67.68% 67.68% +<.01%
==========================================
Files 175 176 +1
Lines 24152 24175 +23
==========================================
+ Hits 16347 16364 +17
- Misses 6678 6683 +5
- Partials 1127 1128 +1
Continue to review full report at Codecov.
|
|
|
||
| import "time" | ||
|
|
||
| var minioTZ = map[string]int{ |
There was a problem hiding this comment.
We need to expand this.. but i think we are better off finding a better approach here. This will make it harder to add new timezones etc.
There was a problem hiding this comment.
i think we are better off finding a better approach here. This will make it harder to add new timezones etc.
I tried, but there is no other way.
We need to expand this..
We can add a new dependency for a more complete list.
IMO we should stick to GMT only as per RFC, and perhaps address this case by case basis as and when the issue arises. It is expected that SDKs should set the proper values in accordance with HTTP standard. Otherwise we will end up writing a lot of boilerplate code which would require us to mimic a new time package for parsing and can be done as a separate package. |
Yes, probably we will never see a client sending a date with a timezone other than GMT, besides the current state is complying with the S3 spec. Closing this.. |
Description
Some headers of type date time, like If-Unmodified-Since, If-Modified-Since
and other can support timezones other than GMT, like UTC, PST, CEST etc..
Though the S3 spec explicitly requires GMT, AWS S3 in practice accepts
time.RFC1123 date format, so this PR follows the same behavior.
Motivation and Context
Fixes #4082
How Has This Been Tested?
Types of changes
Checklist: