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.

Sign up for GitHub

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

Jump to bottom

Accept nested functions in Dangerous Query Methods #44010

Merged
merged 1 commit into from
Jul 19, 2022
Merged

Accept nested functions in Dangerous Query Methods #44010

merged 1 commit into from
Jul 19, 2022

Conversation

siegfault
Copy link
Contributor

Summary

I think there’s an opportunity to reduce additional false positives for
Dangerous Query Method deprecations/errors. Similar to #36448, it seems
reasonable to allow functions that accept other functions (e.g.
length(trim(title))).

Other Information

Mailing list thread: https://discuss.rubyonrails.org/t/feature-proposal-accept-nested-functions-w-r-t-dangerous-query-methods/78650

Background

@ghiculescu
Copy link
Member

ghiculescu commented Dec 27, 2021 •
edited

Thanks for the PR, and welcome to Rails. Could you add a test that shows some example of what would be accepted now?

I think ideally #33330 would handle all these cases, but it doesn't seem to be moving very quickly.

@siegfault
Copy link
Contributor Author

Absolutely - thanks for taking a look! Let me know how those tests look and if there are other places I should add tests. #36448 did also modify activerecord/test/cases/associations/eager_test.rb and activerecord/test/cases/relations_test.rb, but I'm not sure whether that was more out of necessity

@siegfault siegfault force-pushed the dangerous_query_method_allow_nested_functions branch from 5b38084 to d1a1b0f Compare March 2, 2022 23:17
@siegfault
Copy link
Contributor Author

@ghiculescu I went ahead and fixed some merge conflicts, added a test as you mentioned (but let me know if there are others I should add!), and I think this is ready to go. Let me know if there's any other improvements I can make

@ghiculescu ghiculescu added the ready PRs ready to merge label Mar 3, 2022
@ghiculescu
Copy link
Member

That looks really good. I will leave it for review from a core team member.

@siegfault siegfault force-pushed the dangerous_query_method_allow_nested_functions branch 3 times, most recently from e370856 to 86e4dba Compare March 4, 2022 00:35
@siegfault siegfault force-pushed the dangerous_query_method_allow_nested_functions branch 2 times, most recently from 3231694 to 96669d5 Compare March 18, 2022 20:17
@siegfault siegfault force-pushed the dangerous_query_method_allow_nested_functions branch from 96669d5 to 14a4382 Compare May 16, 2022 18:23
@siegfault
Copy link
Contributor Author

@ghiculescu is there anyone in particular that I should request for review or are there any next steps I should be doing? Thanks!

@ghiculescu
Copy link
Member

Nah, it just needs someone from the core team to review. They are pretty busy (particularly with Railsconf this week) but I'm sure they will get to it!

ghiculescu
ghiculescu reviewed May 17, 2022
View reviewed changes
activerecord/CHANGELOG.md Outdated
@@ -62,6 +62,10 @@

*Alex Ghiculescu*

* Allow nested functions as safe SQL string
Copy link
Member

Choose a reason for hiding this comment

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

please move this to the top of the file

@siegfault siegfault force-pushed the dangerous_query_method_allow_nested_functions branch from 14a4382 to abb75d8 Compare May 17, 2022 21:17
@siegfault
Copy link
Contributor Author

particularly with Railsconf this week

Oh, right. Unideal time for me to ping on this 😆 I'll leave this be. Thanks again for all the help!

@siegfault siegfault force-pushed the dangerous_query_method_allow_nested_functions branch 3 times, most recently from d6a9a8c to 557a13e Compare June 21, 2022 19:34
@siegfault siegfault force-pushed the dangerous_query_method_allow_nested_functions branch 3 times, most recently from 1bb52ed to 6baba26 Compare June 30, 2022 19:42
@siegfault
Accept nested functions in Dangerous Query Methods
8fe1bd5 
Mailing list thread: https://discuss.rubyonrails.org/t/feature-proposal-accept-nested-functions-w-r-t-dangerous-query-methods/78650

*Summary*
I think there’s an opportunity to reduce additional false positives for
Dangerous Query Method deprecations/errors.

*Nested Functions*
Similar to rails#36448, it seems
reasonable to allow functions that accept other functions (e.g.
`length(trim(title))`).

*Background*
* PR accepting non-nested functions: rails#36448
* Deep background on deprecation and false positives: rails#32995
* Constants: `COLUMN_NAME` for the first and `COLUMN_NAME_WITH_ORDER` for both
@siegfault siegfault force-pushed the dangerous_query_method_allow_nested_functions branch from 6baba26 to 8fe1bd5 Compare July 13, 2022 18:52
@byroot byroot merged commit 71c59c6 into rails:main Jul 19, 2022
@siegfault siegfault deleted the dangerous_query_method_allow_nested_functions branch July 19, 2022 19:27
koic added a commit to koic/oracle-enhanced that referenced this pull request Jul 20, 2022
@koic
Accept nested functions in Dangerous Query Methods
a1b3a1d 
Follow up rails/rails#44010.
@koic koic mentioned this pull request Jul 20, 2022
Merged
aidanharan added a commit to rails-sqlserver/activerecord-sqlserver-adapter that referenced this pull request Sep 13, 2023
@aidanharan
Accept nested functions in Dangerous Query Methods
c015641 
See rails/rails#44010
@aidanharan aidanharan mentioned this pull request Sep 13, 2023
Merged
aidanharan added a commit to rails-sqlserver/activerecord-sqlserver-adapter that referenced this pull request Sep 13, 2023
@aidanharan
Accept nested functions in Dangerous Query Methods (#1082)
ebf14dc 
See rails/rails#44010
lavika pushed a commit to lavika/activerecord-sqlserver-adapter that referenced this pull request Sep 26, 2023
@aidanharan
Accept nested functions in Dangerous Query Methods (rails-sqlserver#1082
0a4210b 
)

See rails/rails#44010
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ghiculescu ghiculescu ghiculescu left review comments

Assignees
No one assigned
Labels
activerecord ready PRs ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@siegfault @ghiculescu @byroot