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 columns_for_distinct when using Rails 6.1 #2249

Merged
merged 1 commit into from
Jan 20, 2022

Conversation

swamp09
Copy link
Contributor

@swamp09 swamp09 commented Jan 19, 2022

Running ARCONN=oracle bundle exec ruby -Itest ./test/cases/finder_test.rb -n test_find_with_order_on_included_associations_with_construct_finder_sql_for_association_limiting_and_is_distinct in Rails 6.1.4.4 will result in an error.

ARCONN=oracle bundle exec ruby -Itest ./test/cases/finder_test.rb -n test_find_with_order_on_included_associations_with_construct_finder_sql_for_association_limiting_and_is_distinct
Using oracle
Warning: NLS_LANG is not set. fallback to US7ASCII.
Run options: -n test_find_with_order_on_included_associations_with_construct_finder_sql_for_association_limiting_and_is_distinct --seed 32511

# Running:

F

Failure:
FinderTest#test_find_with_order_on_included_associations_with_construct_finder_sql_for_association_limiting_and_is_distinct [./test/cases/finder_test.rb:1503]:
Expected: 2
  Actual: 1

rails test ./test/cases/finder_test.rb:1502

Finished in 1.275346s, 0.7841 runs/s, 0.7841 assertions/s.
1 runs, 1 assertions, 1 failures, 0 errors, 0 skips

Fixing the return value of columns_for_distinct causes the test to pass.
This is a fix for the change to PostgreSQL and MySQL adapters in Rails 6.0. rails/rails#31966

The reason why the tests for ARCONN=oracle bundle exec ruby -Itest ./test/cases/finder_test.rb -n test_find_with_order_on_included_associations_with_construct_finder_sql_for_association_limiting_and_is_distinct don't pass is because of a fix in finder_methods.rb in Rails 6.1.
rails/rails@2ad2425

Running `ARCONN=oracle bundle exec ruby -Itest ./test/cases/finder_test.rb -n test_find_with_order_on_included_associations_with_construct_finder_sql_for_association_limiting_and_is_distinct` in Rails 6.1.4.4 will result in an error.

```
ARCONN=oracle bundle exec ruby -Itest ./test/cases/finder_test.rb -n test_find_with_order_on_included_associations_with_construct_finder_sql_for_association_limiting_and_is_distinct
Using oracle
Warning: NLS_LANG is not set. fallback to US7ASCII.
Run options: -n test_find_with_order_on_included_associations_with_construct_finder_sql_for_association_limiting_and_is_distinct --seed 32511

# Running:

F

Failure:
FinderTest#test_find_with_order_on_included_associations_with_construct_finder_sql_for_association_limiting_and_is_distinct [./test/cases/finder_test.rb:1503]:
Expected: 2
  Actual: 1

rails test ./test/cases/finder_test.rb:1502

Finished in 1.275346s, 0.7841 runs/s, 0.7841 assertions/s.
1 runs, 1 assertions, 1 failures, 0 errors, 0 skips
```

Changing the return value of `columns_for_distinct` causes the test to pass.
This is a fix for the change to PostgreSQL and MySQL adapters in Rails 6.0. rails/rails#31966

The reason why the tests for  `ARCONN=oracle bundle exec ruby -Itest ./test/cases/finder_test.rb -n test_find_with_order_on_included_associations_with_construct_finder_sql_for_association_limiting_and_is_distinct` don't pass is because of a fix in finder_methods.rb in Rails 6.1.
rails/rails@2ad2425
@yahonda
Copy link
Collaborator

yahonda commented Jan 19, 2022

Thanks for opening a pull request. Would you make sure this test fails against Rails main branch (7.1.0.alpha) and Oracle enhanced adapter master branch? If so, I'm happy to merge this pull request and backport them to release70 and release61 branches.

@swamp09
Copy link
Contributor Author

swamp09 commented Jan 20, 2022

Tests failed on the Rails main branch and Oracle enhanced adapter master branch.
Ruby version is 2.7.2.
OracleDB version is Oracle Database 11g Express Edition.

@yahonda yahonda merged commit fa6cda6 into rsim:master Jan 20, 2022
@yahonda
Copy link
Collaborator

yahonda commented Jan 20, 2022

Thank you for the confirmation. Let me backport it to release70 and release61 branches.

@swamp09 swamp09 deleted the fix_columns_for_distinct branch January 20, 2022 10:14
yahonda added a commit to yahonda/oracle-enhanced that referenced this pull request Jan 20, 2022
yahonda added a commit to yahonda/oracle-enhanced that referenced this pull request Jan 20, 2022
yahonda added a commit that referenced this pull request Jan 20, 2022
Merge pull request #2249 from swamp09/fix_columns_for_distinct
yahonda added a commit that referenced this pull request Jan 20, 2022
Merge pull request #2249 from swamp09/fix_columns_for_distinct
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