carrierwaveuploader / carrierwave

Classier solution for file uploads for Rails, Sinatra and other Ruby web frameworks
https://github.com/carrierwaveuploader/carrierwave
8.78k stars 1.66k forks source link

Can't get model.id in store_dir #2689

Closed ogawa5773 closed 1 year ago

ogawa5773 commented 1 year ago

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

Question

While debugging, I confirmed that uploading and referencing can be done without problems by deleting the following line. https://github.com/carrierwaveuploader/carrierwave/blob/f31546ac41c6a11480425b1528e6834581da2726/lib/carrierwave/orm/activerecord.rb#L46

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. https://github.com/carrierwaveuploader/carrierwave/blob/2f91bee6487d8e5d8bd2c1a88dd25269a2c1e4d0/lib/carrierwave/orm/activerecord.rb#L99

Thanks.

rajyan commented 1 year ago

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 commented 1 year ago

@ogawa5773 I think I could reproduce something similar to what you have reported. https://github.com/carrierwaveuploader/carrierwave/pull/2690 Does this test express your situation?

ogawa5773 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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

I see, thanks for the reproducible example!

ogawa5773 commented 1 year ago

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 commented 1 year ago

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 https://github.com/carrierwaveuploader/carrierwave/pull/2690 when I have some time, but maybe @mshibuya can fix it faster.

ogawa5773 commented 1 year ago

That's right. I've got it. I'll check https://github.com/carrierwaveuploader/carrierwave/pull/2690 out too. thank you very much.