Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide typing for serialized ActiveRecord columns #1194

Merged
merged 4 commits into from
Oct 3, 2022

Conversation

robthornton
Copy link
Contributor

Motivation

ActiveRecord columns may require being serialized. Valid types for this attribute are: Object (the default), Array, Hash, JSON, or a custom class that encodes/decodes the value.

See: https://api.rubyonrails.org/classes/ActiveRecord/AttributeMethods/Serialization/ClassMethods.html

Implementation

Typing for JSON matches the GraphQL typing and the JSON spec. No attempt was made to infer the subtypes of Arrays or Hashes.

Tests

@robthornton robthornton added the enhancement New feature or request label Sep 29, 2022
@robthornton robthornton requested a review from a team as a code owner September 29, 2022 16:33
ActiveRecord columns may require being serialized. Valid types for this
attribute are: Object (the default), Array, Hash, JSON, or a custom
class that encodes/decodes the value.

See: https://api.rubyonrails.org/classes/ActiveRecord/AttributeMethods/Serialization/ClassMethods.html

Typing for JSON matches the GraphQL typing and the JSON spec. No attempt
was made to infer the subtypes of Arrays or Hashes.
Comment on lines +127 to +139
case column_type.coder
when ActiveRecord::Coders::YAMLColumn
case column_type.coder.object_class
when Array.singleton_class
"T::Array[T.untyped]"
when Hash.singleton_class
"T::Hash[T.untyped, T.untyped]"
else
"T.untyped"
end
else
"T.untyped"
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I understand serialization formats only give guarantees for Array and Hash types, and everything else should just be T.untyped:

serialize attr_name [, class_name_or_coder]

                     |                           |  database storage   |
class_name_or_coder  | attribute read/write type | serialized | NULL   |
---------------------+---------------------------+------------+--------+
  <not given>        | any value that supports   |    YAML    |        |
                     |   .to_yaml                |            |        |
                     |                           |            |        |
Array                | Array **                  |    YAML    |  []    |
                     |                           |            |        |
Hash                 | Hash **                   |    YAML    |  {}    |
                     |                           |            |        |
JSON                 | any value that supports   |    JSON    |        |
                     |   .to_json                |            |        |
                     |                           |            |        |
<custom coder class> | any value supported by    |   custom   | custom |
                     | the custom coder class    |            |        |

Representing JSON as a Hash type or generic YAML as an Object type would not be correct, since they serialize anything that responds to to_json and to_yaml respectively.

Copy link
Member

@rafaelfranca rafaelfranca Sep 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

class_name_or_coder not given also checks if the object inherits from Object, so BasicObject isn't allowed. The signature of T.nilable(Object) seems correct for that case. The JSON case should be really untyped.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we type that return value as T.nilable(Object), it will be correct but not very ergonomic:

extend T::Sig

class Foo
  def foo
  end
end

sig { returns(Object) }
def serialized_field
  Foo.new
end

f = serialized_field
f.foo # Error: Method foo does not exist on Object

Since, yes, the return value technically "is a" Object but most of the time you want to access specific functionality on it.

Because we don't know what type was stored in the DB field, I'd rather we type it as T.untyped.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, good point. Agree.

@@ -356,6 +356,61 @@ def string_enum_column=(value); end
assert_includes(output, expected)
end

it "generates correct types for serialized columns" do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved this out to separate test case, since the other one was getting a little busy.

@paracycle paracycle merged commit 7470067 into main Oct 3, 2022
@paracycle paracycle deleted the serialized-column-typing branch October 3, 2022 15:22
@shopify-shipit shopify-shipit bot temporarily deployed to production November 10, 2022 17:34 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants