allow array or hash syntax for partial index conditions #13353

Open
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
Contributor

ccutrer commented Dec 17, 2013

No description provided.

Owner

rafaelfranca commented Dec 17, 2013

Thank you but I don't think we should allow this in the migration API.

It should be low level and don't rely on the models code.

WDYT @jeremy?

Contributor

ccutrer commented Dec 17, 2013

Would it be more agreeable to remove the constantize, and only use ActiveRecord::Base as the klass in the relation? We try to encourage hash or array syntax for conditions when possible to avoid SQL injections, and not being able to use a hash here goes against that habit.

Owner

rafaelfranca commented Dec 17, 2013

Migrations are not susceptible SQL injection. I also find a anti-pattern to
use models in Migrations so I really don't want to encourage the usage.

Sent using mobile Gmail
On Dec 17, 2013 8:49 PM, "Cody Cutrer" notifications@github.com wrote:

Would it be more agreeable to remove the constantize, and only use
ActiveRecord::Base as the klass in the relation? We try to encourage hash
or array syntax for conditions when possible to avoid SQL injections, and
not being able to use a hash here goes against that habit.


Reply to this email directly or view it on GitHubhttps://github.com/rails/rails/pull/13353#issuecomment-30799747
.

Owner

jeremy commented Dec 18, 2013

Neat idea! It does mix up our layers of abstraction, though. By working with relations, it appears that we'll accept any relation to auto-construct an index, but we're really just grabbing it's where value—we're using it as a SQL builder.

On the flip side, this is the same API you'd expect to use to specify conditions in all other app code.

I'm positive on exploring the use of Array and Hash where: values, but dropping the Relation. That keeps the arg value nailed down to simple column-comparison conditions.

Contributor

ccutrer commented Dec 19, 2013

@jeremy @rafaelfranca updated to not allow relations, and use lower level methods for converting hash and array to raw SQL.

@jeremy jeremy and 1 other commented on an outdated diff Dec 19, 2013

...ord/connection_adapters/abstract/schema_statements.rb
@@ -785,8 +791,16 @@ def add_index_options(table_name, column_name, options = {})
using = "USING #{options[:using]}" if options[:using].present?
- if supports_partial_index?
- index_options = options[:where] ? " WHERE #{options[:where]}" : ""
+ if supports_partial_index? && options[:where]
+ index_options = " WHERE #{case options[:where]
+ when Array
+ ActiveRecord::Base.send(:sanitize_sql, options[:where], table_name)
+ when Hash
+ Arel::Visitors::WhereSql.new(self).accept(
+ PredicateBuilder.build_from_hash(ActiveRecord::Base, options[:where], Arel::Table.new(table_name)))
+ else
+ options[:where]
+ end}"
@jeremy

jeremy Dec 19, 2013

Owner

Embedding a multiline case within a string interpolation is hard to read. Good time to extract the case out to a separate method. Something like:

index_options = " WHERE #{partial_index_conditions(options[:where])}"
@ccutrer

ccutrer Dec 19, 2013

Contributor

@jeremy updated to use a refactored method

@jeremy jeremy and 1 other commented on an outdated diff Dec 19, 2013

...ord/connection_adapters/abstract/schema_statements.rb
@@ -800,6 +806,18 @@ def add_index_options(table_name, column_name, options = {})
[index_name, index_type, index_columns, index_options, algorithm, using]
end
+ def partial_index_conditions(where, table_name)
+ case where
+ when Array
+ ActiveRecord::Base.send(:sanitize_sql, where, table_name)
@jeremy

jeremy Dec 19, 2013

Owner

Should call sanitize_sql_for_conditions

@ccutrer

ccutrer Dec 19, 2013

Contributor

fixed. I just followed the example at QueryMethods#build_wheres. I take it there's a preference for _for_conditions? (they're aliased to be the same thing)

@jeremy jeremy commented on an outdated diff Dec 19, 2013

...ord/connection_adapters/abstract/schema_statements.rb
@@ -800,6 +806,18 @@ def add_index_options(table_name, column_name, options = {})
[index_name, index_type, index_columns, index_options, algorithm, using]
end
+ def partial_index_conditions(where, table_name)
+ case where
+ when Array
@jeremy

jeremy Dec 19, 2013

Owner

No need to indent when clauses - keep on the same depth as the case

@jeremy jeremy and 1 other commented on an outdated diff Dec 19, 2013

...ord/connection_adapters/abstract/schema_statements.rb
@@ -800,6 +806,18 @@ def add_index_options(table_name, column_name, options = {})
[index_name, index_type, index_columns, index_options, algorithm, using]
end
+ def partial_index_conditions(where, table_name)
+ case where
+ when Array
+ ActiveRecord::Base.send(:sanitize_sql_for_conditions, where, table_name)
+ when Hash
+ Arel::Visitors::WhereSql.new(self).accept(
+ PredicateBuilder.build_from_hash(ActiveRecord::Base, where, Arel::Table.new(table_name)))
@jeremy

jeremy Dec 19, 2013

Owner

2 extra spaces here still ;)

@ccutrer

ccutrer Dec 19, 2013

Contributor

fixed

Contributor

ccutrer commented Dec 19, 2013

github seems confused. I rebased, and now it thinks the commits from rails master are part of this pull request

Owner

rafaelfranca commented Dec 19, 2013

OMG!, try to use rebase interactive to delete the repeated commits.

Contributor

ccutrer commented Dec 19, 2013

okay, it's better now. just rebased again. it was weird, cause the compare view from my repo was correct, but the pull request here was wrong. the parent was the current master of rails at that time.

Contributor

ccutrer commented Jan 10, 2014

any update on this?

Contributor

agrobbin commented Oct 29, 2014

👍 would also love to see this get updated with the latest 4.2 code! Having to pass a string for a simple WHERE condition is unfortunate.

Member

sgrif commented Oct 29, 2014

Needs a rebase and probably another review at this point.

Contributor

agrobbin commented Oct 29, 2014

I'd be happy to take a stab at bringing this up-to-date with master if @ccutrer doesn't have the time to do so.

Contributor

ccutrer commented Oct 29, 2014

@agrobbin: that'd be great. It's not high on my priority list, so it would take me a while to get around to it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment