Refactor query constraints feature set #47378
Conversation
| elsif query_constraints = record.class.query_constraints_list | ||
| query_constraints | ||
| else | ||
| :id |
There was a problem hiding this comment.
This :id is a reimplementation of the previous code here
I am surprised we were hardcoding :id like this and after we merge this I think we should consider getting this from elsewhere. I feel like there's an old bug here since it's not using primary key from the record or reflection.
There was a problem hiding this comment.
I had the same concern and even tried to switch to reflection.active_record_primary_key, which technically was supposed to be equal to calling record.id but it didn't work because of at least one test failing so I decided not to dig into this to avoid unnecessary breaking things - #47230 (comment)
Though design-wise I still see reflection to be a more appropriate source of the primary key columns instead of a hardcoded :id method call, which in Rails refers to model's @primary_key
| order_columns = implicit_order_column.nil? ? query_constraints_list : ([implicit_order_column] | query_constraints_list) | ||
| order(*order_columns.map { |column| table[column].asc }) | ||
| if order_values.empty? && (implicit_order_column || !query_constraints_list.nil? || primary_key) | ||
| order(*Array(_order_columns).map { |column| table[column].asc }) |
There was a problem hiding this comment.
It looks like _order_columns is always an array so wrap is not neccessary
| order(*Array(_order_columns).map { |column| table[column].asc }) | |
| order(*_order_columns.map { |column| table[column].asc }) |
This PR refactors the query constraints feature to be more contained and have less implicit behavior. Eventually we'll want `primary_key` to flow through query constraints and return `["id"]` when we have a single column and internally Rails will use an array for all `primary_key` and `foreign_key` calls, falling back to query constraints. At the moment however, this is a large change in Rails and one that could break current expected behavior. In order to implenent the feature and not break compatibility I think we should walk back the feature a little bit. The changes are: * Only return `query_constraints_list` if `query_constraints` was set by the model, otherwise return nil. * Re-implement `primary_key` calls where we only have a single ID * Update the `foreign_key` option on associations to use a `query_constraints` option instead so we don't need to check `is_a?(Array)`. These changes will ensure that the changes are contained to `query_constraints` rather than having to make decisions about whether we want to treat `primary_key` as an array. For now we will call everything `query_constraints` which makes it easy to see the line when we want an array vs single column. Later we can change the meaning of `primary_key` if necessary.
eileencodes
force-pushed
the
refactor-query-constraints
branch
from
4917b96
to
f25fa3f
Compare
I worked on this last week and shared with @nvasilevski as well. The main thing I'm doing here is walking the composite keys / query constraints feature back a bit so that we know when we have query constraints vs a primary rather than trying to make primary key into query constraints. Eventually we'll want to remove the branching but that's not required to implement the feature and we can't change the meaning of
primary_keyin the process (we can do it later as a separate implementation).Commit message:
This PR refactors the query constraints feature to be more contained and have less implicit behavior. Eventually we'll want
primary_keyto flow through query constraints and return["id"]when we have a single column and internally Rails will use an array for allprimary_keyandforeign_keycalls, falling back to query constraints. At the moment however, this is a large change in Rails and one that could break current expected behavior. In order to implement the feature and not break compatibility I think we should walk back the feature a little bit. The changes are:query_constraints_listifquery_constraintswas set by the model, otherwise return nil.primary_keycalls where we only have a single IDforeign_keyoption on associations to use aquery_constraintsoption instead so we don't need to checkis_a?(Array).These changes will ensure that the changes are contained to
query_constraintsrather than having to make decisions about whether we want to treatprimary_keyas an array. For now we will call everythingquery_constraintswhich makes it easy to see the line when we want an array vs single column. Later we can change the meaning ofprimary_keyif necessary.