Ruby on Rails | Screencasts | Download | Documentation | Weblog | Community | Source

Ticket #11046 (closed enhancement: fixed)

Opened 7 months ago

Last modified 7 months ago

[PATCH] Fix eager loading with pre-quoted table names

Reported by: danielmorrison Assigned to: core
Priority: normal Milestone: 2.1
Component: ActiveRecord Version: edge
Severity: normal Keywords: verified
Cc:

Description

[8673] (preload query strategy for eager :includes) introduces an issue with pre-quoted table names.

String conditions work correctly 

>> Person.find(:all, :conditions => ["accounts.domain = ?", 'example.com'], :include => :account)
=> []

However, if you properly quote table names in the conditions string, it raises an error. 

>> Person.find(:all, :conditions => ["`accounts`.`domain` = ?", 'example.com'], :include => :account)
ActiveRecord::StatementInvalid: Mysql::Error: Unknown column 'accounts.domain' in 'where clause': SELECT * FROM `people`   WHERE (`accounts`.`domain` = 'example.com')

This is because the regular expression to determine if included tables are in the conditions doesn't take quoted names into account. Therefore, it drops the include option and fails.

This becomes more of an issue in queries where you need to use string conditions (LIKE, >, etc.)

Big thanks to Steve Smith for diagnosing the problem. I'm just the patch monkey.

Attachments

fix_eager_loading_with_quoted_table_names.diff (5.6 kB) - added by danielmorrison on 02/08/08 15:36:54.

Change History

02/07/08 20:18:03 changed by danielmorrison

Rats. that version of the patch has the actual mysql quotes instead of the method call... fixing...

02/08/08 00:38:01 changed by danielmorrison

  • summary changed from [PATCH][TINY] Fix eager loading wit pre-quoted table names to [PATCH] Fix eager loading wit pre-quoted table names.

No longer a tiny patch, now we have to deal with the fact that we need to know the quote character for each adapter.

This patch is the minimum to get it working. Tests pass in mysql and it won't affect other adapters.

02/08/08 01:10:00 changed by bitsweat

  • milestone changed from 2.x to 2.1.

we know there's a single quote character -- the specific character doesn't matter

02/08/08 01:13:58 changed by orderedlist

Thanks Daniel, nice work! +1

02/08/08 14:33:23 changed by danielmorrison

Maybe I need to clarify what this patch does.

If the world only used MySQL, I could change the regexp in associations.rb from 

/([\.\w]+)\.\w+/

to 

/`?([\.\w]+)`?\.`?\w+`?/

This is what pulls out all table names from a string, and the addition is the optional quote characters.

Unfortunately, we don't know the quote character for each adapter, so I added a method to figure this out, and refactored the abstract adapter and mysql as necessary.

02/08/08 15:09:05 changed by danielmorrison

I updated the patch to fix the same problem with :order. Might as well include it in this patch because its exactly the same.

02/08/08 15:25:42 changed by jqr

+1

02/08/08 15:36:54 changed by danielmorrison

  • attachment fix_eager_loading_with_quoted_table_names.diff added.

02/08/08 15:38:09 changed by danielmorrison

Based on bitsweat's comment (and disucssion with jqr), I've updated the patch again.

Now we assume that tables and columns are quoted with the same character.

Cleaned up the patch nicely.

02/08/08 16:12:34 changed by brandon

+1

02/08/08 16:16:06 changed by danielmorrison

  • keywords set to verified.
  • summary changed from [PATCH] Fix eager loading wit pre-quoted table names to [PATCH] Fix eager loading with pre-quoted table names.

02/08/08 23:35:36 changed by lotswholetime

+1

02/11/08 02:56:35 changed by nzkoz

  • status changed from new to closed.
  • resolution set to fixed.

I applied your tests from this patch and tried something simpler for the implementation in [8856]

I think this is fine, and all the tests pass. reopen if there's a case this doesn't cater for or breaks in some way.

02/11/08 03:00:30 changed by nzkoz

(In [8856]) Fix eager loading with pre-quoted table names. Closes #11046 [danielmorrison, Koz, Jeremy Kemper]