Shopify / Shopify/tapioca

Suggestion: Implement `<Model>::Persisted` opaque type to solve column nilability confusion

Open
#1,667 19 comments 13 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

help-wanted
Dominant language
Ruby
Stars
873
Forks
164
Avg merge
4d 27m
Merged PRs (30d)
9

Description

The Problem

The tapioca compiler for ActiveRecord models makes most columns nilable, ignoring database constraints.
This is correct on paper, because when you call Model.new none of those would matter, until you eventually try to persist the record in the database.

In practice though, this is a common pitfall and cause for confusion, especially for those just getting started with Sorbet.

This is not a new problem, it has in fact been discussed in the past both here and in the Sorbet Slack workspace:

Existing Solutions

There are currently two possible paths to address this:

  1. The most common one is to just deal with this in the code and use T.must whenever Sorbet complains about nilability and we know the record has been persisted, so the column cannot be nil.
  2. The other, less known and currently undocumented solution is to define a StrongTypeGeneration module in the model to tell the compiler to generate the class following the database constraints (as explained in this comment). This will make the class work in most cases, but it won't allow you to call a non-nullable column on a newly instantiated model without throwing an error (e.g. Model.new.non_nilable_column)

The two solution seem mutually exclusive and present pros/cons to different cases, so developers will always need to weight in with additional T.must or T.unsafe to "correct" Sorbet misunderstandings.
Ideally, we'd like to teach sorbet exactly what to expect.

A proposal for an alternative solution

I recently stumbled across the concept of "opaque types" thanks to an article from Jez.

After reading the article, I began wondering if it wouldn't be possible to change the compiler so that on top of the Model class, a Model::Persisted or PersistedModel class would be generated as well.
The two classes would be identical and they'd only differ by the fact the the latter will have stricter constraints on columns, reflecting database constraints.

Then, methods signatures can be changed to return one type or the other. For example:

# app/models/user.rb
class User < ActiveRecord::Base
  # == Schema Information
  #
  # Table name: users
  #
  #  id                          :integer          not null, primary key
  #  email                       :string           default(""), not null
  #  encrypted_password          :string           default(""), not null
  #  created_at                  :datetime         not null
  #  updated_at                  :datetime         not null
end

# user.rbi
module CommonMethods
  sig { returns(PersistedUser) }
  def save; end

  sig { returns(PersistedUser) }
  def find_by; end

  sig { returns(PersistedUser) }
  def update; end
end

class User
  include CommonMethods

  sig { returns(T.nilable(Integer)) }
  def id; end

  sig { returns(T.nilable(String)) }
  def email; end

  ...
end

class PersistedUser # or User::Persisted ?
  include CommonMethods

  sig { returns(Integer) }
  def id; end

  sig { returns(String) }
  def email; end

  ...
end

cc @rafaelfranca @Morriar @KaanOzkan based on your past contributions to the topic 🙏

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Review the prior discussions in issues 600, 1342, and 1642, then compare the example app/models/user.rb and user.rbi shapes in this proposal with Tapioca's ActiveRecord RBI generation. The work is complete when the compiler can generate a persisted model type with stricter column nilability and the relevant model method signatures can use it without the existing workarounds.

Written by the indexing model from the issue text.

Assessment

Tech stack
rails, ruby
Domain
backend, tooling
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.