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

Infer types for active record fixtures #1871

Merged
merged 8 commits into from
Apr 22, 2024

Conversation

TobiasBales
Copy link
Contributor

@TobiasBales TobiasBales commented Apr 15, 2024

Motivation

It has been bugging me some time that fixtures are T.untyped.
After a conversation today with @vinistock we realized that method overloading might work - and lo and behold - it does work.

The generated signatures return Model for models(:one) and T::Array[Model] for models(:one, :two) by overloading it with nil as the rest arg type, hat tip to @vinistock for the idea.

Implementation

The signature of the fixture methods changed from users(*fixture_names) to users(fixture_name, *other_fixutres) in order to allow overloading based on the type of the rest args (e.g. nil or T.any(String, Symbol).

The mapping for fixture method (e.g. users) to model name (e.g. User) happens by creating a lookup table (once) for a mapping from underscore name to name. All descendants of ActiveRecord::Base go into the lookup table.
This is required since there are potential conflicts with the fixture method name between FooBar and Foo::Bar which both map to foo_bars.
By building this lookup table once we can map this reasonably efficiently and correctly.

Should no matching model be found it falls back to T.untyped.
Do we have some way of collecting metrics if/how often this happens?

I added an ad-hoc implementation for turning the Blog::Post into blog_posts - I would assume there is something like that already but I did not find it - any pointers would be more than welcome!

Tests

I modified the class documentation and tests to account for the changed signature and added the correct return types.
I also modified one test case to have a namespaces model Post -> Blog::Post to ensure that namespaced models are handled correctly and I also added a test that ensures the fallback to T.untyped happens as expected.

I also ran this on a personal project of mine (rails app with ~10k lines of ruby) and it worked as expected and srb tc still passed (meaning I used the fixtures as expected but also, at least for my case, it generated reasonable rbis)

@TobiasBales TobiasBales requested a review from a team as a code owner April 15, 2024 17:34
@TobiasBales TobiasBales added the enhancement New feature or request label Apr 15, 2024
@TobiasBales TobiasBales force-pushed the better-fixtures-types branch 2 times, most recently from d0b52d7 to 181ece1 Compare April 15, 2024 17:45
@bdewater
Copy link
Contributor

bdewater commented Apr 15, 2024

Should no matching model be found it falls back to T.untyped.

I was poking at creating this compiler as well and noticed it is possible to specify a model class in the fixture file itself. Could try ActiveRecord::FixtureSet::File.open to read the YAML file and call #model_class on it before falling back to T.untyped.

@TobiasBales
Copy link
Contributor Author

Should no matching model be found it falls back to T.untyped.

I was poking at this one as well and noticed it is possible to specify a model class in the fixture file itself. Could try ActiveRecord::FixtureSet::File.open to read the YAML file and call #model_class on it before falling back to T.untyped.

Good point, will have a look at that, probably tomorrow though.
I would use that information, fallback to the class name mapping and then fall back to T.untyped - does that make sense?

@bdewater
Copy link
Contributor

bdewater commented Apr 15, 2024

Yep it does and matches the logic here for the in-file declaration to take precedent: https://github.com/rails/rails/blob/6d3fd5b98cc1c700fe9733521118abc7540546e7/activerecord/lib/active_record/fixtures.rb#L791-L793

@TobiasBales TobiasBales force-pushed the better-fixtures-types branch 4 times, most recently from 67d53f1 to c4956af Compare April 16, 2024 09:00
@TobiasBales
Copy link
Contributor Author

TobiasBales commented Apr 16, 2024

@bdewater @Morriar I managed to get it work (in the way I understand it to work).

There are a couple of questions:
The whole fixture_file_class_mapping method is a bit all over the place but right now I am not sure what it would like in a nice way - very open to suggestions here.

Does this cover all needed cases?

Does it seem reasonable to require ActiveRecord::FixtureSet to be defined? Otherwise we'd need to reimplement a bunch of things which I think we should rather not

I will try to run this against Core in a bit to see if this causes issues (thanks @Morriar for doing the first run!) and report back. The first run issues seemed unrelated but I have not dug a whole lot and now I want to make sure it actually matches expectations now and works.

@TobiasBales TobiasBales force-pushed the better-fixtures-types branch 2 times, most recently from 389170c to 6185195 Compare April 16, 2024 09:22
@TobiasBales
Copy link
Contributor Author

The tests are now failing on rails 7.0 due to missing #fixture_sets methods.
I will see if I can find something based on what we do for rails 7.0 to get the same data.

Should that turn out to not be possible - thoughts on just supporting this for rails 7.1+ and going with T.untyped for 7.0?
It might be worth the reduced complexity if it is an acceptable trade

@TobiasBales
Copy link
Contributor Author

The tests are now failing on rails 7.0 due to missing #fixture_sets methods. I will see if I can find something based on what we do for rails 7.0 to get the same data.

Should that turn out to not be possible - thoughts on just supporting this for rails 7.1+ and going with T.untyped for 7.0? It might be worth the reduced complexity if it is an acceptable trade

I implemented a fallback to descendants of ActiveRecord::Base should prior resolution attempts not yield anything and made the lookup via fixutre_sets dependent on the method being present

Copy link
Collaborator

@Morriar Morriar left a comment

Choose a reason for hiding this comment

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

One minor suggestion but it looks good to me 👍

lib/tapioca/dsl/compilers/active_record_fixtures.rb Outdated Show resolved Hide resolved
@TobiasBales TobiasBales force-pushed the better-fixtures-types branch 5 times, most recently from 3dd652f to 624c7ac Compare April 19, 2024 08:08
This is to discriminate between single argument calls which return a
single model, e.g. User and multi argument calls which return an array
of the model, e.g. T::Array[User].

This is the precursor to actually typing the return values since
otherwise we would not be able to type it correctly.
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