[PATCH] omnibus Oracle patch, fixes issues with sequences, camelCase, tests, and synonyms

Patch file (from svn diff)

Won't "change the default sequence name from rails_sequence to #{table_name}_seq" break backwards compatibility? (We can't really do that)

It will break it, but this is how it should be in AR for oracle. If you go into any oracle shop and tell them they need to do it with rails sequence, you wont be using rails. The only options I see are

1. dont change (bad) 2. Implement patch as is existing apps can use set_sequence_name and itll be fine 3. make the default sequence name changeable so you could set the default to rails_sequence or tablename_seq ??

Yeah, I'd have to agree with Sam here. We're pretty much an oracle-only shop, and having one global sequence is what prompted me to write my portion of this patch. We just can't use rails with a global sequence.

And with AR::Base.set_sequence_name it can be overridden to go back to rails_sequence if need be. It is just a matter of setting that up in the models. This really is a 1.0 feature for those of us that are using oracle. Especially for any sort of existing database support. I know that is not necessarily the biggest priority for Rails, but it does make it nice.

Plus, there's going to have to be some pain, at some point, for people using oracle. Either everyone who does things the oracle way, creating sequences and all that, will be defining every sequence name in every table, or this change gets made, and the set of folks who are using rails_sequence makes the change now. From my perspective, I think it makes sense to play nice with the way most oracle folks are used to working, cause a little pain now for OCI/rails_sequence users, but gain much broader appeal in the long run.

If, in the short term, you on the development team feel strongly about backwards compatibility, then perhaps you could accept part of this one, patch 1751, which at least allows the setting of the sequence name? That would let me, and others who use oracle's features, use rails with oracle.

caleb

Clearly I agree with Caleb and Sam (since I submitted the patch). We're on the Rails train regardless, but I'd like to see Rails fit better into a typical Oracle environment as of the 1.0 release.

The upgrade path to retain compatibility is also really easy. Either:

a. I expect most folks using Oracle ALREADY have sequences named #{table_name}_seq, so they'll just breathe a sigh of relief and drop the rails_sequences they had previously been forced to use.

b. If they prefer the rails_sequence, all they need to do is add, to the end of environment.rb:

ActiveRecord::Base.set_sequence_name = "rails_sequence"

Is absolute compatibility with previous releases without any changes (even the single line in option b above) by the developer is a requirement? I didn't think it was until POST 1.0, which is why I'm hoping to get this patch accepted prior to that milestone.

In my opinion, Oracle is not a functional Rails adapter without this patch. I have a number of tables that need sequences, and they all use 'id' as their primary key name. Unless I'm missing some big piece of documentation somewhere, the existing oci adapter does not allow this. Sequences are global, not table-specific.. You can only have one sequence called 'rails_sequence' If it were up to me, I would mark this as a 'blocker' for Rails 1.0

That's the kind of feedback I was hoping for :). Okay, I'll trust your judgement here and send anyone screaming your way. Thanks for spending the time to make Rails great on Oracle.

Alternatively you could use:

  ActiveRecord::Base.set_sequence_suffix="seq"

Full backwards compatibility if you do it that way. Having said that, I don't believe there are too many rails applications in use at present, so perhaps a little breakage isn't so bad.

Is there any reason not to do a set_suffix?

If full backwards compatibility were required, I'd prefer the approach of having the patch as is, except have the sequence name default to "rails_sequence", and then achieve the desired effect with:

ActiveRecord::Base.set_sequence_name = "#{table_name}_seq"

This would retain the full flexibility of allow non-standard sequence names (whereas the suffix approach would be more limited).

However, I feel strongly that Rails will be better positioned for success in Oracle environments if the default is consistent with how most Oracle shops operate. Ie., I think that compatibility with the potential base of Rails/Oracle folks is more important at this point that with the current base of Rails/Oracle shops.

And perhaps more important, DHH seems to agree. ;-)

Ok, My objections are registered, but remember, david's going to send the complainers your way ;).

Perhaps you guys could verify and judge a few more members for the omnibus? Please have a look at #1807, #1604 (and extending it to include all the Rakefile tasks), and #714. Thanks, guys.

Now includes the code from ticket 1807

Per David's suggestion, I've added in the code from ticket 1807 to this patch. I've got a super-busy weekend here, but I will snag the code from the 714 as well and take a look at it, though if anyone else thinks they can get it done before monday, please feel free to take a crack at it, as I can't make any guarantees of free time today or tomorrow!

caleb

I've attached version 3 of the omnibus patch, which closes #1624 and #714, along with notes for the changelog.

With David's gentle prodding, I think we've helped make Oracle a full-fledged citizen in the Rails workd.

Patch file

Proposed changelog

I've applied the patch with a few stylistic changes. Please rerun the tests and make sure everything still works.

Checked out the trunk again -- couldn't find the stylistic changes, but everything still seems to work. Thanks!

I am concerned that a portion of the patch from 1784 was lost in translation. Specifically, the part where the reference to user_tab_columns was replaced by a more complicated (but correct) reference to all_tab_columns. I know from using the current OCI adapter (0.13.1), that it is STILL referencing user_tab_columns

Here is the relevent patch code I found from 1784:

Index: activerecord/lib/active_record/connection_adapters/oci_adapter.rb
===================================================================
--- activerecord/lib/active_record/connection_adapters/oci_adapter.rb	(revision 1860)
+++ activerecord/lib/active_record/connection_adapters/oci_adapter.rb	(working copy)
@@ -155,7 +155,11 @@
         def columns(table_name, name = nil)
           cols = select_all(%Q{
               select column_name, data_type, data_default, data_length, data_scale
-              from user_tab_columns where table_name = '#{table_name.upcase}'}
+              from user_catalog cat, user_synonyms syn, all_tab_columns col
+              where cat.table_name = '#{table_name.upcase}'
+              and syn.synonym_name (+)= cat.table_name
+              and col.owner = nvl(syn.table_owner, user)
+              and col.table_name = nvl(syn.table_name, cat.table_name)}
           ).map { |row|
             OCIColumn.new row['column_name'].downcase, row['data_default'],
               row['data_length'], row['data_type'], row['data_scale']

The patch was applied, but AFTER 0.13.1 -- you can use the patched version by either running on edge rails, or by using the latest beta gems.

The next official release (expected soon) will include the patch as well.

Keep a good job up! http://quick-adult-links.com

dan river bed in a bag discount man cologne jewelry and watches pink prom dresses matisse shoes nike shox running shoes steel toe boots womens athletic shoes dr martens boots ecco shoes sale aero bed twin indoor soccer shoes hawaiian wedding dress sheepskin boots metal frame futon bunk bed candies shoes vintage dresses dada basketball shoes twin deluxe aero bed watches omega

tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet tatunet