Allow PredicateBuilder to recognize schema namespaced table names #48171

sholden · 2023-05-08T22:15:39Z

Motivation / Background

ActiveRecord::PredicateBuilder currently assumes that a column name will only be specified in dot notation with a single period. If a column is specified in dot notation for a table that is namespaced in a schema, it will use the schema name as the table name. This PR allows columns to specified in the format schema.table.column as well as table.column.

This fixes #48172

Detail

This Pull Request changes the behavior of ActiveRecord::PredicateBuilder.references and ActiveRecord::PredicateBuilder#convert_dot_notation_to_hash

I didn't add this to the CHANGELOG, but if a reviewer thinks it's significant enough I'm happy to do so.

Checklist

Before submitting the PR make sure the following are checked:

This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
Tests are added or updated if you fix a bug or add a feature.
CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

casperisfine · 2023-05-09T01:51:27Z

activerecord/lib/active_record/relation/predicate_builder.rb

@@ -30,7 +30,7 @@ def self.references(attributes)
        if value.is_a?(Hash)
          result << Arel.sql(key)
        elsif key.include?(".")
-          result << Arel.sql(key.split(".").first)
+          result << Arel.sql(key.split(".")[0..-2].join("."))


I think [0..-2] is a bit hard to grasp. Given we do this in two places it might make sense to extract a small private method here, e.g.

def split_column_name(key) *table, column = key.split(".") [table.join("."), column] end

At a glance I feel key.split(".")[0..-2].join(".") is a bit expensive. How about String#[] like idx = key.rindex("."); key[0, idx]?

I was a little meh on the split as well. I like the rindex idea, can rewrite that way. I just kept it since it was originally written that way. We could extract a method, but in one usage we need to return both the table and column, but in the other the column is ignored, so it might be wasteful to make an array and another string that we'd throw away.

casperisfine

Please squash your commits.

casperisfine · 2023-05-10T01:08:57Z

activerecord/lib/active_record/relation/predicate_builder.rb

+        dot_notation = attributes.each_with_object({}) do |(k, v), dn|
+          next if v.is_a?(Hash)
+          idx = k.rindex(".")
+          dn[k] = idx if idx
        end

-        dot_notation.each_key do |key|
-          table_name, column_name = key.split(".")
+        dot_notation.each do |key, idx|
+          table_name, column_name = key[0, idx], key[idx + 1, key.length]


Can't we merge these two loops and avoid the temporary Hash?

We can't mutate the original attributes hash while iterating over it. But maybe we could save a hash allocation on L163 with #merge!? I'm always hesitant to mutate things getting passed in if I don't know all of the contexts where it would get used, but if we're already mutating attributes that are passed to #build_from_hash maybe that's already fine?

if we're already mutating attributes that are passed to #build_from_hash maybe that's already fine?

Yeah that was my point. But also I don't think we need to mutate attributes at all, just iterate once over it and create the returned hash as we go.

Ah I gotcha. Updated to build a converted hash in one pass.

casperisfine · 2023-05-10T07:25:16Z

activerecord/lib/active_record/relation/predicate_builder.rb

+          if !value.is_a?(Hash) && (idx = key.rindex("."))
+            table_name, column_name = key[0, idx], key[idx + 1, key.length]
+            converted[table_name] ||= {}
+            converted[table_name] = converted[table_name].merge(column_name => value)


Any reason why you don't do:

Suggested change

converted[table_name] = converted[table_name].merge(column_name => value)

converted[table_name][column_name => value]

? That would avoid lots of Hash allocations.

Beyond the hash allocations, this single pass rewrite is breaking other tests. It's past midnight, going to revert and try again later. The way I'm attempting a single pass is causing keys to be overwritten. For example this is failing: https://github.com/sholden/rails/blob/predicate-builder-schemas/activerecord/test/cases/finder_test.rb#L1120

casperisfine · 2023-05-10T07:25:31Z

One comment, other than that LGTM.

sholden · 2023-05-10T20:00:25Z

Ok, I think this latest version is optimal as far as creating the converted hash in one pass with as few hash allocations as possible, albeit a little less readable. If it looks good I'll squash and clean up. I also added a test to ensure that we don't accidentally mutate the arguments originally passed to #where. In my first pass I realized updating with converted[table_name][column_name] = value as you suggested above was sometimes mutating the original hash, depending on the key ordering. I added the #dup call to prevent this from happening. Now, we will always merge in place or set keys on a fresh hash to avoid that scenario.

casperisfine · 2023-05-11T00:29:54Z

Yeah, looks good now 👍. Thanks a lot.

rails#48172]

sholden · 2023-05-11T01:18:19Z

No problem. Think we're good to go then!

rails-bot bot added the activerecord label May 8, 2023

sholden force-pushed the predicate-builder-schemas branch 3 times, most recently from 904e308 to 0fbcb51 Compare May 8, 2023 22:55

casperisfine reviewed May 9, 2023

View reviewed changes

sholden force-pushed the predicate-builder-schemas branch from 4a8899e to 1a06b1e Compare May 9, 2023 18:10

casperisfine reviewed May 10, 2023

View reviewed changes

sholden force-pushed the predicate-builder-schemas branch 2 times, most recently from 781c082 to dcc429f Compare May 10, 2023 06:34

casperisfine reviewed May 10, 2023

View reviewed changes

sholden force-pushed the predicate-builder-schemas branch 2 times, most recently from 04ab481 to 8b8d15b Compare May 10, 2023 19:29

Allow PredicateBuilder to recognize schema namespaced table names. [Fix

c5a99e0

rails#48172]

sholden force-pushed the predicate-builder-schemas branch from 9c82468 to c5a99e0 Compare May 11, 2023 00:47

byroot merged commit 403e4e5 into rails:main May 11, 2023
9 checks passed

casperisfine mentioned this pull request Jul 10, 2023

Stop deep duping query values of serialized attributes #48705

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow PredicateBuilder to recognize schema namespaced table names #48171

Allow PredicateBuilder to recognize schema namespaced table names #48171

sholden commented May 8, 2023 •

edited

casperisfine May 9, 2023

kamipo May 9, 2023

sholden May 9, 2023 •

edited

casperisfine left a comment

casperisfine May 10, 2023

sholden May 10, 2023 •

edited

casperisfine May 10, 2023

sholden May 10, 2023 •

edited

casperisfine May 10, 2023

sholden May 10, 2023

casperisfine commented May 10, 2023

sholden commented May 10, 2023 •

edited

casperisfine commented May 11, 2023

sholden commented May 11, 2023

	converted[table_name] = converted[table_name].merge(column_name => value)
	converted[table_name][column_name => value]

Allow PredicateBuilder to recognize schema namespaced table names #48171

Allow PredicateBuilder to recognize schema namespaced table names #48171

Conversation

sholden commented May 8, 2023 • edited

Motivation / Background

Detail

Checklist

casperisfine May 9, 2023

Choose a reason for hiding this comment

kamipo May 9, 2023

Choose a reason for hiding this comment

sholden May 9, 2023 • edited

Choose a reason for hiding this comment

casperisfine left a comment

Choose a reason for hiding this comment

casperisfine May 10, 2023

Choose a reason for hiding this comment

sholden May 10, 2023 • edited

Choose a reason for hiding this comment

casperisfine May 10, 2023

Choose a reason for hiding this comment

sholden May 10, 2023 • edited

Choose a reason for hiding this comment

casperisfine May 10, 2023

Choose a reason for hiding this comment

sholden May 10, 2023

Choose a reason for hiding this comment

casperisfine commented May 10, 2023

sholden commented May 10, 2023 • edited

casperisfine commented May 11, 2023

sholden commented May 11, 2023

sholden commented May 8, 2023 •

edited

sholden May 9, 2023 •

edited

sholden May 10, 2023 •

edited

sholden May 10, 2023 •

edited

sholden commented May 10, 2023 •

edited