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

Merge pull request #2070 from koic/build_subselect_doesnt_have_ordering #2073

Merged
merged 1 commit into from
Nov 30, 2020

Conversation

yahonda
Copy link
Collaborator

@yahonda yahonda commented Nov 30, 2020

This pull request backports #2070 into the release60 branch.
Since #2002 has been merged to release61 branch or newer, #2070 is not feasible to merge to release60 branch. Then applied some dirty monkey patch to lib/active_record/connection_adapters/oracle_enhanced_adapter.rb file.

…dering

`build_subselect` does not have ordering
@yahonda yahonda requested a review from koic November 30, 2020 12:06
@yahonda
Copy link
Collaborator Author

yahonda commented Nov 30, 2020

@koic Would you review this pull request? I do not think this "monkey patch" is the best way but do not want to add new file structures to the release60 branch.

Copy link
Collaborator

@koic koic left a comment

Choose a reason for hiding this comment

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

Thank you for creating the patch. I think this monkey patch approach is not bad.

The following is a comparison with opening a patch to Arel Visitor of Rails repo vs this PR approach.

Pros:

  • Oracle enhanced adapter gem can have a release cycle independent of Rails.
  • No need to have Rails maintainers consider Oracle-specific issue.

Cons:

  • Arel Visitor of Oracle packaged in the upstream Rails 6.0 series never resolve this issue.

Also, Rails 6.0 series is a stable version. So I think any drastic changes will not be made to Arel Visitor, so I think it's unnecessary to copy all of Arel Visitor.

Conclusion. In practice, Active Record Oracle enhanced apapter for Rails 6.0 should also be updated to the latest version if user expect bug fixes. So I'm not sure whether this monkey patch is the best option, but I think it's a better option.

@yahonda
Copy link
Collaborator Author

yahonda commented Nov 30, 2020

Thanks for the review.

@yahonda yahonda merged commit 00e84eb into rsim:release60 Nov 30, 2020
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.

None yet

2 participants