This repository has been archived by the owner on Apr 17, 2018. It is now read-only.
Custom type migration bugfix #45
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
|
@jpr5 just for the record, this issue will be addressed in 1.3.0. I'm working on the improved property API which is already merged in to master in dm-core, dm-types and dm-migrations (see my recent commits in those repos) |
|
@solnic That's great. Does that mean you're saying don't merge this pull request in the meantime? The fix itself is 1 line (in dm-migrations). |
|
@jpr5 ok so as I commented on dm-migrations' pull request it's already merged in. I didn't merge this pull request though as the specs you added fail for me. Let's work this out for 1.2.1 |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Simple enough use case:
Json property needs more than 64k, so adjust the :length the same way you do Text's. b00m on migration.
My solution was to add an additional dm-migrations' Adapter#type_map lookup for property.class.superclass, before falling back to the underlying DM primitive.
This will solve the vast majority of cases, like Json < Text. It doesn't solve for any cases of inheritance depth from builtin type > 1: MoreThanJson < Json. (MoreThanJson.class.superclass won't be in Adapter#type_map's table.) I argue that a practical 99% is better than a real 0%.
More detailed explanation in both the commits and the integration spec I provided for dm-types.
FYI merging this pull will require 2 more steps: