Skip to content
This repository has been archived by the owner on Jan 30, 2025. It is now read-only.

ActiveRecord::StatementInvalid when saving with association #8

Open
n00ge opened this issue May 16, 2019 · 9 comments
Open

ActiveRecord::StatementInvalid when saving with association #8

n00ge opened this issue May 16, 2019 · 9 comments

Comments

@n00ge
Copy link

n00ge commented May 16, 2019

I'm getting an error while using this gem when saving a model with built associations.

#<ActiveRecord::StatementInvalid: PG::InFailedSqlTransaction: ERROR:  current transaction is aborted, commands ignored until end of transaction block

An example would be:

user = User.new(some: attrs)
user.build_profile(some: attrs)
user.save

(not real but communicating the scenario of a has_one being built)

rescue_unique_constraint is on both the association (profile) and primary model (user) but the failure in this scenario is stemmed from the uniqueness failure on the association (profile).

Has anybody ran into this before? Any suggestions?

Wondering if the rescue in the gem for the unique constraint should also re-raise with ActiveRecord::Rollback if ActiveRecord::Base.connection.open_transactions.positive? is true?

This might be related?
jeremyevans/sequel#908

@vaibhavatul47
Copy link

Please share associations and unique constraints of user and profile models along with a code to reproduce the issue.

@n00ge
Copy link
Author

n00ge commented May 17, 2019

Working on creating an isolated scenario... Not at the same place, but still seeing an issue.

## migration for unique indexes
class UniqueIndexes < ActiveRecord::Migration[5.1]
  def change
    add_index :users, :email, unique: true
    add_index :profiles, :user_id, unique: true
    add_index :profiles, %w[street zip], unique: true
  end
end

## schema 
create_table "profiles", force: :cascade do |t|
  t.bigint "user_id"
  t.string "street"
  t.string "unit"
  t.string "zip"
  t.datetime "created_at", null: false
  t.datetime "updated_at", null: false
  t.index ["street", "zip"], name: "index_profiles_on_street_and_zip", unique: true
  t.index ["user_id"], name: "index_profiles_on_user_id", unique: true
end

create_table "users", force: :cascade do |t|
  t.string "first_name"
  t.string "last_name"
  t.string "email"
  t.datetime "created_at", null: false
  t.datetime "updated_at", null: false
  t.index ["email"], name: "index_users_on_email", unique: true
end

## models/user.rb
class User < ApplicationRecord
  include RescueUniqueConstraint
  rescue_unique_constraint index: :index_users_on_email,
                           field: :email

  has_one :profile, inverse_of: :user
end

## models/profile.rb
class Profile < ApplicationRecord
  include RescueUniqueConstraint
  belongs_to :user, optional: true, inverse_of: :profile

  rescue_unique_constraint index: :index_profiles_on_user_id,
                           field: :user_id

  rescue_unique_constraint index: :index_profiles_on_street_and_zip,
                           field: :street
end

Given the above setup, the below code returns a successful save with an id on the user returned even though neither the user or built profile are persisted.

Profile.create(street: '123 street', zip: '12345') # this is successful
user = User.new(email: '[email protected]')
user.build_profile(street: '123 street', zip: '12345')
user.save # returns true
user # shows an id on the user but was not persisted

The difference might that the code I mentioned in the original issue is running in rspec with DatabaseCleaner. Either way, the code in this example returning true from the save and an id on the user is not good.

I believe we'll need to add a raise ActiveRecord::Rollback if running within a transaction.

@n00ge
Copy link
Author

n00ge commented May 20, 2019

I've been testing out re-raising within create_or_update_with_rescue where the return false is.
Neither ActiveRecord::Rollback or ActiveRecord::RecordInvalid propagate to return the User to a rolled-back state. See the logs below:

irb(main):001:0> user = TestUniqueConstraint.call
   (0.1ms)  BEGIN
  User Create (0.8ms)  INSERT INTO "users" ("email", "created_at", "updated_at") VALUES ($1, $2, $3) RETURNING "id"  [["email", "[email protected]"], ["created_at", "2019-05-20 13:35:50.410636"], ["updated_at", "2019-05-20 13:35:50.410636"]]
  Profile Create (1.6ms)  INSERT INTO "profiles" ("user_id", "street", "zip", "created_at", "updated_at") VALUES ($1, $2, $3, $4, $5) RETURNING "id"  [["user_id", 31], ["street", "123 street"], ["zip", "12345"], ["created_at", "2019-05-20 13:35:50.414714"], ["updated_at", "2019-05-20 13:35:50.414714"]]
#<ActiveRecord::RecordNotUnique: PG::UniqueViolation: ERROR:  duplicate key value violates unique constraint "index_profiles_on_street_and_zip"
DETAIL:  Key (street, zip)=(123 street, 12345) already exists.
: INSERT INTO "profiles" ("user_id", "street", "zip", "created_at", "updated_at") VALUES ($1, $2, $3, $4, $5) RETURNING "id">
   (0.4ms)  COMMIT
=> #<User id: 31, first_name: nil, last_name: nil, email: "[email protected]", created_at: "2019-05-20 13:35:50", updated_at: "2019-05-20 13:35:50">
irb(main):002:0> user.persisted?
=> true
irb(main):003:0> user.reload
  User Load (0.5ms)  SELECT  "users".* FROM "users" WHERE "users"."id" = $1 LIMIT $2  [["id", 31], ["LIMIT", 1]]
ActiveRecord::RecordNotFound: Couldn't find User with 'id'=31
	from (irb):3

Where TestUniqueConstraint.call runs:

Profile.create(street: '123 street', zip: '12345')
user = User.new(email: '[email protected]')
user.build_profile(street: '123 street', zip: '12345')
user.save

Having the user returned in a state that appears to be persisted is very dangerous.
Any thoughts or suggestions on how to handle?

@n00ge
Copy link
Author

n00ge commented May 21, 2019

FWIW, this issue isn't present when using sqlite. If you use postgres it is present.

@vaibhavatul47
Copy link

vaibhavatul47 commented May 21, 2019

This is the default nature of ActiveRecord associations. The parent object will be saved to DB even if any child object fails validations.
In case you want to validate that all profiles are validated before saving user object, then you can add validates_associated :profile to User model. [ docs ]

class User < ActiveRecord::Base
 include RescueUniqueConstraint
 rescue_unique_constraint index: :index_users_on_email, field: :email

 has_one :profile, inverse_of: :user
 validates_associated :profile # will ensure user is saved only when Profile is also valid
end

More on Rails5: https://api.rubyonrails.org/classes/ActiveRecord/AutosaveAssociation.html

@n00ge
Copy link
Author

n00ge commented May 21, 2019

You're saying it is standard for Rails to return a User model that looks as if it is persisted but is not?
If you look at comment above with more detail you'll see what I'm referring to:

=> #<User id: 31, first_name: nil, last_name: nil, email: "[email protected]", created_at: "2019-05-20 13:35:50", updated_at: "2019-05-20 13:35:50">
irb(main):002:0> user.persisted?
=> true
irb(main):003:0> user.reload
  User Load (0.5ms)  SELECT  "users".* FROM "users" WHERE "users"."id" = $1 LIMIT $2  [["id", 31], ["LIMIT", 1]]
ActiveRecord::RecordNotFound: Couldn't find User with 'id'=31

Note the User returned has an id (31), returns true when calling persisted?, and then throws RecordNotFound when calling reload.

I know that is not default nature of ActiveRecord. When a record is rolled back, Rails acknowledges that by removing the primary key (id) value to communicate that it is not persisted.

@vaibhavatul47
Copy link

vaibhavatul47 commented May 24, 2019

You're saying it is standard for Rails to return a User model that looks as if it is persisted but is not?

No

In your case, you are saving two models at the same time. (A user and its associated profile ). IMO, this is how stuff is working internally:
Step1: You called user.save
Step2: Validations of user model are performed.
Step3: If validation is successful, ActiveRecord prepares to save user to DB.
Step4: ActiveRecord observes that user has an associated profile. ActiveRecord attempts to save to profile to DB. [ Note: without performing validations of Profile model ]
Step5: profile is not saved due to exception from DB.
Step6: ActiveRecord resumes saving of user record.
Step7: user record is saved to DB and thus it is persisted and has an id, but user.profile failed to save, thus no id on user.profile.

@n00ge
Copy link
Author

n00ge commented May 24, 2019

That sounds right but step 7 does not occur as you've noted. User saved to the database, had an id assigned to the model instance in memory, but rolled back after the profile save failed without rolling back the model changes to the user in memory leaving the id on the user. Sqlite works as you described but not postgres.

I'll try to get a sample project pushed up this weekend to make it easier for you to run the code I had above and see the issue.

@n00ge
Copy link
Author

n00ge commented May 29, 2019

I posted up a sample rails app to show the issue when using postgres. This did also confirm that the ActiveRecord::StatementInvalid comes from using using transactions around tests.

See this spec file for details around the problem.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants