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

Make ActiveRecord#find return value typed #1089

Merged
merged 2 commits into from
Aug 4, 2022

Conversation

gaborszakacs
Copy link
Contributor

@gaborszakacs gaborszakacs commented Jul 30, 2022

Motivation

Currently the return value of e.g. Post#find gets generated as untyped. We could catch some subtle bugs if it was typed.

Imagine a novice Rails dev who's not familiar with which finder method would return nil and which would raise exception if no record was found. Sorbet could warn e.g. if we tried to nil check on the return value of a find.

image

Implementation

I first thought that find always returns with a model, but it turned out it can be used to query multiple record at once so I went with a union type: either the model or an array of the models is returned.

(Some more background: rails-sorbet chose to support only the one model retrieval for find and introduced a new method for the multiple record use case.)

Tests

I've modified the existing test for the generated signature for #find.

@gaborszakacs gaborszakacs requested a review from a team as a code owner July 30, 2022 16:01
@st0012
Copy link
Member

st0012 commented Aug 4, 2022

@gaborszakacs Thank you for the PR!

@st0012 st0012 merged commit 9e46b80 into Shopify:main Aug 4, 2022
@gaborszakacs gaborszakacs deleted the typed-active-record-find branch August 4, 2022 11:12
@paracycle
Copy link
Member

@gaborszakacs Thanks for the contribution, but I am afraid this is not the correct type for what find returns. find returns a Model instance if supplied a single argument, or (exclusive or), it returns an array of Model instances if supplied more than one argument. So the return type cannot be modelled by a union type, it is a discriminated union (or a method overload) which Sorbet does not support. So T.untyped is the best we can do, given those limitations.

Revert PR here: #1101

@gaborszakacs
Copy link
Contributor Author

@paracycle Yes, you're right, sorry for pushing this in the wrong direction, I'm kind of new to the Sorbet type system.

One question, I guess we don't want to "force" the single argument use case as sorbet-rails does, but probably it's a good idea to mention in the documentation that find is not type-safe and suggest using find_by!(id: ...) instead. I suppose find is quite frequent in Rails codebases, probably worth pointing this out explicitly. Wdyt? I'm happy to open a PR, if you agree.

@paracycle paracycle added the invalid This doesn't seem right label Aug 22, 2022
@shopify-shipit shopify-shipit bot temporarily deployed to production August 31, 2022 14:40 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid This doesn't seem right
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants