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

More accurately type ActiveRecordRelations calculation methods #1408

Merged
merged 2 commits into from
Mar 13, 2024

Conversation

jeffcarbs
Copy link
Contributor

Motivation

Currently the calculation methods like sum and count return T.untyped. For some methods we cannot know the type (e.g. minimum) but for others we at least know it's numeric (e.g. sum) and for count we know it's an Integer.

However, I'm assuming these are untyped because if they're used with group the return value is actually a hash where the keys cannot be typed but the values will be the types mentioned above. This PR updates the ActiveRecordRelations compiler to type these correctly.

Implementation

I first updated the return values of the calculation methods where possible. Then I largely copied from the code around PrivateRelationWhereChain to add a PrivateRelationGroupChain module which defines the calculation methods with the hash return values.

Some callouts:

  • Unlike "WhereChain", there's no such analogous GroupChain runtime class available so this is solely made for RBI. From looking at the source it looks like when a query uses group some metadata is added to it.
  • Technically a group anywhere in the query chain produces this behavior but to avoid needing to re-type every query method inside this module we make a simplifying assumption that the calculation method is called immediately after the group (e.g. group().count and not group().where().count). The one exception is group().having().count which is fairly idiomatic so that gets handled without breaking the chain.

Tests

Updated the existing specs.

@jeffcarbs jeffcarbs requested a review from a team as a code owner February 22, 2023 04:46
@KaanOzkan KaanOzkan requested review from a team, egiurleo and KaanOzkan and removed request for a team March 13, 2024 15:44
@KaanOzkan
Copy link
Contributor

@jeffcarbs sorry for the wait looks like the review automation failed. Could you rebase on main? Thanks.

@jeffcarbs jeffcarbs force-pushed the relation-calculation-methods branch from ff9734a to afef640 Compare March 13, 2024 20:18
@jeffcarbs
Copy link
Contributor Author

@KaanOzkan - rebased and pushed up a new build.

@jeffcarbs
Copy link
Contributor Author

jeffcarbs commented Mar 13, 2024

I think you need to add a label, enhancement probably makes the most sense. I would add it but I don't think I have permission.

Also it looks like the build are failing on main:

Screenshot 2024-03-13 at 2 47 47 PM

@KaanOzkan KaanOzkan added the enhancement New feature or request label Mar 13, 2024
@KaanOzkan
Copy link
Contributor

Thanks I'll now take a look. And yes the build failures seem to be because we remove Gemfile.lock and bump google-protobuf during CI #1819

Copy link
Contributor

@KaanOzkan KaanOzkan left a comment

Choose a reason for hiding this comment

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

This LGTM from my limited understanding of group and calculation methods. Tested on a small Rails app as well.

@KaanOzkan KaanOzkan merged commit cec60f7 into Shopify:main Mar 13, 2024
13 of 19 checks passed
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.

None yet

2 participants