rails: Postgres default function values do not get loaded on create
Steps to reproduce
1. Create a model
class Order < ApplicationRecord
end
2. Create a migration with a DB function as default.
class CreateOrders < ActiveRecord::Migration[5.2]
def change
create_table :orders do |t|
t.string :uuid, default: -> { "gen_random_uuid()" }
t.references :user
t.references :product
t.text :comment
t.timestamps
end
end
end
rails db:migrate
3. Create a new order
rails console
order = Order.create(user_id: 1, product_id: 1)
Expected behavior
I expect create to set uuid (by the postgres default) and for it to set the new uuid value into the order AR instance, as it does with the id.
> order
# =>
# +----+--------------------------------------+---------+------------+---------+----------------+----------------+
# | id | uuid | user_id | product_id | comment | created_at | updated_at |
# +----+--------------------------------------+---------+------------+---------+----------------+----------------+
# | 1 | bec94fe0-f7c3-4ede-8b25-c4bce702ec00 | 1 | 1 | | 2018-10-17 ... | 2018-10-17 ... |
# +----+--------------------------------------+---------+------------+---------+----------------+----------------+
Actual behavior
The uuid does get set in the db, but it is not returned into the order instance.
> order
# =>
# +----+------+---------+------------+---------+----------------+----------------+
# | id | uuid | user_id | product_id | comment | created_at | updated_at |
# +----+------+---------+------------+---------+----------------+----------------+
# | 1 | | 1 | 1 | | 2018-10-17 ... | 2018-10-17 ... |
# +----+------+---------+------------+---------+----------------+----------------+
Current workaround
Calling order.reload does then retrieve the values set by postgres. Other workarounds are described in this issue
> order
# =>
# +----+--------------------------------------+---------+------------+---------+----------------+----------------+
# | id | uuid | user_id | product_id | comment | created_at | updated_at |
# +----+--------------------------------------+---------+------------+---------+----------------+----------------+
# | 1 | bec94fe0-f7c3-4ede-8b25-c4bce702ec00 | 1 | 1 | | 2018-10-17 ... | 2018-10-17 ... |
# +----+--------------------------------------+---------+------------+---------+----------------+----------------+
Background
This issue already describes this bug, but the issue was initially a request to support functional defaults, which has since been implemented.
This quote from the discussion summarizes it pretty well:
@kenaniah Sure, but it makes sense why the full row shouldn’t be returned by default: there could be too much data, and ActiveRecord already knows both default and non-default values. (Or at least it thinks so.) But if the default value is generated by a function in PG then ActiveRecord doesn’t know. Turns out that is not enough reason to make
RETURNING *the default, and I’m ok with that. I think what we need is another parameter to tell ActiveRecord, that for a particular model, it should useRETURNING *instead.
This blog post describes the issue wonderfully.
Next steps
So a solution could be an option to tell AR to use RETURNING * in some instances. But I myself am not familiar with the ActiveRecord internals so I feel this should be further discussed.
System configuration
Rails version: 5.2.1
Ruby version: 2.4.1
Postgres version: 9.6.3
About this issue
- Original URL
- State: closed
- Created 6 years ago
- Reactions: 20
- Comments: 35 (20 by maintainers)
ActiveRecord should support adding additional
RETURNINGstatements to models to support things like default functions and triggers (for things like tsvector).I almost would see something like:
Where the
numbercolumn is automatically generated by our database (function or trigger). This implementation could also be used internally for theidcolumn as well on the defaultBaseclass.Updated my proof of concept for Rails 6.0.0.rc1: https://github.com/tbuehlmann/active_record-returning_attributes. Have to admit, monkeypatching core code isn’t fun.
Hi,
I’m having trouble with this matter too and I’m using the callback workaround but I would love for a proper solution to exist 😊
@md5’s approach seems reasonable, triggers being a whole other issue, although I would argue that implementing a manually set list of
RETURNINGcolumns would cover all cases and probably be easier to implement.A solution providing the columns with db-set values automatically could be implemented afterward, piggybacking on the manual implementation, and be a nice-to-have.
What do you think?
❤️
P.S. Could the issue be reopened since people are commenting on it?
This issue caught me off guard, so I did a bit of digging and then a write up on it. In case it’s useful to anyone else who ends up here, here it is: https://dev.to/jbranchaud/a-few-methods-for-returning-default-values-when-creating-activerecord-objects-1457
Right, right now it handles no db-set values. Having it handle db-set values for those with database defaults configured in a migration would handle a whole lot of use cases very well, and be a big step forward. As per the title of this GH issue. No need to solve everything to solve something. I agree with @md5 , and think more expanded feature discussion could be on a different ticket.
(Of course, this ticket is already closed as stale anyway, and apparently commenting on it does not re-open it. 😦 That doesn’t necessarily mean Rails team would look upon a PR unfavorably if one did appear…)
I tackled that exact problem in a POC over here. It wasn’t that simple as I had to override quite some AR core code. So it’s not a very great implementation and will probably break as soon as internals change.
Rather than
RETURNING *, perhaps ActiveRecord could know which columns have proc default values (which I think is what means “direct to DB that can call functions”), and justRETURNINGthose in addition toid?But either way would be an improvement.
@davegson @md5 @matthewd I created an initial PR with a possible solution for this. For now it only has a basic test for record creation which is passing.
Please let me know if you consider this is the right approach to solve this issue. With that I will continue to do the required modifications for record updates and check other tests are passing. Thanks!
I’ll just add that Postgres 12 supports fully generated columns on otherwise normal tables, so the use case here will expand due to that
@tbuehlmann that seems like a separate concern, given that Rails does not natively support managing triggers as part of a schema. It does, however, support computed defaults, so it’s reasonable to expect AR models to reflect those values with no additional effort.
Until such a feature (hopefully) emerges, I’m going with the simplest solution:
after_create :reload@00dav00 thank you so much!
At first glance I really like what I see. Though I would mention I’ve never contributed to the Rails code myself, just inspected it from time to time for my own projects.
What I think you could improve upon is your wording/presentation of the PR. As @matthewd mentioned, this issue is not a bug, hence it was rightfully closed, but a “missing feature”. So your PR is not attempting to “solve a bug”, but “adding a feature”.
Describe it that way, and feel free to copy my intro texts. Show and describe both the problem and your provided solution, so reviewers immediately see what it is all about. (I created a pastebin of my initial markdown so you can copy it more easily - especially the tables were a hassle 😉
I’m sure that will spice things up and help the rails team then review the code part.
@00dav00 I do imply that PRs are the correct way and are welcomed/accepted to then discuss this matter further.
Implied both from 1) @matthewd’s response:
as well as 2) the contribution instructions from rubyonrails.org.
I guess somebody just needs to take a stab at it - I myself sadly don’t have the time to spare.
Yes. To my eye, the ideal behaviour would be @tbuehlmann’s explicit
self.returning_attributes =(for manual additions reflecting things like triggers), plus automatic detection of unparseable default expressions [to always be returned on insert] and computed columns [to always be returned on insert or update].No; this is a feature request, and by policy we don’t track feature requests in this issue tracker. (A missing feature is not a bug, and the more strongly people pretend to believe otherwise, the less emotional energy I, for one, have to deal with it. [A comment for the thread in general, not directed at you.])
@md5 Apologies for misunderstanding your point.
@md5 Sure, ActiveRecord doesn’t native support managing triggers, but the effect of a trigger could be a new value in the column before the insert or update is complete. If ActiveRecord is concerned with the true state of the world it’s only fair that it opens an avenue to be informed of what columns are getting their values out of band. In my opinion,
RETURNINGis the best option. I used it a lot with Ecto, which also doesn’t natively support triggers, stored procedures, etc.This won’t work in situations where database triggers will update the row, think of database-backed timestamps or database-backed counter cache columns. As you only ever get the id returned, you have to manually reload the record as of now.
Support for adding additional
RETURNINGseems orthogonal to me. As @jrochkind mentioned, ActiveRecord knows which columns have computed defaults and whether it is providing values for those columns in any particularINSERT. ActiveRecord should automatically add these columns to theRETURNINGclause and correctly update the model in memory.