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

Ignore orders for delete_all/update_all queries. #621

Conversation

Jack12816
Copy link

- What is it good for

While upgrading an application from Rails 5.2 to 6.1 (CPK 11.2 -> 13.0) some specs broke. I tracked it down to Entity.delete_all calls on the setup of the test suite. Then I found the broken SQL statement and about an syntax error near to ORDER - there is nothing to order at the top-level for DELETE and UPDATE queries. This is caused by a default_scope { order(created_at: :asc) }.

Click for Details

ActiveRecord::Base.delete_all

class Tariff < ActiveRecord::Base
  default_scope { order(created_at: :asc) }
end

Tariff.delete_all
DELETE FROM "tariffs"
WHERE ("tariffs"."tariff_id", "tariffs"."start_date") IN (
  SELECT "tariffs"."tariff_id", "tariffs"."start_date"
  FROM "tariffs"
  ORDER BY "tariffs"."created_at" ASC
)
ORDER BY "tariffs"."created_at" ASC -- syntax error due to the
                                    -- default scope/order

-- ActiveRecord::StatementInvalid: PG::SyntaxError:
--   ERROR:  syntax error at or near "ORDER"
-- LINE 1: ...OM "tariffs" ORDER BY "tariffs"."created_at" ASC) ORDER BY "...

ActiveRecord::Base.update_all

class Tariff < ActiveRecord::Base
  default_scope { order(created_at: :asc) }
end

Tariff.update_all(amount: 1337)
UPDATE "tariffs"
WHERE ("tariffs"."tariff_id", "tariffs"."start_date") IN (
  SELECT "tariffs"."tariff_id", "tariffs"."start_date"
  FROM "tariffs"
  ORDER BY "tariffs"."created_at" ASC
)
ORDER BY "tariffs"."created_at" ASC -- syntax error due to the
                                    -- default scope/order

-- ActiveRecord::StatementInvalid: PG::SyntaxError:
--   ERROR:  syntax error at or near "ORDER"
-- LINE 1: ...OM "tariffs" ORDER BY "tariffs"."created_at" ASC) ORDER BY "...

- What I did

Just removed the ordering on delete_all and update_all SQL statement assembly and added tests for it. I fixed the issue at the ar_6.1.x branch, but I'm sure this must be ported back to 6.0 and master.

- A picture of a cute animal (not mandatory but encouraged)
image

@Jack12816 Jack12816 changed the base branch from master to ar_6.1.x December 30, 2024 11:51
@cfis
Copy link
Contributor

cfis commented Dec 31, 2024

The calls to stmt.order(*arel.orders) are copied from ActiveRecord. See the original method here:

https://github.com/rails/rails/blob/6-1-stable/activerecord/lib/active_record/relation.rb#L455

Removing the order call would likely will break any non-CPK AR classes. So we can't do that. I also imagine the stmt.offset and stmt.take calls cause the same issue type of issue.

The PR needs to be updated to not break non-CPK models.

@Jack12816
Copy link
Author

Closing for the same reason: #622 (comment)

@Jack12816 Jack12816 closed this Jan 7, 2025
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