From 317dcad677382c876ee9360ba662a56e2efc04c3 Mon Sep 17 00:00:00 2001 From: Jordan Ritter Date: Thu, 8 Sep 2011 22:00:09 -0700 Subject: [PATCH 1/2] Introduce failing specs for custom types migration bug. When dm-migrations' dm-do-adapter runs, Adapter#property_schema_hash is invoked on each property to generate the SQL for it. For Property::Text, type_map[Property::Text] yields a schema of TEXT with no :length property. When DM encounters a String primitive whose length exceeds the schema's capacity, it auto-adjusts the schema primitive to compensate (i.e. in MySQL, {SHORT,MEDIUM,LONG}TEXT). Result: MEDIUMTEXT == AWESOME. The case is different for (1) a custom Property derived from (2) a builtin Property whose schema primitive changes based on the Property's size options. For Property::Json, the first type_map[property.class] lookup is nil because custom types can't/don't update Adapter#type_map -- custom properties can't know what model/repository/adapter they're going to be on at definition time, which they would need because the type_map is stored on the adapter *class*. So, the second lookup type_map[property.primitive] kicks in, which for Property::Json is type_map[String]. That in turn yields a schema of VARCHAR with a :length property. As with Property::Text, when DM encounters a String primitive whose length exceeds the schema's capacity, it auto-adjusts the schema primitive to compensate (i.e. in MySQL, {SHORT,MEDIUM,LONG}TEXT). However, when dm-migrations encounters any property_schema_hash with a :length option, it automatically appends "(%i)" % length to the SQL statement. Result: MEDIUMTEXT(123412341234) == entire migration FKD. --- spec/fixtures/custom.rb | 20 ++++++ .../integration/custom_type_migration_spec.rb | 66 +++++++++++++++++++ 2 files changed, 86 insertions(+) create mode 100644 spec/fixtures/custom.rb create mode 100644 spec/integration/custom_type_migration_spec.rb diff --git a/spec/fixtures/custom.rb b/spec/fixtures/custom.rb new file mode 100644 index 0000000..98f8cde --- /dev/null +++ b/spec/fixtures/custom.rb @@ -0,0 +1,20 @@ +module DataMapper + module TypesFixtures + + class CustomWithoutOptions + include ::DataMapper::Resource + + property :id, Serial + property :text, Text + property :json, Json + end + + class CustomWithOptions + include ::DataMapper::Resource + + property :id, Serial + property :text, Text, :length => 2**24-1 + property :json, Json, :length => 2**24-1 + end + end +end diff --git a/spec/integration/custom_type_migration_spec.rb b/spec/integration/custom_type_migration_spec.rb new file mode 100644 index 0000000..2032a24 --- /dev/null +++ b/spec/integration/custom_type_migration_spec.rb @@ -0,0 +1,66 @@ +require 'spec_helper' + +# When dm-migrations' dm-do-adapter runs, Adapter#property_schema_hash is invoked +# on each property to generate the SQL for it. +# +# For Property::Text, type_map[Property::Text] yields a schema of TEXT with no +# :length property. When DM encounters a String primitive whose length exceeds +# the schema's capacity, it auto-adjusts the schema primitive to compensate +# (i.e. in MySQL, {SHORT,MEDIUM,LONG}TEXT). Result: MEDIUMTEXT == AWESOME. +# +# The case is different for (1) a custom Property derived from (2) a builtin +# Property whose schema primitive changes based on the Property's size options. +# For Property::Json, the first type_map[property.class] lookup is nil because +# custom types can't/don't update Adapter#type_map -- custom properties can't know +# what model/repository/adapter they're going to be on at definition time, which +# they would need because the type_map is stored on the adapter *class*. +# +# So, the second lookup type_map[property.primitive] kicks in, which for +# Property::Json is type_map[String]. That in turn yields a schema of VARCHAR +# with a :length property. As with Property::Text, when DM encounters a String +# primitive whose length exceeds the schema's capacity, it auto-adjusts the schema +# primitive to compensate (i.e. in MySQL, {SHORT,MEDIUM,LONG}TEXT). However, when +# dm-migrations encounters any property_schema_hash with a :length option, it +# automatically appends "(%i)" % length to the SQL statement. Result: +# MEDIUMTEXT(123412341234) == entire migration FKD. +# + +require './spec/fixtures/custom' + +try_spec do + + describe 'a model with a builtin property whose schema primitive changes according to size options and a custom property based on it' do + + supported_by :postgres, :mysql, :sqlite, :oracle, :sqlserver do # basically all SQL/DO-based + + { + ::DataMapper::TypesFixtures::CustomWithoutOptions => "without any specified property options", + ::DataMapper::TypesFixtures::CustomWithOptions => "with specified property options that shift the schema primitive", + }.each do |klass, desc| + + describe(desc) do + let(:model) { klass } + + context 'when migrated' do + it 'should use the same property_schema_hash as its logical parent' do + proc { model.auto_upgrade! }.should_not raise_error(DataObjects::SQLError) + + adapter = model.repository.adapter + + json_hash = adapter.send(:property_schema_hash, model.json) + text_hash = adapter.send(:property_schema_hash, model.text) + + json_hash.delete(:name) + text_hash.delete(:name) + + json_hash.should == text_hash + end + end + end + + end # each test + + end # supported_by + end # describe: model +end # try_spec + From e13b18372a23cfd8697a02dab8b0082a849cacfc Mon Sep 17 00:00:00 2001 From: Jordan Ritter Date: Thu, 8 Sep 2011 22:25:36 -0700 Subject: [PATCH 2/2] Point dm-migrations at git/branch for fix of custom property migration. --- Gemfile | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Gemfile b/Gemfile index ca6c58d..ac12765 100644 --- a/Gemfile +++ b/Gemfile @@ -59,8 +59,10 @@ group :datamapper do gem "dm-#{adapter}-adapter", DM_VERSION, SOURCE => "#{DATAMAPPER}/dm-#{adapter}-adapter#{REPO_POSTFIX}" end + gem "dm-migrations", :git => "#{DATAMAPPER}/dm-migrations.git", :branch => "custom_type_migration" + plugins = ENV['PLUGINS'] || ENV['PLUGIN'] - plugins = plugins.to_s.tr(',', ' ').split.push('dm-migrations').uniq + plugins = plugins.to_s.tr(',', ' ').split.uniq plugins.each do |plugin| gem plugin, DM_VERSION, SOURCE => "#{DATAMAPPER}/#{plugin}#{REPO_POSTFIX}"