Refactor the typecasting behavior of ActiveRecord columns #15134
Conversation
This removes all cases from `Column` where behavior was determined soley based on the SQL type string. Instead, we push this decision up to the Column's creation, and inject the object with knowledge of the type casting behavior of it. The API exposed was intended to be close to the PostgreSQL implementation for OID lookups. There's additional refactoring that could be done on this, especially in the PostgreSQL adapter and MySQL adapter, but this felt like a good place to cut it off, as the pull request was getting quite large.
|
Cool! Thank you for working on this. @senny since you are working on this area could you take a look on it? |
|
Just a few notes on the reasoning here: I went with the mapping structure that's used in order to make it easiest to extend in subclasses, and bring the API of the
|
|
@sgrif please don't add more stuff to this PR. It's already very big and will take a long time to review. I was working on the same code and sadly we now have a lot of duplicated work (my branch is not yet pushed). I try to look into this and hopefully apply the PR in separate parts and not in one go. |
|
@sgrif Thanks for moving this forward. It will clarify a lot ❤️ |
|
Not a problem, I don't plan to add any more, I had just spotted a minor issue. I apologize for the size, it would have been nice to break it up a bit more. |
|
@senny Let me know if there's any way I can help. I'd be happy to pair if it might help reduce the duplicated work. :) |
|
@sgrif actually I was thinking about that. Might be a good approach to keep the ball going. Can I reach you over GTalk/Hangout? If so, let me know over mail (found on my profile page). |
|
Sent you my contact info, you can reach me on hangouts from that address. |
| @@ -644,6 +626,15 @@ def valid_type?(type) | |||
|
|
|||
| protected | |||
|
|
|||
| def initialize_type_map(mapping) | |||
| super | |||
| mapping.alias_type(/tinyint\(1\)/i, 'boolean') if emulate_booleans | |||
There was a problem hiding this comment.
mapping code block-indented here but not in abstract adapter. should be consistent one way or another
|
@rafaelfranca @senny There's definitely parts of this that can be easily split into smaller PRs. I'm going to start doing that, now that there's context on what I'm working towards. |
|
👍 |
|
@sgrif 👍 thanks for splitting stuff up. Let's keep this PR open for reference but review the smaller pieces instead. |
Part of rails#15134. In order to perform typecasting polymorphically, we need to add another argument to the constructor. The order was chosen to match the `oid_type` on `PostgreSQLColumn`.
|
This has been merged. |
This removes all cases from
Columnwhere behavior was determined soleybased on the SQL type string. Instead, we push this decision up to the
Column's creation, and inject the object with knowledge of the type
casting behavior of it.
The API exposed was intended to be close to the PostgreSQL
implementation for OID lookups. There's additional refactoring that
could be done on this, especially in the PostgreSQL adapter and MySQL
adapter, but this felt like a good place to cut it off, as the pull
request was getting quite large.