[Fix #48535]: fix behavior of `proc_for_binds` in `Arel::Nodes::HomogenousIn` #49050

JohnAnon9771 · 2023-08-27T14:57:07Z

Motivation / Background

This PR addresses the issues #48535 and #48072.

With the introduction of PR #41068, an unintended behavior surfaced, causing the proc_for_binds to apply casting to attributes that were already casted. Consequently, this led to the malfunctioning of queries, as noted in the referenced issues.

This commit fixes the issue by replacing the type with ActiveModel::Type.default_value so that the 2nd serialization is effectively a no-op. This correction has obviated the need for changes such as "Fix deterministic queries that were broken after #41068," as detailed in this commit. These changes had inadvertently caused the tests initially intended for proc_for_binds within HomogeneousIn to erroneously evaluate the proc_for_binds within InWithAdditionalValues. This resulted in false positives whenever adjustments were made to the proc_for_binds within HomogeneousIn.

Fixes #48072.
Fixes #48535.

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.

matthewd · 2023-09-27T00:24:33Z

The description here says a lot about what the previous change did and what broke, but I've struggled to follow the reasoning of the change. Is the following accurate?

The value supplied to (the block returned by) proc_for_binds has already been typecast to database format
with_cast_value expects its value to be in application format (the "rich type" actually matching the Type)
This difference is hidden for many default types (integer, string, etc) because the two formats are equivalent
We need a bind attribute that knows it has already been cast, so it can still say which attribute name it belongs to, but will make no further conversion when asked for the DB value

Does FromDatabase do that?

JohnAnon9771 · 2023-10-07T22:43:11Z

Sorry for the confusion, I ended up messing up my local and pushed without realizing, which ended up causing a bug in the bot. I'll be more careful

JohnAnon9771 · 2023-10-08T16:37:32Z

The description here says a lot about what the previous change did and what broke, but I've struggled to follow the reasoning of the change. Is the following accurate?

The value supplied to (the block returned by) proc_for_binds has already been typecast to database format

with_cast_value expects its value to be in application format (the "rich type" actually matching the Type)

This difference is hidden for many default types (integer, string, etc) because the two formats are equivalent

We need a bind attribute that knows it has already been cast, so it can still say which attribute name it belongs to, but will make no further conversion when asked for the DB value

Does FromDatabase do that?

Yes, your conclusion is correct. And FromDatabase really works perfectly to solve this problem, greatly simplifying the solution. Thanks for the sugestion.

jonathanhefner

After digging into this, I think this solution is mostly on target. 👍 I've left a few comments, but I pushed a commit to address them since I was digging into the code anyway. I'll merge once the build is green.

jonathanhefner · 2023-11-09T18:56:55Z

activerecord/CHANGELOG.md

+*   Fix behavior of `proc_for_binds` in `Arel::Nodes::HomogeneousIn`
+
+    Previously, the `proc_for_binds` was introduced to address issues related to binds with null attribute names. However, this led to duplicated casting behavior, impacting searches involving casted attributes. This fix aims to resolve the issue using `FromDatabase`, mitigating casting issues while preserving the handling of null attribute names.


CHANGELOG entries are for users rather than Rails maintainers, so this should focus on how the user is affected instead of the implementation details.

jonathanhefner · 2023-11-09T20:22:55Z

activerecord/lib/arel/nodes/homogeneous_in.rb

-        -> value { ActiveModel::Attribute.with_cast_value(attribute.name, value, attribute.type_caster) }
+        -> value { ActiveModel::Attribute.from_database(attribute.name, value, ActiveModel::Type.default_value) }


The fix here is actually due to changing attribute.type_caster to ActiveModel::Type.default_value because that changes the subsequent serialize call into a no-op:

rails/activemodel/lib/active_model/type/value.rb

Lines 65 to 67 in a409c63

def serialize(value)

value

end

As such, changing with_cast_value to from_database has no effect because the methods that differ between them (see WithCastValue and FromDatabase) aren't being called.

jonathanhefner · 2023-11-09T21:10:38Z

activerecord/test/cases/log_subscriber_test.rb

+    def test_where_in_binds_with_json_attribute
+      Admin::User.where(json_options: [{ a: 1 }, { b: 2 }]).load
+      wait
+      assert_match(%{[["json_options", "{\\"a\\":1}"], ["json_options", "{\\"b\\":2}"]]}, @logger.logged(:debug).last)
+    end


This tests the logging output, but what we're really trying to fix is the query, so I think we should test the query results instead.

Previously, `proc_for_binds` was introduced to address missing attribute names when logging binds. However, this causes double serialization, impacting searches involving serialized attributes. This commit fixes the issue by replacing the type with `ActiveModel::Type.default_value` so that the 2nd serialization is effectively a no-op. Fixes rails#48072. Fixes rails#48535. Co-authored-by: Jonathan Hefner <jonathan@hefner.pro>

jonathanhefner · 2023-11-09T21:38:24Z

Thank you, @JohnAnon9771! 🥛

Backported to 7-1-stable in 28deb8b and 7-0-stable in 8f72133.

[Fix #48535]: fix behavior of `proc_for_binds` in `Arel::Nodes::HomogenousIn` (cherry picked from commit 377f6f6)

rails-bot bot added the activerecord label Aug 27, 2023

JohnAnon9771 force-pushed the fix/issue-48535 branch 2 times, most recently from 40d32f1 to 42ee8a2 Compare September 2, 2023 18:45

JohnAnon9771 force-pushed the fix/issue-48535 branch 3 times, most recently from 1444b29 to 5627047 Compare September 17, 2023 21:12

ghiculescu mentioned this pull request Sep 27, 2023

Postgres: use ANY instead of IN for array inclusion queries #49388

Open

JohnAnon9771 force-pushed the fix/issue-48535 branch from 5627047 to 2acf113 Compare October 7, 2023 22:33

JohnAnon9771 requested a review from rafaelfranca as a code owner October 7, 2023 22:33

rails-bot bot added actioncable actionmailbox actionmailer labels Oct 7, 2023

JohnAnon9771 force-pushed the fix/issue-48535 branch from 2acf113 to 5627047 Compare October 7, 2023 22:41

JohnAnon9771 force-pushed the fix/issue-48535 branch 2 times, most recently from ab31def to ad4506d Compare October 8, 2023 16:21

skipkayhil removed actionmailer actioncable actionmailbox labels Oct 20, 2023

JohnAnon9771 force-pushed the fix/issue-48535 branch 3 times, most recently from 78b0001 to a26277b Compare November 4, 2023 16:18

jonathanhefner force-pushed the fix/issue-48535 branch from a26277b to 9d6532f Compare November 9, 2023 21:24

jonathanhefner reviewed Nov 9, 2023

View reviewed changes

jonathanhefner force-pushed the fix/issue-48535 branch from 9d6532f to fb5773b Compare November 9, 2023 21:26

jonathanhefner merged commit 377f6f6 into rails:main Nov 9, 2023
4 checks passed

jonathanhefner added a commit that referenced this pull request Nov 9, 2023

Merge pull request #49050 from JohnAnon9771/fix/issue-48535

28deb8b

[Fix #48535]: fix behavior of `proc_for_binds` in `Arel::Nodes::HomogenousIn` (cherry picked from commit 377f6f6)

jonathanhefner added a commit that referenced this pull request Nov 9, 2023

Merge pull request #49050 from JohnAnon9771/fix/issue-48535

8f72133

[Fix #48535]: fix behavior of `proc_for_binds` in `Arel::Nodes::HomogenousIn` (cherry picked from commit 377f6f6)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fix #48535]: fix behavior of `proc_for_binds` in `Arel::Nodes::HomogenousIn` #49050

[Fix #48535]: fix behavior of `proc_for_binds` in `Arel::Nodes::HomogenousIn` #49050

JohnAnon9771 commented Aug 27, 2023 •

edited

matthewd commented Sep 27, 2023

JohnAnon9771 commented Oct 7, 2023 •

edited

JohnAnon9771 commented Oct 8, 2023

jonathanhefner left a comment

jonathanhefner Nov 9, 2023

jonathanhefner Nov 9, 2023

jonathanhefner Nov 9, 2023

jonathanhefner commented Nov 9, 2023 •

edited

		* Fix behavior of `proc_for_binds` in `Arel::Nodes::HomogeneousIn`

		Previously, the `proc_for_binds` was introduced to address issues related to binds with null attribute names. However, this led to duplicated casting behavior, impacting searches involving casted attributes. This fix aims to resolve the issue using `FromDatabase`, mitigating casting issues while preserving the handling of null attribute names.

		-> value { ActiveModel::Attribute.with_cast_value(attribute.name, value, attribute.type_caster) }
		-> value { ActiveModel::Attribute.from_database(attribute.name, value, ActiveModel::Type.default_value) }

[Fix #48535]: fix behavior of proc_for_binds in Arel::Nodes::HomogenousIn #49050

[Fix #48535]: fix behavior of proc_for_binds in Arel::Nodes::HomogenousIn #49050

Conversation

JohnAnon9771 commented Aug 27, 2023 • edited

Motivation / Background

Checklist

matthewd commented Sep 27, 2023

JohnAnon9771 commented Oct 7, 2023 • edited

JohnAnon9771 commented Oct 8, 2023

jonathanhefner left a comment

Choose a reason for hiding this comment

jonathanhefner Nov 9, 2023

Choose a reason for hiding this comment

jonathanhefner Nov 9, 2023

Choose a reason for hiding this comment

jonathanhefner Nov 9, 2023

Choose a reason for hiding this comment

jonathanhefner commented Nov 9, 2023 • edited

[Fix #48535]: fix behavior of `proc_for_binds` in `Arel::Nodes::HomogenousIn` #49050

[Fix #48535]: fix behavior of `proc_for_binds` in `Arel::Nodes::HomogenousIn` #49050

JohnAnon9771 commented Aug 27, 2023 •

edited

JohnAnon9771 commented Oct 7, 2023 •

edited

jonathanhefner commented Nov 9, 2023 •

edited