PostgreSQL, fix type_map lookup when defining ModelSchema [Thibaud Guillaume-Gentil & Yves Senn]

senny · thibaudgg · senny · commit f79b5e9a41ae · 2020-02-25T11:19:41.000+01:00
After upgrading to Rails 6 we have encountered sporadic type casting issues in our staging system. Our production installation did not exhibit the same behavior. The error behavior looked something like this: ``` TypeError: can't quote RGeo::Geographic::SphericalPointImpl [GEM_ROOT]/gems/activerecord-6.0.2.1/lib/active_record/connection_adapters/abstract/quoting.rb:231:in `_quote' [GEM_ROOT]/gems/activerecord-6.0.2.1/lib/active_record/connection_adapters/postgresql/quoting.rb:144:in `_quote' [GEM_ROOT]/gems/activerecord-6.0.2.1/lib/active_record/connection_adapters/abstract/quoting.rb:18:in `quote' [GEM_ROOT]/gems/activerecord-postgis-adapter-6.0.0/lib/active_record/connection_adapters/postgis_adapter.rb:105:in `quote' [GEM_ROOT]/gems/activerecord-6.0.2.1/lib/arel/collectors/substitute_binds.rb:17:in `add_bind' ... ``` or this: ``` NoMethodError: undefined method `factory' for "0101000020E61000006094A0BFD023014089601C5C3AB64440":String [GEM_ROOT]/gems/rgeo-1.1.1/lib/rgeo/feature/types.rb:160:in `cast' [GEM_ROOT]/gems/rgeo-1.1.1/lib/rgeo/geographic/spherical_feature_methods.rb:31:in `distance' ... ``` Both indicate that data for PostGIS columns was not properly type casted. In the first case when writing/querying and in the second when reading. Through instrumentation on the staging system, we were able to replicate the flow of events that caused our model to enter this state. 1. PostGIS ActiveRecord Adapter We've identified a [bug in the `activerecord-postgis-adapter`](rgeo/activerecord-postgis-adapter#309) that results in a partially initialized `type_map`. This is a prerequisite for the bug outlined in this ticket to ocurr. Note that the bug is not exclusive to PostGIS, it could happen with other custom defined types in PostgreSQL as well. Important is that the `type_map` does not include a mappin for that OID type after [`initialize_type_map`](https://github.com/rails/rails/blob/8d57cb39a88787bb6cfb7e1c481556ef6d8ede7a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb#L526) ran. 2. `type_map` lookups There are two instances where the PostgreSQL adapter uses the `type_map`: - (A) [`get_oid_type`](https://github.com/rails/rails/blob/master/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb#L514-L516) - (B) [`lookup_cast_type_from_column`](https://github.com/rails/rails/blob/15748f6a052d4df68b6acf66456c181b42d9fe25/activerecord/lib/active_record/connection_adapters/postgresql/quoting.rb#L77-L79) What's important to note here is that `get_oid_type` will attmpt to add missing OID mappings, while `lookup_cast_type_from_column` assums the `type_map` is complete and does a simple access. 3. Attribute definitions in ModelSchema. The attributes of a model are defined on first access in [`load_schema!`](https://github.com/rails/rails/blob/master/activerecord/lib/active_record/model_schema.rb#L491-L498) using `lookup_cast_type_from_column` (the `type_map` lookup that does not try to load missing OID mappings). However, in most scenarios, this does not cause an issue because of the preceeding line [`connection.schema_cache.columns_hash(table_name)`](https://github.com/rails/rails/blob/master/activerecord/lib/active_record/model_schema.rb#L490). Fetching the table structure will go through `get_oid_type` and add the missing OID just before `lookup_cast_type_from_column` happens. 4. Shared data The issue only occurs because some information is shared between threads, while other data is local to a connection: - `type_map`: Local to a connection. Fresh connections can lack certain OID mappings. - `schema_cache`: Per connection pool. - `ModelSchema`: Global. 5. Flow of events that cause the problem Given the behavior outlined above, there is a sequence of events that can cause the ModelSchema to define an attribute with `ActiveModel::Type::Value` (default fallback) instead of a specific OID type. The sequence that we have seen goes something like this: 1. Connection A caches the `columns_hash` for table `examples` in the `schema_cache`. It does not yet define the `ModelSchema` for Example. Loading the column definitions goes through `get_oid_type` and registeres the missing OID. 2. Connection B (a fresh connection) defines the `ModelSchema` for `Example`. Since `schema_cache` returns cached data for `examples` (`get_oid_type` is not used). The `type_map` lookup in `lookup_cast_type_from_column` will not find the OID and use `ActiveModel::Type::Value` instead. It's very likely that Connection B later goes through `get_oid_type` and completes it's `type_map`. However, since the attribute definitions are shared for all threads. This process is now in a corrupted state with invalid type casting. 6. The patch Making sure that `lookup_cast_type_from_column` has the same semantics as `get_oid_type` will prevent `type_map` lookups of OIDs that could be loaded by Rails. It's worth noting that in our case, the main issue was that the activerecord-postgis-adapter left the `type_map` only partially initialized. Fixing that, will shadow the potential conflict outlined in this ticket. Co-authored-by: Thibaud Guillaume-Gentil <thibaud@thibaud.gg>
diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
@@ -1,3 +1,7 @@
+*   Fix a bug in the PostgreSQL adapter that could cause models to have invalid type casting behavior for custom PostgreSQL types.
+
+    *Thibaud Guillaume-Gentil*, *Yves Senn*
+
 *   Add support for horizontal sharding to `connects_to` and `connected_to`.
 
     Applications can now connect to multiple shards and switch between their shards in an application. Note that the shard swapping is still a manual process as this change does not include an API for automatic shard swapping.
diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/quoting.rb b/activerecord/lib/active_record/connection_adapters/postgresql/quoting.rb
@@ -75,7 +75,7 @@ def quote_default_expression(value, column) # :nodoc:
         end
 
         def lookup_cast_type_from_column(column) # :nodoc:
-          type_map.lookup(column.oid, column.fmod, column.sql_type)
+          get_oid_type(column.oid, column.fmod, column.name, column.sql_type || "")
         end
 
         def column_name_matcher
