Skip to content

Conversation

@metaskills
Copy link
Member

@metaskills metaskills commented Aug 20, 2016

Creating or updating records resets the attributes by mapping the hash using ActiveRecord's Attribute#forgetting_assignment method. This method essentiall mimics the reload by setting the attributes to FromDatabase using the FromUser value "for the database".

rails/rails@07723c23

This is a problem for us because the value_for_database for date time objects are strings that can not be re-parsed due to SQL Server's time formats. For example, a date of August 18th 2016 would be formatted as '08-19-2016' and fail deserialize:

ActiveModel::Type::Date.new.deserialize('08-19-2016') # => nil

Both ActiveModel's date and time types rescue Date.parse methods of new_date and new_time with nil. It was suggested that we use Data classes for our time types and we may explore that after this commit which essentially prepends a new forgetting_assignment method that checks the type. If it is a SQL Server date/time, it will just convert that value object to a FromDatabase using the existing value.

cc @sgrif @dpulliam

Creating or updating records resets the attributes by mapping the hash
using ActiveRecord's `Attribute#forgetting_assignment` method. This
method essentiall sets mimic the reload by setting the attributes to
FromDatabase using the FromUser value "for the database".

This is a problem for us because the `value_for_database` for datetime
objects are strings that can not be re-parsed due to SQL Server's time
formats. For example, a date of August 18th 2016 would be formatted as
'08-19-2016' and fail deserialize:

  ActiveModel::Type::Date.new.deserialize('08-19-2016') # => nil

Both ActiveModel's date and time types rescue `Date.parse` methods of
`new_date` and `new_time` with nil. It was suggested that we use
Data classes for our time types and we may explore that after this
commit which essentially prepends a new `forgetting_assignment` method
that checks the type. If it is a SQL Server date/time, it will just
convert that value object to a FromDatabase using the existing value.
@sgrif
Copy link

sgrif commented Aug 25, 2016

This patch would completely break dirty. You should just use a newtype wrapper in serialize and handle it in the adapter and deserialize

@metaskills
Copy link
Member Author

Documenting current pains of the alternate approach. No judgement, just sharing thoughts.

  • Relying on Date/Time string parsing in Rails/Ruby to properly parse the time formats sent to the Database requires us to do more work in deserialization to make this happen. A good example is our DateTimeOffset type which can not rely on ActiveModel's fallback_string_to_time.
  • It feels like we are comping what is sent to the Database in SQL to what we expect to get back from the database.
  • In general we are loosing the benefits - or being forced to write code - where TinyTDS does this work for us. It feels like ActiveRecord has a lot of responsibility here that the C extensions should really be doing.

@metaskills
Copy link
Member Author

  • fast_string_to_time does not support nanoseconds or zones.

@metaskills metaskills closed this Dec 9, 2016
@metaskills metaskills deleted the rails5-forgetting_assignment branch May 29, 2017 00:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants