Skip to content
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

Fix for Rails 8 query issue breaks select with no order clause #659

Merged
merged 3 commits into from
Dec 1, 2024

Conversation

shioyama
Copy link
Owner

@shioyama shioyama commented Dec 1, 2024

#655 fixed an issue with failing querying tests in AR 8, but as it turns out test coverage was incomplete and the fix has introduced breakage to select when used without order.

I added a small expectation which removes the order on part of existing querying tests, and tests now fail on master. Removing the fix from #655, they pass (but others fail on AR 8).

The change in Rails was this commit: rails/rails@ba468db

cc @jukra @n-rodriguez

We now have a broken state on master.

@@ -354,6 +354,9 @@

describe "selecting translated attributes" do
it "returns value from attribute methods on results" do
selected_unordered = query_scope.select(a1).map { |result| result.send(a1) }
expect(selected_unordered.sort).to eq(["bar", "bar", "bar", "foo", "foo"])
Copy link
Owner Author

@shioyama shioyama Dec 1, 2024

Choose a reason for hiding this comment

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

This fails with the query.order_values.any? check, but passes with it removed.

@n-rodriguez
Copy link
Contributor

n-rodriguez commented Dec 1, 2024

With the patch :

SELECT "article_translations_en"."title" FROM "articles" LEFT OUTER JOIN "article_translations" "article_translations_en" ON "article_translations_en"."article_id" = "articles"."id" AND "article_translations_en"."locale" = 'en'
#<Article title: "foo", id: nil>
nil
#<Article title: "foo", id: nil>
nil
#<Article title: "bar", id: nil>
nil
#<Article title: "bar", id: nil>
nil
#<Article title: "bar", id: nil>
nil

Without the patch :

SELECT "article_translations_en"."title" AS __mobility_title_en__ FROM "articles" LEFT OUTER JOIN "article_translations" "article_translations_en" ON "article_translations_en"."article_id" = "articles"."id" AND "article_translations_en"."locale" = 'en'
#<Article __mobility_title_en__: "foo", id: nil>
"foo"
#<Article __mobility_title_en__: "foo", id: nil>
"foo"
#<Article __mobility_title_en__: "bar", id: nil>
"bar"
#<Article __mobility_title_en__: "bar", id: nil>
"bar"
#<Article __mobility_title_en__: "bar", id: nil>
"bar"

SQL :

SELECT "article_translations_en"."title"                          FROM "articles" LEFT OUTER JOIN "article_translations" "article_translations_en" ON "article_translations_en"."article_id" = "articles"."id" AND "article_translations_en"."locale" = 'en'
SELECT "article_translations_en"."title" AS __mobility_title_en__ FROM "articles" LEFT OUTER JOIN "article_translations" "article_translations_en" ON "article_translations_en"."article_id" = "articles"."id" AND "article_translations_en"."locale" = 'en'

@n-rodriguez
Copy link
Contributor

Maybe related? :

rails/rails#53509
rails/rails#53514

@shioyama
Copy link
Owner Author

shioyama commented Dec 1, 2024

I think I have a fix which works for now, see my last commit.

@shioyama shioyama force-pushed the shioyama/fix_ar_8_querying branch 4 times, most recently from 2a97a65 to dc7b2f5 Compare December 1, 2024 04:12
This is not ideal but will do as a quick fix.
@shioyama shioyama force-pushed the shioyama/fix_ar_8_querying branch from dc7b2f5 to 4d688a3 Compare December 1, 2024 04:12
@shioyama
Copy link
Owner Author

shioyama commented Dec 1, 2024

There should be a better way to make selects work, but for now I'm going to ship this to at least fix the immediate problem.

@shioyama shioyama merged commit c9fc68e into master Dec 1, 2024
121 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants