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
conditions: datetime headers supports new TZs #4087
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,67 @@ | ||
/* | ||
* Minio Cloud Storage, (C) 2017 Minio, Inc. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
package cmd | ||
|
||
import "time" | ||
|
||
var minioTZ = map[string]int{ | ||
|
||
"PST": -8 * 3600, | ||
"PDT": -7 * 3600, | ||
|
||
"EST": -5 * 3600, | ||
"EDT": -4 * 3600, | ||
|
||
"GMT": 0, | ||
"UTC": 0, | ||
|
||
"CET": 1 * 3600, | ||
"CEST": 2 * 3600, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why only these timezone? what is 3600? why can't we use |
||
} | ||
|
||
// parseTime returns the correct time when a timezone is provided | ||
// with the time string, if the specified timezone is not found in | ||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why can we use https://golang.org/pkg/time/#ParseInLocation? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My observation on time/timezone in Go is below
Based on the above we could do
If we go with option 2, the right fix would just have a map of abbreviated name to full name and use 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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.. |
||
if err != nil { | ||
return time.Time{}, err | ||
} | ||
|
||
// Fetch the location of the passed time | ||
loc := t.Location() | ||
|
||
if loc == nil || loc == time.UTC || loc == time.Local { | ||
// Nothing to do, even when location is set to time.Local | ||
// since time will be obviously correct with the local | ||
// machine's timezone | ||
return t, nil | ||
} | ||
|
||
// Fetch the time offset associated to the passed timezone. | ||
// If not found, return golang std time parser result. | ||
offset, ok := minioTZ[loc.String()] | ||
if !ok { | ||
return t, nil | ||
} | ||
|
||
// Calculate the new time | ||
newTime := t.Add(-time.Duration(offset) * time.Second) | ||
|
||
return newTime, nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,52 @@ | ||
/* | ||
* Minio Cloud Storage, (C) 2017 Minio, Inc. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
package cmd | ||
|
||
import ( | ||
"testing" | ||
"time" | ||
) | ||
|
||
func TestParseTime(t *testing.T) { | ||
testCases := []struct { | ||
timeStr string | ||
timeLayout string | ||
timeUTC time.Time | ||
shouldPass bool | ||
}{ | ||
{"", time.RFC1123, time.Time{}, false}, | ||
{"foo", time.RFC1123, time.Time{}, false}, | ||
{"Fri, 03 Apr 2015 17:10:00 GMT", time.RFC1123, time.Date(2015, 04, 03, 17, 10, 00, 00, time.UTC), true}, | ||
{"Fri, 03 Apr 2015 17:10:00 UTC", time.RFC1123, time.Date(2015, 04, 03, 17, 10, 00, 00, time.UTC), true}, | ||
{"Fri, 03 Apr 2015 18:10:00 CET", time.RFC1123, time.Date(2015, 04, 03, 17, 10, 00, 00, time.UTC), true}, | ||
{"Fri, 03 Apr 2015 19:10:00 CEST", time.RFC1123, time.Date(2015, 04, 03, 17, 10, 00, 00, time.UTC), true}, | ||
{"Fri, 03 Apr 2015 09:10:00 PST", time.RFC1123, time.Date(2015, 04, 03, 17, 10, 00, 00, time.UTC), true}, | ||
} | ||
|
||
for i, testCase := range testCases { | ||
parsedTime, err := parseTime(testCase.timeLayout, testCase.timeStr) | ||
if err == nil && !testCase.shouldPass { | ||
t.Errorf("Test %d expected to fail but passed instead\n", i+1) | ||
} | ||
if err != nil && testCase.shouldPass { | ||
t.Errorf("Test %d expected to pass but failed with err: %v\n", i+1, err) | ||
} | ||
if !parsedTime.UTC().Equal(testCase.timeUTC) { | ||
t.Errorf("Test %d found an unexpected result, found: %v, expected: %v\n", i+1, parsedTime.UTC(), testCase.timeUTC) | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried, but there is no other way.
We can add a new dependency for a more complete list.