Skip to content

Conversation

@Connorhd
Copy link
Contributor

Extending ActiveRecord::Type::Time means that string values get incorrectly coerced to have a date of 2000-01-01. See https://github.com/rails/rails/blob/main/activemodel/lib/active_model/type/time.rb#L14. This is because Time in activerecord is intended to store just the time value (and maps to a TIME type in other databases), unlike Time generally in ruby that represents a date and time.

Extending ActiveRecord::Type::Time means that string values get incorrectly coerced to have a date of 2000-01-01. See https://github.com/rails/rails/blob/main/activemodel/lib/active_model/type/time.rb#L14. This is because Time in activerecord is intended to store just the time value (and maps to a TIME type in other databases), unlike Time generally in ruby that represents a date and time.x
@Connorhd Connorhd requested review from a team, ansh0l, olavloite and rahul2393 as code owners February 10, 2023 00:40
@google-cla
Copy link

google-cla bot commented Feb 10, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@product-auto-label product-auto-label bot added the api: spanner Issues related to the googleapis/ruby-spanner-activerecord API. label Feb 10, 2023
@olavloite
Copy link
Collaborator

@Connorhd I agree with you that it would probably have been a better mapping, but this choice was made 3 years ago, and I'm afraid that changing it now could cause unwanted breakages in existing applications. Are you running into specific problems other than the default date of '2000-01-01' when using string values with this type?

@Connorhd
Copy link
Contributor Author

Yeah, that's the only problem I've seen so far.

Definitely understand the hesitation and I'm not sure how to be confident there are not subtle issues caused that this. But, not making this change also seems quite risky, it seems pretty reasonable for activemodel (or anything built on top of it) to do strange things to the date component of these values as the type clearly only represents a time value.

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

Labels

api: spanner Issues related to the googleapis/ruby-spanner-activerecord API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants