Make insert return an array, and include the id_value if super returns nil (12) #2381
Conversation
Introduce the _connection method which returns whichever is defined between @unconfigured_connection or @raw_connection.
verify! ensures that @raw_connection is defined.
Rails commit bd19d1baf1 [1] added a check of logger.level which was not supported by the MockLogger. This commit adds a mock level method. And limits the missing_method to respond only to logging calls, so any other calls will raise a NoMethodError to make future debugging easier. [1] rails/rails@bd19d1b
The SchemaMigration object no longer inherits from ActiveRecord::Base. See "Move SchemaMigration to an independent object" rails/rails@436277d Several methods were renamed, and are no longer available as class methods on SchemaMigration, but as an instance of SchemaMigration per connection. The object per connection seems to remove the need to `reset_table_name` in the tests. That method is not available on the new object.
Rails commit e6da3ebd6c6 [1] introduces ColumnDefinition validations for the keywords used in database commands. This commit adds the additional keywords supported by this gem. [1] rails/rails@e6da3eb
andynu
changed the title
andynu
force-pushed
the
stack-12-sql-statement-args
branch
from
8da032a
to
1d80f71
Compare
See rails/rails@07d2407 Both `table_name` and `column_name` were removed in favor of doing o.left in the visitor pattern. This is just mimicing the adjustments from upstream.
andynu
force-pushed
the
stack-12-sql-statement-args
branch
2 times, most recently
from
ffc39e0
to
fb1aae6
Compare
This extends the existing class level auto_retry functionality to
optionally allow an allow_retry boolean kwarg to allow retry of specific
queries.
This PR preserves the requirement that `autocommit?` be enabled which is the
safer choice.
```
def with_retry(allow_retry: false) # :nodoc:
should_retry = (allow_retry || self.class.auto_retry?) && autocommit?
```
See issue rsim#2310
See rails/rails@3682aa1 So the shortening of index names that was added to rails itself was almost immediately changed to use an 'idx_' prefix instead of an 'ix_' prefix. See rails/rails@59f2b06 However, now that rails does shortening. It is an opportunity to simplify this library and just rely on the rails index name shortening.
DEPRECATION WARNING: Passing the class as positional argument is deprecated and will be removed in Rails 7.2.
Please pass the class as a keyword argument:
serialize :serialized, type: Array
(called from <class:TestSerializedColumn> at /home/andy/src/oracle-enhanced/spec/active_record/connection_adapters/oracle_enhanced_adapter_spec.rb:446)
Rails removed the table_name alias to Arel::Table.name See rails/rails@1d98bc5
In rails commit rails/rails@942785f A method `internal_exec_query` is introduced as the implementation for exec_query. For now this just aliases one to the other.
andynu
force-pushed
the
stack-12-sql-statement-args
branch
from
fb1aae6
to
e58a68a
Compare
This is to accomodate rails commit rails/rails@3421e89 The trouble is primary keys are not returned with this code. The Column#auto_incremented_by_db? method would have to return true for any column with an associated sequence, or perhaps marked as the primary key.
…s nil The super is not returning the id_value if it is available (as it used to). Furthermore ActiveRecord::Persistence#_create_record expects an array of values.
andynu
force-pushed
the
stack-12-sql-statement-args
branch
from
e58a68a
to
e08c0d9
Compare
|
Hi @andynu, thank you for your PRs. I had a few questions.
Thanks for your time |
|
Hi @mattalat
The purpose of the stack of PRs is to show passing tests along the way. That does mean that each one in the stack contains the changes from the previous, and that this last one contains all the changes. As I mentioned in the rails 7.1 upgrade issue, I'd be happy to re-form these PRs as independent PRs instead of a stack, but with those you would not be able to see the passing tests because unresolved issues from other PRs would still be breaking the tests. There are two ways to go about reviewing these changes. First we can go in order, that first PR is a minimal change on top of master, and if/when that one were merged in then the second PR in the stack would become simplified to just another small change. However, I acknowledge that some of these PRs may not be the implementation that the maintainers would like to use, in which case I would look at their fixes to rebase subsequent PRs on their changes. If you'd like the unstacked diff for any of these you need to do a comparison with the previous PR e.g. andynu/oracle-enhanced@stack-09-internal_exec_query...andynu:oracle-enhanced:stack-12-sql-statement-args (sorry that the branch names are not nicely numerically sequential, the bisecting to produce this stack of PRs was a challenging journey with many false starts and backpedaling).
So the issue in #2380 has been fixed in rails 7.2, and backported to 7.1. However, in this PR in DatabaseStatements#insert you can see
You can definitely see the failure. If you check out c043981 (That's one commit back in this PR), then you'll see a pile of test failures that illustrate the issue. I tried to have the meat of each of these PRs start with a version bump, and then a fix for the test failures that showed up. My stack of PRs here was explicitly chasing test failures of tests that already exist in this library as I bumped the rails version closer and closer to 7.1 stable. |
Make insert return an array, and include the id_value if super returns nil
ActiveRecord::ConnectionAdapters::OracleEnhanced::DatabaseStatements#insert
The super here is not returning the id_value if it is available (as it used
to). Furthermore ActiveRecord::Persistence#_create_record expects an
array of values.
I don't think this is the right change. In fact I think that the original abstract
ActiveRecord::ConnectionAdapters::DatabaseStatements#insert
may not be handling id_value correctly. The method appears to sometimes return
an array, and other times return a singular id_value.
However, I do think this highlights the crux of supporting rails 7.1 in this library as with this little tweak, all the tests passing against rails 7-1-stable.
This fixes #2380