activerecord-jdbc-adapter icon indicating copy to clipboard operation
activerecord-jdbc-adapter copied to clipboard

`MyModel.my_column = "null"` should be recognized as a String, not NULL

Open betesh opened this issue 6 years ago • 10 comments

I'm in the process of upgrading a Rails app from 3.2 to 4.0 and there are some new test failures because Rails now type casts on assignment, i.e.:

MyModel.columns_hash.fetch('my_column').type # => :string

# Rails 3
MyModel.new(my_column: 3) # => my_column is set to 3, an Integer

# Rails 4
MyModel.new(my_column: 3) # => my_column is set to '3', a String

This means that attribute assignments now call the type_cast method, resulting in this behavior:

MyModel.columns_hash.fetch('my_column').type # => :string

# Rails 3
MyModel.new(my_column: 'null') # => my_column is set to 'null', a String

# Rails 4
MyModel.new(my_column: 'null') # => my_column is set to nil

This behavior is undesirable in my opinion. I should be able to put the String 'null' into the database.

This is the result of https://github.com/jruby/activerecord-jdbc-adapter/blob/v52.0/lib/arjdbc/db2/column.rb#L50.

I traced this back through git history and didn't find a clear explanation for this design decision (for instance, 2cfa4f3ea893433260fac64e088141442549a1db doesn't tell me anything about why String that match /^\s*NULL\s*$/i are now type-cast to nil). HSQLDB used to do something similar until be172b1adfe76f00d9e441ce7d51b582800693bf and it's not clear why it used to do that or why it was removed.

With all that in mind, before I try working on a PR, here are my questions:

  1. What is the rationale for this?
  2. Are there any DB's that don't support storing the value "NULL" (or anything that matches /^\s*NULL\s*$/i) as a column value?
  3. If I submit a PR and it is merged, will it be backported into 1-3-stable? (Unfortunately, it will be a long time until I upgrade to Rails 5)

betesh avatar Sep 05 '18 20:09 betesh

with Rails 3/4 pretty much dead I am closing this one.

kares avatar Apr 30 '19 12:04 kares

@kares Did you confirm that this is not an issue in versions compatible with rails 5 and 6? Can you shed some light on the rationale behind the change here?

betesh avatar Apr 30 '19 13:04 betesh

did not, sorry. AR-JDBC doesn't support DB2. I can re-open if it works differently than with MRI on AR 4+

kares avatar Apr 30 '19 13:04 kares

What do you mean by "doesn't support DB2"? I still see this issue in v52.2: https://github.com/jruby/activerecord-jdbc-adapter/blob/v52.2/lib/arjdbc/db2/column.rb#L53

betesh avatar Apr 30 '19 13:04 betesh

I can re-open if it works differently than with MRI on AR 4+

It seems to me that it works differently than all the other adapters in this project. Isn't that enough of a reason to re-open? At the very least, I'd still like to understand the reason behind this breaking change.

betesh avatar Apr 30 '19 13:04 betesh

the files are there but the DB2 adapter likely won't work with AR 5.x (no much work was done)

It seems to me that it works differently than all the other adapters in this project.

that might not be unusual at all - all of the DB dialects have its specifics. feel free to work on it, the issue was stale for months - hopefully it won't end up the same after a few more

kares avatar Apr 30 '19 13:04 kares

the issue was stale for months

That's because my initial comment ask for feedback before I start working on a PR. So just to make sure we're on the same page, it sounds like your answers to my original 3 questions are:

What is the rationale for this?

None; I can change this back to the way it worked with Rails 3

Are there any DB's that don't support storing the value "NULL" (or anything that matches /^\sNULL\s$/i) as a column value?

It does't matter, since this issue is specific to the DB2 adapter, and DB2 does support the string "NULL"

If I submit a PR and it is merged, will it be backported into 1-3-stable? (Unfortunately, it will be a long time until I upgrade to Rails 5)

No, since Rails 4 is no longer supported. If that's the case, then I may not be able to work on this until the Rails app I work on is running rails 4.2 and we begin upgrading to 5.0. I can't promise that will be in the next few months--the upgrade to 4.1 is still in progress.

betesh avatar Apr 30 '19 14:04 betesh

If I submit a PR and it is merged, will it be backported into 1-3-stable?

you should target that branch. might stay un-released for a while (and maybe even forever) but if smt serious comes up and we need to do a release (really no promises here) it will get in. sorry about not being able to support the old adapters better but its pretty much all a volunteer effort.

kares avatar May 02 '19 07:05 kares

so you don't intend to fix this in master as well?

betesh avatar May 02 '19 17:05 betesh

@betesh A PR which works for Rails 5 and db2 we will accept but DB2 is in limbo support wise due to lack of human resources. There have been some fixes applied to DB2 for Rails 5 recently but until we see more "new" commitment from individuals (or businesses) to wanting to support it we are in the position of landing those changes but not actually releasing it officially.

At this stage 5.0+ (50.0+) we support mysql, postgresql, and sqlite. In the near future we have two serious groups who will end up supporting sqlserver. We do have some interest in db2 but it is not quite enough for us to say we can support it. Also note: 5.x support means doing testings and fixes for branch 50-stable 51-stable, 52-stable and master (eventually 60.x). Across each version the deltas are not big but if we offer support we want to support all versions Rails supports. Actually I should put a caveat that if someone was serious and only wanted support from, let's say, 5.2+ then that is also ok too.

enebo avatar May 02 '19 19:05 enebo