Skip to content

Conversation

eileencodes
Copy link
Member

Active Record already has the ability to guess the foreign key. With query constraints we can use the information from the owner class and that foreign key to infer what the query constraints should be for an association. This change improves the ergonomics of the query constraints feature by making it so that you only have to define the query constraints on the top level model and skip defining it on the association. There are a few caveats to this as of this PR:

  • If the query constraints on the parent are greater than 2, Rails can't derive the association constraints
  • If the query constraints on the parent don't include the primary key, Rails can't derive the association constraints.
  • I have not implemented support for CPK yet as it's more complex and maybe not possible since we don't know which key to split on and use for the foreign association.

Prior to this change you would need to do the following to use query constraints:

class Post
  query_constraints :blog_id, :id

  has_many :comments, query_constraints: [:blog_id, :blog_post_id]
end

class Comment
  query_constraints :blog_id, :id

  belongs_to :blog, query_constraints: [:blog_id, :blog_post_id]
end

After this change, the associations don't require you to set the query constraints, Active Record will generate [:blog_id, :blog_post_id] foreign key itself.

class Post
  query_constraints :blog_id, :id

  has_many :comments
end

class Comment
  query_constraints :blog_id, :id

  belongs_to :blog
end

@eileencodes eileencodes force-pushed the derive-foreign-key-query-constraints branch from 0f60ce6 to 109b962 Compare August 30, 2023 16:14
for the #{klass} model. The query constraints on #{klass} are
#{primary_query_constraints} and the foreign key is #{foreign_key}.
Do you need to explicitly set the query constraints on the association?
MSG
Copy link
Member Author

Choose a reason for hiding this comment

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

There is no test for this case because it shouldn't happen. But if it does happen the error will give an indication that we can't infer the query constraints and they need to be set explicitly. I could return the foreign key passed in here instead but I'm not sure it would be correct so for now I'd rather raise an error.

@eileencodes eileencodes force-pushed the derive-foreign-key-query-constraints branch 2 times, most recently from ca0b93a to e86b500 Compare August 30, 2023 18:41
@eileencodes eileencodes force-pushed the derive-foreign-key-query-constraints branch from e86b500 to 0d9226b Compare August 30, 2023 20:45
-(options[:foreign_key]&.to_s || derive_foreign_key(infer_from_inverse_of: infer_from_inverse_of))
derived_fk = options[:foreign_key]&.to_s || derive_foreign_key(infer_from_inverse_of: infer_from_inverse_of)

if active_record.has_query_constraints?
Copy link
Contributor

Choose a reason for hiding this comment

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

We should allow applications to override primary_key: configuration and disable automatic foreign key resolution as otherwise a setup like:

class Order < ActiveRecord::Base
   self.primary_key = :id
   query_constraints :shop_id, :id
end

class LineItem < ActiveRecord::Base
  belongs_to :order, primary_key: :id, foreign_key: :order_id
  # and
  #  belongs_to :order, primary_key: :id
end

will be broken since we will always try to append/prepend shop_id to the foreign key

Suggested change
if active_record.has_query_constraints?
if active_record.has_query_constraints? && !options[:primary_key]

Copy link
Member Author

@eileencodes eileencodes Aug 31, 2023

Choose a reason for hiding this comment

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

I can't make this change to add the primary key check without breaking the existing tests. I originally had it so if you set an explicit foreign key we don't generate a query constraint but then the async destroy tests were failing because it couldn't figure out the query constraints because the derived foreign key was wrong. I think setting an explicit foreign key should always override inferred query constraints - I'll make this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think setting an explicit foreign key should always override query constraints

That works 👍

Active Record already has the ability to guess the foreign key. With
query constraints we can use the information from the owner class and
that foreign key to infer what the query constraints should be for an
association. This change improves the ergonomics of the query
constraints feature by making it so that you only have to define the
query constraints on the top level model and skip defining it on the
association. There are a few caveats to this as of this PR:

- If the query constraints on the parent are greater than 2, Rails can't
derive the association constraints
- If the query constraints on the parent don't include the primary key,
Rails can't derive the association constraints.
- If the association has an explicit `foreign_key` or
`query_constraints` option set, Active Record won't infer the key.
- I have not implemented support for CPK yet as it's more complex and
maybe not possible since we don't know which key to split on and use
for the foreign association.

Prior to this change you would need to do the following to use query
constraints:

```ruby
class Post
  query_constraints :blog_id, :id

  has_many :comments, query_constraints: [:blog_id, :blog_post_id]
end

class Comment
  query_constraints :blog_id, :id

  belongs_to :blog, query_constraints: [:blog_id, :blog_post_id]
end
```

After this change, the associations don't require you to set the query
constraints, Active Record will generate `[:blog_id, :blog_post_id]`
foreign key itself.

```ruby
class Post
  query_constraints :blog_id, :id

  has_many :comments
end

class Comment
  query_constraints :blog_id, :id

  belongs_to :blog
end
```
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.

2 participants