Skip to content

Allow specifying columns to use in ActiveRecord::Base object queries #46331

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

Merged

Conversation

nvasilevski
Copy link
Contributor

Description

This PR is a continuation of the PR that made a small refactoring to prepare Rails to support composite primary keys in the future - #41427
This PR renames _primary_key_constraints_hash private method to be less specific and adds a capability to configure it using a public ActiveRecord::Base.query_constraints macro.

Rationale

This proposed capability is fundamental for supporting composite primary keys in Rails and utilizing tenant based sharding. A Tenant based sharding design requires all queries to a SQL database to include the sharding key. For example tables sharded by user_id will always need to be queried by user_id value to make sure that the query reaches the correct shard. As a real-world example of a sharding solution we can name Vitess, however, we are building a solution that is generic in a similar way with default_scope(all_queries: true) and will enable applications to query by multiple default constraints regardless of underlying technology.

Implementation details

Renames _primary_key_constraints_hash to _query_constraints_hash to avoid being too specific as the feature doesn’t necessarily have to represent a primary key.

Defines a new QueryConstraints module with the definition of the query_constraints macro and query_constraints_list class attribute which stores the configuration. At first class attribute and macro definition were placed in ActiveRecord::Core but a separate module was chosen mainly to avoid cluttering core with documentation for query_constraints
QueryConstraints module is included directly into Persistence as Persistence relies on the class attribute to be defined.
_query_constraints_hash uses attribute_in_database to fetch the value to unlock the ability to update parts of the composite primary key like

developer.update!(company_id: 2)
# => UPDATE "developers" SET "company_id" = 2 WHERE "developers"."company_id" = 1 AND "developers"."id" = 1

Naming

The main use-case of this capability is to define a “virtual” composite primary key like [:shard_id, :id] or [:tenant_id, :id] for an Active Record model. However we would like to avoid explicitly mentioning the term “primary key” as the capability in the current implementation is generic and only provides foundation to support composite primary keys.
At the same time, name query_constraints may be too generic but luckily it’s short enough for us to perhaps consider specializing it a little bit with an additional word like query_by_attributes
Please do not hesitate to provide your naming suggestions!

Isn’t it similar to default_scope(all_queries: true) {}
Even though the proposed feature and all_queries capabilities share some similarities they are not quite the same. While default_scope(all_queries: true) applies to all queries, query_constraints changes behavior issued by a single Active Record object. It also gets column values directly from the object while default_scope needs either a hardcoded or globally accessible value like default_scope { company_id: Current.company_id }

Vision

I also wanted to briefly share a bigger picture for this foundational feature and upcoming additions we would like to propose soon:
Rails starts to implicitly configure query_constraints to match the primary_key
Add capability to build a “row-constructor” using an Active Record relation. For example Model.where([:tenant_id, :id] => [[1,2], [1,3], [2,4]]) Similar to #36003
Associations should respect the query_constraints and by implication composite primary key configuration. For example given a blogging platform with models Blog, BlogPost, Comment sharded by blog_id, query like comment.blog_post should load blogpost using whole query_constraints clause

@eileencodes eileencodes self-assigned this Oct 25, 2022
@eileencodes eileencodes added this to the 7.1.0 milestone Oct 25, 2022
@nvasilevski nvasilevski force-pushed the introduce-AR-query-by-configuration branch 2 times, most recently from e9e5e14 to 959dfa8 Compare October 25, 2022 15:51
def _primary_key_constraints_hash
{ @primary_key => id_in_database }
def _query_constraints_hash
return { @primary_key => id_in_database } unless self.class.query_constraints_list
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line is to keep backwards compatibility, eventually we expect it to go away once Rails starts deriving query_constraints_list from the database schema which means all existing models should get ["id"] in the list "by convention"

# developer.delete
# # => DELETE FROM "developers" WHERE "developers"."company_id" = 1 AND "developers"."id" = 1
#
# developer.reload
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a note that reload doesn't work as intended at the moment as it wasn't relying on _primary_key_constraints_hash and a follow-up PR incoming to fix this

@nvasilevski nvasilevski force-pushed the introduce-AR-query-by-configuration branch 2 times, most recently from b3c4824 to 13dcf56 Compare October 31, 2022 21:27
@simi
Copy link
Contributor

simi commented Oct 31, 2022

🤔 I was thinking for a while about the naming. query_constraints seems to be related to all queries and as mentioned is too generic. What about something more closer to the actual usage (-> persistence) aka persistence_constraints?

Renames `_primary_key_constraints_hash` private method to avoid implying
that it strictly represents a constraint based on the model primary key

Allows columns list used in query constraints to be configurable using
`ActiveRecord::Base.query_constraints` macro
@nvasilevski nvasilevski force-pushed the introduce-AR-query-by-configuration branch from 13dcf56 to 415e6b6 Compare November 1, 2022 15:53
@nvasilevski
Copy link
Contributor Author

nvasilevski commented Nov 1, 2022

What about something more closer to the actual usage (-> persistence) aka persistence_constraints?

I can see where this suggestion is coming from and it's fair if scoped to this particular PR as currently it only changes persistence behavior, however, I'm afraid that calling it persistence will limit our long-term vision for the feature which includes making sure that .reload respects the config, associations like comment.blog_post respecting query constraints of the Blog model and presumably, mainly for the composite primary keys use-case, we may even need .find method to respect a different signature like

class Vehicle < ActiveRecord::Base
   query_constraints :make, :model
end

Vehicle.find(["Toyota", "Corolla"])

For this reason we would like to stick to query or querying term or any other term but as long as it implies "querying behavior" in general

# frozen_string_literal: true

module ActiveRecord
module QueryConstraints
Copy link
Member

Choose a reason for hiding this comment

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

Why this module with 1 method and included in one single class exist?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After finishing implementation of all anticipated features we will most likely inline the code into ActiveRecord::Persistence

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants