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

Can't get model.id in store_dir #2689

Closed
ogawa5773 opened this issue Aug 21, 2023 · 9 comments · Fixed by #2690
Closed

Can't get model.id in store_dir #2689

ogawa5773 opened this issue Aug 21, 2023 · 9 comments · Fixed by #2690

Comments

@ogawa5773
Copy link

ogawa5773 commented Aug 21, 2023

My Trouble

I'm trying to update the version from 2.x to 3.x.

While checking the operation associated with the version upgrade, we observed a phenomenon that model.id could not be obtained in store_dir and the path was different when uploading images and when referencing them.

example

class BaseImageUploader < CarrierWave::Uploader::Base
  include CarrierWave::MiniMagick

  def store_dir
    "uploads/#{model.class.name.downcase}/#{mounted_as}/#{model.id}"
  end
...

If implement the above, the path difference will be like this

  • Save like this uploads/user/thumbnail
  • When referring to uploads/user/thumbnail/111

Question

While debugging, I confirmed that uploading and referencing can be done without problems by deleting the following line.

@_mounters[:"#{column}"] = nil

I read the code there, but I can't understand the relationship that model.id can't be obtained. Can you give me a brief explanation, please? Also is it deprecated to use model.id in store_dir?

Memo

It has been confirmed that version 2 series (implementation below) is working without problems.

def initialize_dup(other)

Thanks.

@rajyan
Copy link
Contributor

rajyan commented Aug 22, 2023

We are using carrierwave 3.x with the same store_dir configuration, but it's working fine for us.
I think it's related to the dup call in your codebase? (which might be a bug anyway)

rajyan pushed a commit to rajyan/carrierwave that referenced this issue Aug 22, 2023
duplication should not affect the duplicated objects path
@rajyan
Copy link
Contributor

rajyan commented Aug 22, 2023

@ogawa5773
I think I could reproduce something similar to what you have reported.
#2690
Does this test express your situation?

@ogawa5773
Copy link
Author

ogawa5773 commented Aug 22, 2023

@rajyan
Thank you for confirming, that is exactly the phenomenon.
And as far as I verified on the local PC, I confirmed that the dup method was called internally when executing the ApplicationRecord update.

For example, if create a User model like the one below and update the name property, the debug log described in initialize_dup will be output.

class User < ApplicationRecord

  def initialize_dup(other)
    puts '////////////'
    puts 'initialize_dup called'
    super
  end  
end
CREATE TABLE users (
  id int(11) NOT NULL auto_increment,
  name varchar(255),
  PRIMARY KEY  (id)
);

@rajyan
Copy link
Contributor

rajyan commented Aug 22, 2023

@ogawa5773

For example, if create a User model like the one below and update the name property, the debug log described in initialize_dup will be output.

Maybe I'm missing something, but I couldn't reproduce this behavior. There might be a code calling dup somewhere else?

Tested with the below Dockerfile

FROM ruby:3.2.2

WORKDIR /app

RUN <<EOF cat >> Gemfile
source 'https://rubygems.org'
gem 'rails', '~> 7.0.6'
EOF

RUN bundle install
RUN bundle exec rails new /app

RUN bundle exec rails g model User name:string
RUN bundle exec rails db:create
RUN bundle exec rails db:migrate

RUN <<EOF cat > app/models/user.rb
class User < ApplicationRecord
  def initialize_dup(other)
    puts '////////////'
    puts 'initialize_dup called'
    super
  end
end
EOF
docker run -it sha256:9c2dcb806eeee17a72d38e406aef0cca69a92c5c818e47ef0534a01164af33ae rails console
Loading development environment (Rails 7.0.7)
irb(main):001:0> u=User.new
=> #<User:0x0000ffff7a67a230 id: nil, name: nil, created_at: nil, updated_at: nil>
irb(main):002:0> u.name="test1"
=> "test1"
irb(main):003:0> u.save!
  TRANSACTION (0.1ms)  begin transaction
  User Create (0.9ms)  INSERT INTO "users" ("name", "created_at", "updated_at") VALUES (?, ?, ?)  [["name", "test1"], ["created_at", "2023-08-22 07:03:04.019553"], ["updated_at", "2023-08-22 07:03:04.019553"]]
  TRANSACTION (5.6ms)  commit transaction
=> true
irb(main):004:0> u.update(name: "test2")
  TRANSACTION (0.2ms)  begin transaction
  User Update (1.0ms)  UPDATE "users" SET "name" = ?, "updated_at" = ? WHERE "users"."id" = ?  [["name", "test2"], ["updated_at", "2023-08-22 07:03:17.329174"], ["id", 1]]
  TRANSACTION (3.3ms)  commit transaction
=> true
irb(main):005:0> u.name="test3"
=> "test3"
irb(main):006:0> u.save!
  TRANSACTION (0.2ms)  begin transaction
  User Update (1.1ms)  UPDATE "users" SET "name" = ?, "updated_at" = ? WHERE "users"."id" = ?  [["name", "test3"], ["updated_at", "2023-08-22 07:03:33.600632"], ["id", 1]]
  TRANSACTION (3.4ms)  commit transaction
=> true
irb(main):007:0> u.dup
////////////
initialize_dup called
=> #<User:0x0000ffff7bbaba50 id: nil, name: "test3", created_at: nil, updated_at: nil>

@ogawa5773
Copy link
Author

ogawa5773 commented Aug 22, 2023

@rajyan

Thank you for checking.
In my environment, the following log is output.

[2] pry(main)> u.class.name
"User"
[3] pry(main)> u.name
"local_user_4"
[4] pry(main)> u.update(name: 'local_user_5')
  TRANSACTION (2.2ms)  BEGIN
  User Update (9.7ms)  UPDATE `users` SET `users`.`name` = 'local_user_5', `users`.`updated_at` = '2023-08-22 08:15:23' WHERE `users`.`id` = 1
/////////////////////
initialize_dup called
  TRANSACTION (9.6ms)  COMMIT
true
[5] pry(main)> 

And after you reported it, I checked and it seems that the counter_culture gem is using dup internally.
Therefore, could you please check if it can be reproduced based on the following Dockerfile?

FROM ruby:3.2.2

WORKDIR /app

RUN <<EOF cat >> Gemfile
source 'https://rubygems.org'
gem 'rails', '~> 7.0.6'
EOF

RUN bundle install
RUN bundle exec rails new /app
RUN bundle add counter_culture
RUN bundle install

RUN bundle exec rails g model Prefecture name:string users_count:integer
RUN bundle exec rails g model User name:string prefecture_id:integer
RUN bundle exec rails db:create
RUN bundle exec rails db:migrate

RUN <<EOF cat > app/models/prefecture.rb
class Prefecture < ApplicationRecord
  has_many :users
end
EOF

RUN <<EOF cat > app/models/user.rb
class User < ApplicationRecord
  belongs_to :prefecture, optional: true
  counter_culture :prefecture
  def initialize_dup(other)
    puts '////////////'
    puts 'initialize_dup called'
    super
  end
end
EOF

I have reproduced as below.

irb(main):001:0> u = User.new
=> #<User:0x0000ffff9dbe4040 id: nil, name: nil, prefecture_id: nil, created_at: nil, updated_at: nil>
irb(main):002:0> u.name = "test"
=> "test"
irb(main):003:0> u.save
  TRANSACTION (0.3ms)  begin transaction
  User Create (0.9ms)  INSERT INTO "users" ("name", "prefecture_id", "created_at", "updated_at") VALUES (?, ?, ?, ?)  [["name", "test"], ["prefecture_id", nil], ["created_at", "2023-08-22 08:41:20.756351"], ["updated_at", "2023-08-22 08:41:20.756351"]]
  TRANSACTION (3.9ms)  commit transaction
=> true
irb(main):004:0> u.update(name: "test2")
  TRANSACTION (0.2ms)  begin transaction
  User Update (0.7ms)  UPDATE "users" SET "name" = ?, "updated_at" = ? WHERE "users"."id" = ?  [["name", "test2"], ["updated_at", "2023-08-22 08:41:31.665055"], ["id", 1]]
////////////
initialize_dup called
  TRANSACTION (5.7ms)  commit transaction
=> true

@rajyan
Copy link
Contributor

rajyan commented Aug 22, 2023

I checked and it seems that the counter_culture gem is using dup internally.

I see, thanks for the reproducible example!

@ogawa5773
Copy link
Author

ogawa5773 commented Aug 22, 2023

Thank you very much for guiding me to the solution.

I'm thinking of stopping the use of counter_culture as an interim solution, but do you have any plans to fix this phenomenon in this gem?
If there are no plans, I will close this issue.

@rajyan
Copy link
Contributor

rajyan commented Aug 22, 2023

I’m just one user of this library and not an admin of this repo, so not sure this is going to be fixed. I can look into #2690 when I have some time, but maybe @mshibuya can fix it faster.

@ogawa5773
Copy link
Author

That's right. I've got it. I'll check #2690 out too. thank you very much.

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

Successfully merging a pull request may close this issue.

2 participants