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

Ticket #7987 (closed defect: fixed)

Opened 1 year ago

Last modified 9 months ago

[PATCH] Fix column type detection while loading fixtures

Reported by: roderickvd Assigned to: core
Priority: normal Milestone: 2.x
Component: ActiveRecord Version: edge
Severity: normal Keywords: tiny fixtures binary verified
Cc: ctm, fotos

Description

Running Ruby 1.8.6 on Edge Rails r6500, Mac OS X 10.4.9:

Trying to load binary data from test fixtures currently fails on many database servers. This is because of an error in the fixture loading code that causes table column types to go unrecognized. Because ActiveRecord doesn't know the column type it also doesn't know how to properly quote the fixture data. Finally, the SQL statements will go awry.

This problem shows up with :binary columns because binary input typically require special data escaping beyond basic string escaping. Such escaping is done in the database adapters' #quote method, which needs a properly detected column type to function correctly.

The current code calls #kind_of? on a constantized class to test for a superclass. This is incorrect. The following irb snippet shows why:

irb(main):001:0> class Person; end
=> nil
irb(main):002:0> class Father < Person; end
=> nil
irb(main):003:0> dad = Father.new
=> #<Father:0x89314>
irb(main):004:0> dad.kind_of?(Person)
=> true
irb(main):005:0> Father.kind_of?(Person)
=> false
irb(main):006:0> Father.ancestors.include?(Person)
=> true

The attached patch corrects the code to use #ancestors.include? and adds corresponding tests.

I've tested MySQL 5.0.37, PostgreSQL 8.2.3, SQLite 2.8.17, and SQLite 3.3.13. Only MySQL did not exhibit the issue, all three others did. The included test case passes on all four databases.

Attachments

fix_fixtures_column_type_detection.diff (30.5 kB) - added by roderickvd on 04/03/07 15:06:58.
ora_binary_fixtures.patch (4.6 kB) - added by mschoen on 05/26/07 04:17:47.
small_flower_fixture.diff (38.3 kB) - added by roderickvd on 05/28/07 10:00:18.
flowers_small.jpg (5.7 kB) - added by roderickvd on 05/28/07 10:01:23.
Fixture data < 7 KB should work on Microsoft SQL Server
complete_binary_fixtures_patch_including_small_flower_image.diff (39.5 kB) - added by tomafro on 07/03/07 12:18:34.
Patch including binary changes and sql server adapter fix
sqlserver_binary.patch (1.3 kB) - added by lawrence on 10/17/07 13:09:46.
sqlserver_edge_binary.patch (38.0 kB) - added by lawrence on 10/17/07 13:11:42.

Change History

04/03/07 15:06:58 changed by roderickvd

  • attachment fix_fixtures_column_type_detection.diff added.

04/18/07 17:35:54 changed by ctm

  • cc set to ctm.

I came up with the same use of ancestors? as is in roderickvd's patch. I was alerted to the problem by reading Peter Donald's workaround.

Not wanting to detect binary data by looking for NULs, I looked into the problem and found the same thing roderickvd did. The existing test of "klass.kind_of?(ActiveRecord::Base)" is clearly in error.

05/21/07 16:42:45 changed by vesaria

  • summary changed from [PATCH] Fix column type detection while loading fixtures to [PATCH] Fix column type detection while loading fixtures [TINY].

I can confirm that this simple patch fixed problems loading binary fixtures for me as well (Rails 1.2.3, Postgres 8.2, Windows).

The current code is clearly a mistake. This one line patch fixes it, has good test cases, and is well thought out. I vote it be accepted into the code.

Since it's only one line, I'm adding the [TINY] tag.

05/21/07 18:35:17 changed by kevinclark

  • keywords set to tiny.
  • summary changed from [PATCH] Fix column type detection while loading fixtures [TINY] to [PATCH] Fix column type detection while loading fixtures.

05/21/07 18:48:43 changed by kevinclark

Tests all pass on sqlite3. Code and test look good. +1

05/21/07 18:54:54 changed by rick

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

(In [6798]) Fix column type detection while loading fixtures. Closes #7987 [roderickvd]

05/21/07 20:49:05 changed by technoweenie

  • status changed from closed to reopened.
  • resolution deleted.

This breaks oracle and ms sql. Would a use of either db be kind enough to submit a working patch for those?

05/21/07 21:34:21 changed by roderickvd

I don't have either server to test it on, but am pretty sure it breaks Microsoft SQL because the fixture is larger than the 7 KB limit than DBI allows. (The fixture is 19 KB.)

I'm not sure what's happening to Oracle; the test failures aren't very explanatory.

The easy route seems to be changing the fixture to a smaller one. I can do this, but cannot be 100% sure that this will work (Microsoft SQL experts anyone?). Let me know.

05/21/07 21:54:50 changed by kevinclark

Go ahead and change the fixture file. If you email the core list pointing here and asking Oracle or MSSQL users to test the new patch, you should get a response.

05/25/07 17:55:24 changed by mschoen

The Oracle issue isn't one of length -- it's that writing blob's is currently done in AR using an after filter, because it requires a 2nd write to the db. And that's not being done through fixtures, so the fixtured blob doesn't actually get written to the db.

I'm looking into fixing that; failing that I'll just submit a patch to skip this test.

05/26/07 04:17:31 changed by mschoen

Patch attached to resolve the Oracle issue.

05/26/07 04:17:47 changed by mschoen

  • attachment ora_binary_fixtures.patch added.

05/26/07 06:26:54 changed by bitsweat

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

(In [6859]) Oracle binary fixtures; pull fixture insertion into the adapters. Closes #7987.

05/26/07 06:41:36 changed by bitsweat

  • keywords changed from tiny to tiny fixtures binary.
  • status changed from closed to reopened.
  • resolution deleted.

Oops, SQL Server is still broken.

05/26/07 08:48:45 changed by roderickvd

I'll update that fixture Monday at the latest (but not today).

05/28/07 10:00:18 changed by roderickvd

  • attachment small_flower_fixture.diff added.

05/28/07 10:01:23 changed by roderickvd

  • attachment flowers_small.jpg added.

Fixture data < 7 KB should work on Microsoft SQL Server

05/28/07 10:02:40 changed by roderickvd

Any Microsoft SQL Server users please confirm if the two patches above work for you.

07/03/07 11:07:07 changed by tomafro

Sorry this has taken me so long. I've tested the patch and found a single binary failure. I'm looking into the cause now.

07/03/07 12:18:34 changed by tomafro

  • attachment complete_binary_fixtures_patch_including_small_flower_image.diff added.

Patch including binary changes and sql server adapter fix

07/03/07 12:20:35 changed by tomafro

I've added a new patch against edge, which fixes SQL Server, can be run on windows and includes the small_flower.jpg image.

07/05/07 02:34:49 changed by fotos

  • cc changed from ctm to ctm, fotos.

07/12/07 22:38:47 changed by vesaria

Looks like this patch is ready to be applied.

10/10/07 18:27:32 changed by vesaria

Core: What else does this patch need? It looks like it already has the testing and peer verification by 3 people.

10/16/07 03:22:53 changed by vesaria

  • keywords changed from tiny fixtures binary to tiny fixtures binary verified.

10/17/07 13:09:46 changed by lawrence

  • attachment sqlserver_binary.patch added.

10/17/07 13:11:42 changed by lawrence

  • attachment sqlserver_edge_binary.patch added.

10/17/07 13:14:35 changed by lawrence

(In [7953]) SQL Server fix for binary tests. References #7987 [roderickvd]

10/17/07 13:17:12 changed by lawrence

+1

This also fixes the failure of the binary test for MySQL (on Windows).

Last steps needed: - apply patch sqlserver_edge_binary.patch - put attached flowers_small.jpg in /activerecord/test/fixtures - remove /activerecord/test/fixtures/flowers.jpg

10/17/07 21:35:50 changed by nzkoz

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

(In [7958]) Use a smaller binary fixture so we don't overflow column size limitations in some commercial / legacy databases. Closes #7987 [roderickvd, lawrence]