Add option to store datetimes as ISO-8601 formatted strings #223

william101 · 2018-01-11T14:56:01Z

Currently datetimes are stored in DynamoDB as :number (UNIX timestamps)

This pr add the option to store these timestamps as :string (ISO 8601)

Reference:

https://docs.aws.amazon.com/amazondynamodb/latest/developerguide/DynamoDBMapper.DataTypes.html
Date (as ISO_8601 millisecond-precision string, shifted to UTC)

pratik60 · 2018-01-11T14:57:23Z

README.md

@@ -598,6 +598,7 @@ Listed below are all configuration options.
 * `sync_retry_max_times` - when Dynamoid creates or deletes table synchronously it checks for completion specified times. Default is 60 (times). It's a bit over 2 minutes by default
 * `sync_retry_wait_seconds` - time to wait between retries. Default is 2 (seconds)
 * `convert_big_decimal` - if `true` then Dynamoid converts numbers stored in `Hash` in `raw` field to float. Default is `false`
+* `convert_date_to_string` - if `true`, datetimes will be stores as ISO-8601 formatted strings. When `false`, datetimes will be stores as UNIX timestamps with milisecond precission. Defaule is `false`


Typo, default

pratik60 · 2018-01-11T14:57:53Z

lib/dynamoid/persistence.rb

@@ -208,7 +197,7 @@ def dynamo_type(type)
              :string
            else
              raise 'unknown type'
-          end
+            end


Is this intentional?

Just my editor formatting the code. I'll update.

coveralls · 2018-01-11T15:26:22Z

Coverage decreased (-0.01%) to 97.316% when pulling c54c259 on pratik60:william/add_option_to_store_datetimes_as_strings into fb9db64 on Dynamoid:master.

coveralls · 2018-01-11T15:26:26Z

Coverage decreased (-0.01%) to 97.316% when pulling c54c259 on pratik60:william/add_option_to_store_datetimes_as_strings into fb9db64 on Dynamoid:master.

coveralls · 2018-01-11T15:54:12Z

Coverage decreased (-0.01%) to 97.316% when pulling 21376c9 on pratik60:william/add_option_to_store_datetimes_as_strings into fb9db64 on Dynamoid:master.

andrykonchin · 2018-01-11T18:33:16Z

lib/dynamoid/persistence.rb

+          when :utc
+            time_for_timezone(value, 'UTC')
+          when :local
+            time_for_timezone(value, Time.now.getlocal.zone)


ActiveSupport::TimeZone works badly with decree time and may loose 1 hour. That's why we used Time.at(value).to_datetime to convert time.

andrykonchin · 2018-01-11T18:41:47Z

lib/dynamoid/persistence.rb

-                      ActiveSupport::TimeZone[Dynamoid::Config.application_timezone].at(value).to_datetime
-                  end
-                end
+                parse_datetime(value)


It's a good refactoring. The feature could be done just with one additional line

value = DateTime.iso8601(value).to_time.to_i if Dynamoid::Config.convert_date_to_string

andrykonchin · 2018-01-11T18:43:19Z

lib/dynamoid/config.rb

@@ -26,6 +26,7 @@ module Config
    option :sync_retry_max_times, default: 60 # a bit over 2 minutes
    option :sync_retry_wait_seconds, default: 2
    option :convert_big_decimal, default: false
+    option :convert_date_to_string, default: false


To support existed projects and data we may need to specify this format not globally but for specific model and field (as well as allow to set it globally)

@andrykonchin wouldn't the default value of false make it backwards compatible with any existing implementation?

@pboling It would, sure.
But you could want to keep old format for existing tables and new format for new ones. You couldn't use new format if already have data in old one in any table.

Oh, I see. Agreed, this has to be model-specific per field.

andrykonchin · 2018-01-11T18:48:46Z

spec/dynamoid/persistence_spec.rb

+      end
+    end
+
+    include_examples 'dump_datetime_attributes'


I would wrap it with context context 'when convert_date_to_string == false' do

andrykonchin · 2018-01-11T18:58:39Z

spec/dynamoid/persistence_spec.rb

+        user = User.create(last_logged_in_at: time)
+        user = User.find(user.id)
+        expect(user.dump[:last_logged_in_at]).to be_a(String)
+        expect(user.dump[:last_logged_in_at]).to eq '2017-06-20T05:00:00+00:00'


The test passes only because it runs in UTC timezone and it fails when local time zone isn't UTC.
You should set application_timezone='UTC' for this test case. It's local by default

Ideally we would have tests for multiple time zones for any feature based on parsing time.

@pboling Agree, but I didn't find a way to change timezone in tests. We can travel in time with Timecop but we cannot change timezone.

pboling

New option option :convert_date_to_string, default: false must be model specific per field, not global.

…matted strings

coveralls · 2018-01-12T11:09:32Z

Coverage increased (+0.02%) to 97.346% when pulling a85fc93 on pratik60:william/add_option_to_store_datetimes_as_strings into fb9db64 on Dynamoid:master.

andrykonchin · 2018-01-12T11:16:21Z

README.md

+class Document
+  include DynamoId::Document
+
+  field :sent_at, :datetime, convert_date_to_string: true


We already have similar option for boolean field store_as_native_boolean

field :active, :boolean, store_as_native_boolean: true

It makes sense to name new option in similar way

@pboling What do you think?

Again, it may be useful to apply this new option globally and override it for specific field

We should follow existing naming patterns. 👍

coveralls · 2018-01-12T11:34:14Z

Coverage increased (+0.02%) to 97.346% when pulling a85fc93 on pratik60:william/add_option_to_store_datetimes_as_strings into fb9db64 on Dynamoid:master.

coveralls · 2018-01-15T10:06:11Z

Coverage increased (+0.03%) to 97.356% when pulling f5282a6 on pratik60:william/add_option_to_store_datetimes_as_strings into fb9db64 on Dynamoid:master.

coveralls · 2018-01-15T10:06:11Z

Coverage increased (+0.03%) to 97.356% when pulling f5282a6 on pratik60:william/add_option_to_store_datetimes_as_strings into fb9db64 on Dynamoid:master.

coveralls · 2018-01-15T10:06:11Z

Coverage increased (+0.03%) to 97.356% when pulling f5282a6 on pratik60:william/add_option_to_store_datetimes_as_strings into fb9db64 on Dynamoid:master.

pboling

This now addresses everything except @andrykonchin 's suggestion to make the setting an optional global setting with a per model override.

andrykonchin

Other improvements like global option or support string format for date fields can be done in separate PRs

Add option to store datetimes as ISO-8601 formatted strings

c54c259

pratik60 reviewed Jan 11, 2018

View reviewed changes

typo

21376c9

andrykonchin reviewed Jan 11, 2018

View reviewed changes

pboling requested changes Jan 11, 2018

View reviewed changes

william101 force-pushed the william/add_option_to_store_datetimes_as_strings branch from d1d558d to 7963591 Compare January 12, 2018 10:42

[pr comments] - Add field options to convert datetime to iso-8601 for…

a85fc93

…matted strings

william101 force-pushed the william/add_option_to_store_datetimes_as_strings branch from 7963591 to a85fc93 Compare January 12, 2018 10:43

andrykonchin reviewed Jan 12, 2018

View reviewed changes

rename option to store_as_native_string

f5282a6

pboling reviewed Jan 15, 2018

View reviewed changes

andrykonchin approved these changes Jan 15, 2018

View reviewed changes

pboling merged commit 451ff9a into Dynamoid:master Jan 16, 2018

william101 deleted the william/add_option_to_store_datetimes_as_strings branch January 17, 2018 10:44

andrykonchin mentioned this pull request Feb 4, 2018

Store dates as ISO-8601 formatted strings #228

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to store datetimes as ISO-8601 formatted strings #223

Add option to store datetimes as ISO-8601 formatted strings #223

william101 commented Jan 11, 2018

pratik60 Jan 11, 2018

pratik60 Jan 11, 2018

william101 Jan 11, 2018

coveralls commented Jan 11, 2018 •

edited

coveralls commented Jan 11, 2018

coveralls commented Jan 11, 2018 •

edited

andrykonchin Jan 11, 2018

andrykonchin Jan 11, 2018

andrykonchin Jan 11, 2018

pboling Jan 11, 2018

andrykonchin Jan 11, 2018

pboling Jan 11, 2018

andrykonchin Jan 11, 2018

andrykonchin Jan 11, 2018

pboling Jan 11, 2018

andrykonchin Jan 11, 2018

pboling left a comment

coveralls commented Jan 12, 2018 •

edited

andrykonchin Jan 12, 2018 •

edited

andrykonchin Jan 12, 2018

pboling Jan 12, 2018

coveralls commented Jan 12, 2018 •

edited

coveralls commented Jan 15, 2018

coveralls commented Jan 15, 2018

coveralls commented Jan 15, 2018 •

edited

pboling left a comment

andrykonchin left a comment •

edited

Add option to store datetimes as ISO-8601 formatted strings #223

Add option to store datetimes as ISO-8601 formatted strings #223

Conversation

william101 commented Jan 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 11, 2018 • edited

coveralls commented Jan 11, 2018

coveralls commented Jan 11, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pboling left a comment

Choose a reason for hiding this comment

coveralls commented Jan 12, 2018 • edited

andrykonchin Jan 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 12, 2018 • edited

coveralls commented Jan 15, 2018

coveralls commented Jan 15, 2018

coveralls commented Jan 15, 2018 • edited

pboling left a comment

Choose a reason for hiding this comment

andrykonchin left a comment • edited

Choose a reason for hiding this comment

coveralls commented Jan 11, 2018 •

edited

coveralls commented Jan 11, 2018 •

edited

coveralls commented Jan 12, 2018 •

edited

andrykonchin Jan 12, 2018 •

edited

coveralls commented Jan 12, 2018 •

edited

coveralls commented Jan 15, 2018 •

edited

andrykonchin left a comment •

edited