Conversation
ActiveRecord.returning_on_create to specify attributes in RETURNINGActiveRecord.returning_on_create to specify attributes in SQL RETURNING
2045952 to
f52f443
Compare
|
Build error looks like a transitory problem with Sauce Labs throttling.
|
nvasilevski
left a comment
There was a problem hiding this comment.
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)
|
@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. |
ActiveRecord.returning_on_create to specify attributes in SQL RETURNINGActiveRecord.reload_attributes_on_save
f52f443 to
6cefa74
Compare
|
There is something I realized after leaving comments about naming. If we ever going to support an additional 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 |
ActiveRecord.reload_attributes_on_saveActiveRecord.reload_attributes
6cefa74 to
2f61835
Compare
There was a problem hiding this comment.
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.
|
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... |
|
@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. |
@abaldwin88 I've tried this branch with one of our apps today, and it seems to Just Work, which is great! |
2f61835 to
edf30f9
Compare
Add ability for applications to specify a list of attributes that will be reloaded with a SQL `RETURNING` clause when new records are inserted. Long term the plan for databases that do not support RETURNING is to execute an additional SELECT statement. Also handles a minor edge case if application code attempts to write to a virtual column. The values from `RETURNING` will always be used to update the instance's attributes.
edf30f9 to
e73837a
Compare
|
Rebased onto latest from When #49346 is merged it may be worth evaluating that the Also SQLite largely discourages using I need to do a little more reading but I believe that means the ideal SQLite solution would be including the Technically though applications using SQLite could still try to utilize |
|
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! |
|
@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. |
Motivation / Background
PR #48241 detects when certain auto-populated columns should be appended to a
RETURNINGclause. 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_attributesclass 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
RETURNINGwasn't being reflected on the instance attributes after create. See the newly added test and removal of the_read_attributecheck.Performing this nil check with
_read_attributewill also not be compatible whenRETURNINGis implemented onUPDATE. As it is expected that the instance's attributes will be prepopulated with non-nil values. (See related PR here: #48628)Paging: @nvasilevski