Add support for MySQL unsigned integer and float columns

[JasonLunn]

Intended to resolve #373

[kares]

Thanks Jason, would you mind looking at the test failures against 2.3 and 3.0 ? If not I will but it will take a while ...

[JasonLunn]

Part of the problem is almost certainly that the TableDefinition isn't being found in the 2.3/3.0 case. I struggled with that part even in the 4.0 case before I tried to parrot what I saw in lib/arjdbc/postgresql/adapter.rb in terms of how to make the register the TableDefinition class. Do you what the best practice is for custom TableDefinitions?

[kares]

I'm more than fine with disabling the tests on <=3.0 just use the TestCase helper and only run it if ar_version('3.1') ...

[kares]

... and if the unsigned types are only in AR 4.0 native types should only be updated for that AR version (see postgres)

[JasonLunn]

@kares This now run cleans for me locally on 2.3 through 4.0

[kares]

Thanks Jason, consider this merged than - might take a bit cause I'm currently stack locally with some code ...

[JasonLunn]

Thank you, @kares - I appreciate your quick responses and assistance in getting this ready for acceptance

[JasonLunn]

Travis seems stuck. It has had 1 pending test for 23 hours....

[JasonLunn]

How does one restart a test that has been stuck in 'pending' for a day?

[kares]

I'm not really sure, anyways it somehow ended green :)

I just realized this is smt not supported by Rails, or am I wrong (I really thought they do) ?
If so there likely might be some issues e.g. our dumps will be different and not compatible with MRI ones, now I'm all for it but this might be an issue for existing users - maybe I will disable this by default and provide a way to enable support for unsigned types if you have nothing against it ?

[JasonLunn]

If you dump an existing schema that contains an unsigned integer column, pattern matching for /integer/ will result in the schema.rb expressing the column as a normal, signed integer, so subsequent recreations of the schema (like db:test:load) will make an signed integer column.

If you generated schema.rb with this patch included, but then tried to load that schema.rb under MRI, yes, I think it would break, unless I were to go make a parallel patch for them as well. How likely is that use case?

What I would have loved to do would be to add support for a new hash key :unsigned in the column options, but I think I'd have to convince ActiveRecord to change some method signatures in order to achieve that.

[kares]

I see, that :unsigned column option sound about right - should be possible without AR changes ... I guess

[JasonLunn]

I can see how I could modify type_to_sql easily enough to do the right thing when :unsigned => true was passed as a column option, but the hurdle is altering how ActiveRecord::SchemaDumper would render the schema.rb. The table() method, which is what write outs all the column definitions, is huge and doesn't offer a way to add arbitrary new hash keys. It has fixed support for name, limit, precision, scale, null and default. I don't see a way to introduce an :unsigned hash key in the output without duck punching the entire method, copying all 84 lines of the existing method and adding support for a new hash key. That didn't sound DRY enough for serious consideration. Am I missing an extension point that I should be considering?

I'm open to any thoughts you have on how to make this behavior conditional for those that want it.

[kares]

I see, thanks for looking into it and explaining Jason ... will sure look at it - if we can't do it easily as you mentioned than your approach is probably for the best, I'll just add some helper code to turn it on while being off by default above it.

[JasonLunn]

Anything I can do to restart the momentum to get this accepted, @kares?

[kares]

I'm sorry Jason, I was stuck with other mostly 'native' compatibility issues - somehow this is lower priority since it's some "new" feature ... I'll try to squeeze some time (I really wanted to look into related AR internals) but I'm not yet sure when.

[kares]

It really is an "option" of INT/FLOAT numeric type to be unsigned ... thus acting as a "new" type is not really ideal.
Seems it would not require monkey-patching (the almost 100 lines on 3.2) of SchemaDumper's #table but only on 4.0 but I guess you'd like to see this for 3.2 as well ?

[JasonLunn]

Yeah I need a solution that works on 3.2.x. If you want to see a patch of SchemaDumper#table to support a new option I'm willing to provide it, but I as I said before, it doesn't seem DRY enough for serious consideration.

[kares]

Here's how I'm thinking: t.integer :unsigned => true is the "logical" way of doing this from a user's perspective, now the AR API is quite far from being OOP-ish from time-to-time, but the user does not need to know or care about that (as long as it is not used directly). If we add this as is currently - using "new" types such as t.unsigned_integer we'll likely need to support it (even if Rails implements it differently eventually). Thus I would vote for rather doing it the non-DRY but user-intuitive way. That being said it's going to get really messy since you'll need to support patches for all the AR versions we do support currently (not to mention the cost of maintaining it - but if Rails adds it eventually it can be removed).

In my opinion this really seems like a discussion for (core) AR - I do not mind adding it here but since it requires quite a lot of hacks it's probably best to be avoided or added directly where the code to be "hacked" is from a.k.a. Rails master :)

If you still feel that you want it in AR-JDBC adding this as an optional feature - since it will require quite some monkey-patching e.g. in a separate unit arjdbc/mysql/unsigned_types.rb should be considered ...

Hope my concerns are understandable, I really appreciate your attempt to contribute but I'm not sure it's the right place, now that I have look at this a bit more.

[kares]

I'm closing this PR for now there's a more general feature #423 to add (optional) column options.
We'll see how and if it goes, hope you can work-around till that time easily. Thanks again for pointing this out!