Skip to content

Fix issues of renderRfc3339/parseRfc3339 concerning non UTC times#10

Open
sternenseemann wants to merge 4 commits intolpeterse:masterfrom
sternenseemann:fix-renderRfc3339
Open

Fix issues of renderRfc3339/parseRfc3339 concerning non UTC times#10
sternenseemann wants to merge 4 commits intolpeterse:masterfrom
sternenseemann:fix-renderRfc3339

Conversation

@sternenseemann
Copy link
Copy Markdown

Previously renderRfc3339 would interpret the offset of a Local time as minutes instead of seconds contrary to parseRfc3339 and the documented semantics of that type. This lead to the time zone offset being off by a factor of 60 when using renderRfc3339. Fixes #9.

Might add some tests for RFC3339 strings with time zone offsets later, if I get around to it.

@sternenseemann
Copy link
Copy Markdown
Author

sternenseemann commented Jun 19, 2020

On second thought this fix creates a second issue: RFC3339 can't handle offsets more accurate than minutes. This means if using say 3493 as offset, renderRfc3339 t would result in a different value than t when parsed using parseRfc3339 as the value would be truncated somehow.

I think there are two ways to fix this which both break the API of utc:

  1. Change the semantic of offset to minutes instead of seconds (would break the API).
  2. Use MonadThrow m instead of Monad m in rfc3339Builder and throw an exception when an offset is not divisible by 60.

I think option 1 is undesireable as it would break virtually all code using Local.

I'll add an implementation of the second fix to this PR, let me know if you want it changed in any way.

PS: Sorry for the horrible diff, my code quickly got indented a thousand levels, so I decided I'd rework the structure of the builder a bit.

Also adds a test case for this: addSecondFractions (-1.0) should exactly
behave like addSeconds (-1).

Resolves lpeterse#12.
Previously renderRfc3339 would interpret the offset of a Local time
as minutes instead of seconds contrary to parseRfc3339 and the
documented semantics of that type. This lead to the time zone offset
being off by a factor of 60 when using renderRfc3339.

Fixes lpeterse#9.
RFC3339 only supports offsets as accurate as a minute, but Local
provides the offset in seconds. This means that the rendering
of RFC3339 strings in utc is lossy, i. e. a value rendered and could be
different when parsed, as the RFC3339 string wouldn't be able to
represent it accurate enough. To prevent this we now refuse to render
Local values that would result in data loss by throwing an UtcException.
@sternenseemann
Copy link
Copy Markdown
Author

Added a fix for #11 to this PR. I think I found a solution to the problem that will break the least existing code. Using FlexibleContexts we can require Time (Local t) and Date (Local t) which is needed if we want to use the local time adjustment that the Time and Date instances of Local do.

For parseRfc3339 the fix was trivial, but it would fail for typical values of offset, since a whole multiple of -1.0 would be added (see #12). To address this I rebased the change from #13 into this PR as well.

@sternenseemann sternenseemann changed the title Interpret offset as seconds instead of minutes in renderRfc3339 Feix issues of renderRfc3339/parseRfc3339 concerning non UTC times Jun 27, 2020
@sternenseemann sternenseemann changed the title Feix issues of renderRfc3339/parseRfc3339 concerning non UTC times Fix issues of renderRfc3339/parseRfc3339 concerning non UTC times Jun 27, 2020
Contrary to the specification of RFC3339 as a local date time plus a
offset from UTC, renderRfc3339 and parseRfc3339 will now interpret
RFC3339 times as local times if an offset is given.

Resolves lpeterse#11.
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.

renderRfc3339 interprets offset as minutes instead of seconds

1 participant