Incorrect date timezone assumption while importing #361

Closed
wants to merge 1 commit into
from

Projects

None yet

5 participants

@madninja
madninja commented Jan 3, 2013

The server returns data using GMT ("Zulu"-time). Setting the formatter's timezone to local adjusts the time to the local time which results in an incorrect date.

This was also failing the unit test. This fixes the basic bug. A slightly larger feature/fix could allow adjusting the timezone by allowing a timeZone attribute in the data model.

Marc Nijdam Incorrect date timezone assumption while importing
The server returns data using GMT ("Zulu"-time). Setting the formatter's timezone to local adjusts the time to the local time which results in an incorrect date.

This was also failing the unit test. This fixes the basic bug. A slightly larger feature/fix could allow adjusting the timezone by allowing a timeZone attribute in the data model.
bc472ed
@aaronbrethorst

This just bit me, too. Please merge.

Owner

@tonyarnold Somehome this pull request was merged in as a branch rather than integrated onto master. Would you be able to get this fixed Tony?

@tonyarnold
Contributor

Sorry guys – yes, I can merge this — I just need to verify it locally before I pull it into develop. We're due a really good cleanup. The whole team has been busy IRL. Thanks for the bump.

@madninja

We're getting super close to a big app release and I'd like to base it of a real version of MR instead of a bug fix branch. @tonyarnold Any chance at all this will get in in the next few days?

@tonyarnold
Contributor

Looking at this — do all servers return GMT? Is the timezone something that really should be exposed?

I'm going to roll MR 2.1.1 with a few similar bug fixes to this one over the weekend, but it strikes me that setting this to GMT makes as big an assumption as setting it to the machine's local time zone. I'd like to fix this permanently.

@casademora
Member

Can the time Zone offset be specified in the dateFormat attribute that
interprets the date from the external source? This would allow external
configuration and not require a code change.

Saul Mora
@casademora

On Apr 11, 2013, at 8:45 AM, Tony Arnold notifications@github.com wrote:

Looking at this — do all servers return GMT? Is the timezone something that
really should be exposed?

I'm going to roll MR 2.1.1 with a few similar bug fixes to this one over
the weekend, but it strikes me that setting this to GMT makes as big an
assumption as setting it to the machine's local time zone. I'd like to fix
this permanently.


Reply to this email directly or view it on
GitHubhttps://github.com/magicalpanda/MagicalRecord/pull/361#issuecomment-16238939
.

@madninja

On Apr 11, 2013, at 7:45 AM, Tony Arnold notifications@github.com wrote:

Looking at this — do all servers return GMT?

Currently the function is always called with the following format: @"yyyy-MM-dd'T'HH:mm:ss'Z'"
The "Z" means Zulu time which is 0 seconds offset from GMT. Interpreting that time as the local time by setting the timezone to the local timezone is causing a7 hour shift in the parsed time.

The unit tests point this out as well.. they fail on date parsing. This patch fixes it

I don't know why this is not hitting other apps as well. Perhaps they're not importing JSON dates or use a custom import function to work around the issue.

Is the timezone something that really should be exposed?

Yes, that would be good. I guess if you specify a date format in the userInfo you should also specify a timezone "name" or "offset" to go with that format. The default should always be offset 0 (GMT)

I'm going to roll MR 2.1.1 with a few similar bug fixes to this one over the weekend, but it strikes me that setting this to GMT makes as big an assumption as setting it to the machine's local time zone. I'd like to fix this permanently.


Reply to this email directly or view it on GitHub.

@madninja

Why yes.. that's what the 'Z' means.. The timezone of the sent timestamp is given in the GMT (Zulu) time.

On Apr 11, 2013, at 7:54 AM, Saul Mora notifications@github.com wrote:

Can the time Zone offset be specified in the dateFormat attribute that
interprets the date from the external source? This would allow external
configuration and not require a code change.

Saul Mora
@casademora

On Apr 11, 2013, at 8:45 AM, Tony Arnold notifications@github.com wrote:

Looking at this — do all servers return GMT? Is the timezone something that
really should be exposed?

I'm going to roll MR 2.1.1 with a few similar bug fixes to this one over
the weekend, but it strikes me that setting this to GMT makes as big an
assumption as setting it to the machine's local time zone. I'd like to fix
this permanently.


Reply to this email directly or view it on
GitHubhttps://github.com/magicalpanda/MagicalRecord/pull/361#issuecomment-16238939
.

Reply to this email directly or view it on GitHub.

@casademora
Member

Eh, no, there is a default date format which is what you state there. However, this can be overridden per attribute. The way you do this is by adding the 'dateFormat' key to the userInfo dictionary in the data model with your date format, including your timezone offset. This means that even when your server returns a different timezone offset for every date endpoint, you can adjust it for each one. There's some info on this blog post I wrote a while ago about the specifics of where these configuration attributes go: http://www.cimgf.com/2012/05/29/importing-data-made-easy/

@madninja

You're right and I'm not expressing the bug clearly. The dataFormat can be set in the userInfo dictionary, if not set a default format is used. Let's assume here that we're talking about the default format: @"yyyy-MM-dd'T'HH:mm:ss'Z'"

What is not clear in the referenced article is what happens with the timezone part of that format when MR imports a date with that format. My understanding is that the date should be parsed with the timezone set to the encoded timezone. This seems to be what is declared in the unit test as well, which fails without this patch.

What appears to be happening is that we force the NSDateFormatter to local time and then feed it a 'Z'/GMT time. This cause the resulting NSDate to be offset the wrong way.

@tonyarnold
Contributor

So why are we even including the call to - (void)setTimeZone:? Getting rid of it will cause the date formatter to guess the timezone from the passed string. This would seem to be the most logical approach here — we probably shouldn't be second guessing the system APIs unless there's a known bug. Less code, hooray! 👍

Am I missing something here?

@madninja

I didn't know that was standard behavior. If so, heck yah!

On Apr 14, 2013, at 3:11, Tony Arnold notifications@github.com wrote:

So why are we even including the call to - (void)setTimeZone:? Getting rid of it will cause the date formatter to guess the timezone from the passed string. This would seem to be the most logical approach here — we probably shouldn't be second guessing the system APIs unless there's a known bug. Less code, hooray!

Am I missing something here?


Reply to this email directly or view it on GitHub.

@madninja

So I take it 2.1.1 is not going to happen in the next day or so.. I need to push this thing out, so I'll do an import* method to use a custom dateformatter that doesn't mess with the timezone. Looking forward to 2.1.1!

@tonyarnold
Contributor

Ugh, sorry guys — I got totally waylaid with client work. You can see by the massive swathe of unanswered issues that the same thing happened to the rest of the team. I’m making a concerted effort to get to all of these older issues and pull requests — bear with me!

@tonyarnold tonyarnold was assigned Aug 19, 2013
@a2
a2 commented Oct 21, 2013

What ever happened to this PR?

@tonyarnold
Contributor

A similar PR was merged in 74d53e7. Thanks for talking me through this one — you’d think after over a decade as a web developer I’d have some idea about date formats returned from web servers…

There are tests verifying the new behaviour in develop, but please feel free to reopen this issue if I’ve missed something.

@tonyarnold tonyarnold closed this Dec 29, 2013
@madninja

Phew! Thanks for getting it in at last. The existing tests were already failing but I like the additional coverage.

On Dec 29, 2013, at 5:38 AM, Tony Arnold notifications@github.com wrote:

A similar PR was merged in 74d53e7. Thanks for talking me through this one — you’d think after over a decade as a web developer I’d have some idea about date formats returned from web servers…

There are tests verifying the new behaviour in develop, but please feel free to reopen this issue if I’ve missed something.


Reply to this email directly or view it on GitHub.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment