Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Postgres default function values do not get loaded on create #34237

Closed
davegson opened this issue Oct 17, 2018 · 34 comments
Closed

Postgres default function values do not get loaded on create #34237

davegson opened this issue Oct 17, 2018 · 34 comments

Comments

@davegson
Copy link

@davegson davegson commented Oct 17, 2018

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 use RETURNING * 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

@davegson davegson changed the title Postgres default function does not get loaded on create Postgres default function values do not get loaded on create Oct 17, 2018
@jrochkind
Copy link
Contributor

@jrochkind jrochkind commented Oct 17, 2018

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 just RETURNING those in addition to id?

But either way would be an improvement.

@yawboakye
Copy link
Contributor

@yawboakye yawboakye commented Oct 20, 2018

Currently the solution is to immediately reload the model (and that's what I do). That's not much of a penalty (an extra DB inquiry). There's many other things that affect what value eventually ends up in the column (e.g. triggers) which makes me lean more towards a reload after_save. In that case a directive to append a RETURNING * clause to the generated SQL for INSERT and UPDATE for a model and to build the object out of those values would make more sense.

@jrochkind
Copy link
Contributor

@jrochkind jrochkind commented Oct 20, 2018

Oh good point, rather than AR try to automatically discover what to put in RETURNING a directive makes a lot of sense, and is probably pretty straightforward implementation. Directive could be either on the model (for all saves), or an argument to save. Probably the first, or both.

My current work-around is by doing something a little bit different than a reload. I just override the relevant attribute method to do a lazy pluck if the thing is persisted? and the value is nil.

Really it ought to check not just persisted? but a @has_just_been_created set in an after_create too, to only try to do the fetch if the in-memory object was just inserted, not fetched from the db. And as you can see, need to do some odd (and possibly private API?) things to make sure the fetch of the db-provided value doesn't make AR think the model is "dirty". And this technique would start to break down if I had more than one column set from the db (or require an even more complex implementation to make sure there was only one fetch for all of those attributes. (Or just go back to the non-lazy reload). So this is a pretty kludgey and error-prone workaround. It would be better if AR could just handle this with built-in API one way or another.

  # Due to rails bug, we don't immediately have the database-provided value after create. :(
  # If we ask for it and it's empty, go to the db to get it
  # https://github.com/rails/rails/issues/21627
  def friendlier_id(*_)
    in_memory = super

    if !in_memory && persisted? && !@friendlier_id_retrieved
      in_memory = self.class.where(id: id).limit(1).pluck(:friendlier_id).first
      write_attribute(:friendlier_id, in_memory)
      clear_attribute_change(:friendlier_id)
      # just to avoid doing it multiple times if it's still unset in db for some reason
      @friendlier_id_retrieved = true
    end

    in_memory
  end
@tbuehlmann
Copy link
Contributor

@tbuehlmann tbuehlmann commented Nov 6, 2018

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.

@davegson
Copy link
Author

@davegson davegson commented Nov 6, 2018

If this will be implemented, I it will definitely override some AR core code ;) so it's a great reference!

Still, for this I assume we need somebody very familiar with the AR internals

@rails-bot
Copy link

@rails-bot rails-bot bot commented Feb 4, 2019

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.
If you can still reproduce this error on the 5-2-stable branch or on master, please reply with all of the information you have about it in order to keep the issue open.
Thank you for all your contributions.

@rails-bot rails-bot bot added the stale label Feb 4, 2019
@rails-bot rails-bot bot closed this Feb 11, 2019
@bobbytables
Copy link

@bobbytables bobbytables commented Mar 14, 2019

ActiveRecord should support adding additional RETURNING statements to models to support things like default functions and triggers (for things like tsvector).

I almost would see something like:

class User < ApplicationRecord
  add_returning_column "number", on: [:create, :update]
end

Where the number column is automatically generated by our database (function or trigger). This implementation could also be used internally for the id column as well on the default Base class.

@md5
Copy link
Contributor

@md5 md5 commented Apr 15, 2019

Support for adding additional RETURNING seems orthogonal to me. As @jrochkind mentioned, ActiveRecord knows which columns have computed defaults and whether it is providing values for those columns in any particular INSERT. ActiveRecord should automatically add these columns to the RETURNING clause and correctly update the model in memory.

@tbuehlmann
Copy link
Contributor

@tbuehlmann tbuehlmann commented Apr 17, 2019

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.

@md5
Copy link
Contributor

@md5 md5 commented Apr 17, 2019

@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.

@yawboakye
Copy link
Contributor

@yawboakye yawboakye commented Apr 17, 2019

@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, RETURNING is the best option. I used it a lot with Ecto, which also doesn't natively support triggers, stored procedures, etc.

@md5
Copy link
Contributor

@md5 md5 commented Apr 17, 2019

@yawboakye I think I made it pretty clear I support the idea of explicitly adding values to RETURNING. I just think that dealing with the values Rails already manages the defaults for is lower-hanging fruit.

@jrochkind
Copy link
Contributor

@jrochkind jrochkind commented Apr 17, 2019

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...)

@yawboakye
Copy link
Contributor

@yawboakye yawboakye commented Apr 18, 2019

@md5 Apologies for misunderstanding your point.

@simonc
Copy link
Contributor

@simonc simonc commented Jul 17, 2019

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 RETURNING columns 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?

@jrochkind
Copy link
Contributor

@jrochkind jrochkind commented Jul 17, 2019

I guess with no existing Rails maintainers expressing interest, what would have to happen is someone to create a PR? I agree it's hard to motivate to spend time doing this when you don't know if the PR will get attention or have any chance of being merged.

I agree this issue should be open because it is a real issue, affecting multiple people, with or without a current solution. But that may not be how Rails manages it's issues.

Any kind of advice from maintainers would be very welcome.

@md5
Copy link
Contributor

@md5 md5 commented Jul 17, 2019

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

@davegson
Copy link
Author

@davegson davegson commented Jul 18, 2019

@jrochkind, I too want to hear advice from maintainers, since otherwise a PR would be a shot in the dark. Any ideas how to get their attention? Because as it is now, being a closed, stale issue, we're basically lost in space.

Getting in contact with a maintainer could also lead to reopening the issue - as it is a real & active issue. [If we were open, we would be on page 2, sorted by comments - and on page 1 sorted by thumbsup👍.]

@tbuehlmann
Copy link
Contributor

@tbuehlmann tbuehlmann commented Jul 18, 2019

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.

@choncou
Copy link

@choncou choncou commented Aug 16, 2019

@tbuehlmann Nice work on a potential solution for a fix. It would be great if we could get it working without needing to configure self.returning_attributes= on the model (currently it crashes when not set).

@choncou
Copy link

@choncou choncou commented Aug 16, 2019

@rafaelfranca Apologies if you're the wrong person to ask about this. But is this functionality that would make sense as a part of ActiveRecord core ?

If so, can we reopen this issue ?

Below I've rewritten to steps for reproduction, and desired functionality:

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  # Activate the gem you are reporting the issue against.
  gem "activerecord", "6.0.0.rc1"
  gem "pg"
end

require "active_record"
require "minitest/autorun"
require "logger"

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: "postgresql", encoding: 'unicode', database: 'rails_issues', username: 'postgres', host: 'localhost', port: '5432')
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  enable_extension "pgcrypto"

  create_table :posts, force: true do |t|
    t.string :name
    t.string :default_value_string
    t.string :default_generated_string, default: -> { "nextval('posts_id_seq')" }
    t.integer :some_number
    t.bigint :default_value_int, default: -> { "nextval('posts_id_seq')" }
    t.uuid :uuid
  end

  change_column_default :posts, :uuid, 'gen_random_uuid()'
  change_column_default :posts, :default_value_string, 'default_string'
end

class Post < ActiveRecord::Base
end

class BugTest < Minitest::Test
  # This fails
  def test_db_generated_columns_returned
    post = Post.create!(
      name: 'custom name',
      some_number: 2
    )

    assert_equal 'custom name', post.name
    assert_equal 'default_string', post.default_value_string

    refute_nil post.default_generated_string, "expected the string to be present"
    refute_nil post.default_value_int, "expected the int to be present"
    refute_nil post.uuid, "expected the uuid to be present"
  end

  # This passes because of `#reload`
  def test_all_attributes_are_set_on_reload
    post = Post.create!(
      name: 'custom name',
      some_number: 2
    )
    post.reload

    assert_equal 'custom name', post.name
    assert_equal 'default_string', post.default_value_string

    refute_nil  post.default_generated_string
    refute_nil  post.default_value_int
    refute_nil  post.uuid
  end
end
@matthewd
Copy link
Member

@matthewd matthewd commented Aug 17, 2019

is this functionality that would make sense as a part of ActiveRecord core ?

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].

If so, can we reopen this issue ?

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.])

@choncou
Copy link

@choncou choncou commented Aug 17, 2019

@matthewd great, thanks for the feedback.

So would the best approach be to begin a PR, and open discussions like this there? With a potential solution already committed.

@omarsotillo
Copy link

@omarsotillo omarsotillo commented Oct 10, 2019

Hey guys! @tbuehlmann @choncou @matthewd was a PR already started so it can be linked here?

@tbuehlmann
Copy link
Contributor

@tbuehlmann tbuehlmann commented Oct 24, 2019

@omarsotillo: Nope, don't think so.

@md5
Copy link
Contributor

@md5 md5 commented Nov 19, 2019

I just saw that the insert_many functionality added in Rails 6 supports RETURNING: #35077

It reminded me of this issue, which is near and dear to my heart.

@woniesong92
Copy link

@woniesong92 woniesong92 commented Nov 21, 2019

This issue just beat me. So is the best solution for now just doing a reload after a create?

@choncou
Copy link

@choncou choncou commented Nov 22, 2019

@woniesong92 Yup I believe so.

@00dav00
Copy link

@00dav00 00dav00 commented Jul 9, 2020

+1 for implementing this, just hit the problem in a project.
Is there any consensus on PRs being accepted for this issue?

@davegson
Copy link
Author

@davegson davegson commented Jul 14, 2020

@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:

If so, can we reopen this issue ?

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.])

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.

@00dav00
Copy link

@00dav00 00dav00 commented Aug 1, 2020

@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.

Screenshot from 2020-08-01 10-01-11

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!

@davegson
Copy link
Author

@davegson davegson commented Aug 3, 2020

@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.

@yoones
Copy link
Contributor

@yoones yoones commented Dec 5, 2020

Until such a feature (hopefully) emerges, I'm going with the simplest solution: after_create :reload

@jbranchaud
Copy link
Contributor

@jbranchaud jbranchaud commented Jan 19, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet