Add ActiveRecord.reload_attributes

[abaldwin88]

Motivation / Background

PR #48241 detects when certain auto-populated columns should be appended to a RETURNING clause. However, in certain cases it may be necessary for the application to explicitly specify additional columns. e.g. Database Triggers.

This feature was discussed in #45736 and briefly with Matthew Draper in the Discord channel.

Detail

This PR adds the ability for the application to specify those attributes with the reload_attributes class method.

Additional information

Additionally a very minor edge case was handled. The application could try to assign a value to a virtual column which essentially results in a no-op. The values persisted in the database would be the same regardless. However, in doing so the value from RETURNING wasn't being reflected on the instance attributes after create. See the newly added test and removal of the _read_attribute check.

Performing this nil check with _read_attribute will also not be compatible when RETURNING is implemented on UPDATE. As it is expected that the instance's attributes will be prepopulated with non-nil values. (See related PR here: #48628)

---

Paging: @nvasilevski

[abaldwin88]

Build error looks like a transitory problem with Sauce Labs throttling.

09 06 2023 17:22:45.742:ERROR [launcher.sauce]: Can not start microsoftedge 17.17134 (Windows 10)
  Failed to start Sauce Connect:
  'create tunnel' request failed with: URL https://saucelabs.com/rest/v1/rubyonrails/tunnels - 400 (Bad Request) Error: HTTP request failed. Server response: 400: Too many active user tunnels: 10 >= 10 (ref: 0872d2db).

[nvasilevski]

Implementation looks correct to me! I believe this is close to what @matthewd had in mind

However, if I'm not mistaken our long-term intention to have this feature to also impact auto-population of columns on update. Additionally the same feature can eventually be used to perform SELECT query after record creation & update for adapters that do not support RETURNING statement so we need to find another name for the macro that doesn't tie us to specific action or the underlying implementation.

Ultimately the abstraction should allow applications to say "I'd like :col1, :col2 attributes to be consistent with database values whenever possible (on update&create) and using whichever solution is available (RETURNING or additional SELECT)

[abaldwin88]

@nvasilevski Thanks for the review. I'd like to tackle those two items you mentioned down the line. (returning on update and SELECT for mysql). In the interest of keeping this PR small and focused though I didn't try to implement them here.

Keeping those long term plans in mind though I agree the naming should be revised. See my replies within the review.

[nvasilevski]

There is something I realized after leaving comments about naming. If we ever going to support an additional SELECT query using the abstraction we are adding here we need to allow flexible configuration as not every column that needs to be auto-assigned on create needs to be loaded after an update
Imagine having a trigger that populates populated_on_insert_by_a_trigger_column on creation, since we want the attribute to have the value after record creation we would use our new abstraction like reload_attributes_on_save :populated_on_insert_by_a_trigger_column However this will cause an unnecessary SELECT query on updates even though we know the column doesn't change its value on updates unless explicitly updated.

Which means long-term we need to plan to accomomdate an API that actually allows for per-action config. Maybe the API should be similar to callbacks and accept :only or :except arguments. But ultimately we would like to be able to separately configure columns that need to be initialized only on create, only on update and virtual columns will need to always be returned back on create and update

[abaldwin88]

Not sure what the workflow preferences are for the Rails team but my thinking is if this PR looks acceptable we can merge now but keep this method undocumented until the two follow-up PRs are complete (RETURNING on update and SELECT for mysql). Once those are done we can then document to make it a public method.

[owst]

Hi @abaldwin88, thanks for your work on this!

Just to add a +1 for this PR - we're definitely keen to use it when merged - we have many tables with trigger-generated columns that currently force us to issue sub-optimal reloads after create. If we were recreating the app/DB today, we'd be able to use stored generated columns and the recently-merged virtual column RETURNING support, which would be better all-round.

Do you think there's anything else that can be done to get this merged in time for the 7.1 release? I'm happy to help if I can...

[abaldwin88]

@owst Thanks for the kind words!

The key bottleneck for this PR is a Rails team member finding time to review. As you might imagine they're pretty busy folks with a lot of things competing for their attention.

One thing that could help is if you pulled this branch down locally and tested it against your application. Reporting back any issues or otherwise confirming everything works as expected. I can't guarantee doing so would lead to this PR moving ahead imminently but it would help add proof that this patch is working and solves a real-world problem.

[owst]

One thing that could help is if you pulled this branch down locally and tested it against your application. Reporting back any issues or otherwise confirming everything works as expected.

@abaldwin88 I've tried this branch with one of our apps today, and it seems to Just Work, which is great!

[abaldwin88]

Rebased onto latest from main. Fixed conflicts with #49290.

When #49346 is merged it may be worth evaluating that the random_number_plus_one assertion from test_assigned_reload_attributes_on_create passes when using the SQLite3Adapter.

Also SQLite largely discourages using BEFORE triggers:
https://www.sqlite.org/lang_createtrigger.html#cautions_on_the_use_of_before_triggers

I need to do a little more reading but I believe that means the ideal SQLite solution would be including the RETURNING clause for virtual columns and issuing an additional SELECT query for AFTER trigger columns.

Technically though applications using SQLite could still try to utilize BEFORE triggers. Or applications using PostgreSQL could use AFTER triggers for that matter. So we should consider expanding this API so applications can specify whether the values are fetched in the same query or a second follow-up SELECT query.

[owst]

Hi @abaldwin88 just checking in again after a couple of months to see if there's anything more that can be done to progress/prioritise this PR? Thanks!

[abaldwin88]

@owst There's a Rails Discord channel you could try raising this PR in to see if it gets any traction. If there is feedback I'll be happy to address and then resolve conflicts with the main branch.