search

rsim / oracle-enhanced

Oracle enhaced adapter for ActiveRecord
MIT License
544 stars 308 forks source link

Make insert return an array, and include the id_value if super returns nil (12) #2381

Open andynu opened 3 months ago

andynu commented 3 months ago

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

mattalat commented 2 months ago

Hi @andynu, thank you for your PRs. I had a few questions.

  1. What is the intention of the 12 separate MRs? Do they provide different changes or are they all stacked in this twelfth one?

  2. There is a code comment that mentions needing to figure out if a column is a PK/auto-incremented. In https://github.com/rsim/oracle-enhanced/issues/2380 you mention that this may have been fixed upstream. Is it still an ongoing concern?

  3. Regarding your notes on the structure of the abstract adapter’s #insert method, do you have a scenario to which you could direct me that showcases this behavior? The method’s documentation mentions it could return an Array of values if specific conditions are met, but I wasn’t sure if they are in Oracle. And, if it does seem like a bug on the adapter’s end, do you know if anyone has identified it in the Rails repo?

Thanks for your time

andynu commented 2 months ago

Hi @mattalat

  1. The Stack of PRs

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. https://github.com/andynu/oracle-enhanced/compare/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).

  1. insert returning the id

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 Array(super || id_value) which I think is really working around a bug in rails, where super should be providing an array itself when an id_value is provided, but it doesn't it just returns whatever id_value is passed in, even though its api has changed to always expect an array to come out of it. I do think a better workaround involves adding support for OracleEnhanced::Column.auto_incremented_by_db?. I would put a pin in this one until the other fixes are sorted out, and then it'd be great to have a second set of eyes verify this fix /wrt issue #2381 and the upstream changes.

  1. test failures related to this pr

You can definitely see the failure. If you check out c0439811da4bc7f8cb310db78bb69c9be9dd3c69 (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.