norman / friendly_id

FriendlyId is the “Swiss Army bulldozer” of slugging and permalink plugins for ActiveRecord. It allows you to create pretty URL’s and work with human-friendly strings as if they were numeric ids for ActiveRecord models.
http://norman.github.io/friendly_id/
MIT License
6.15k stars 591 forks source link

Slug not generated if using Model.upsert #927

Closed dewey closed 4 years ago

dewey commented 4 years ago

Hey,

I'm using friendly_id 5.3.0, Rails 6.0.1 and Ruby 2.6.5p114. I just refactored some code to use the new "upsert" method.

Now I have the problem that the slug is not generated any more at insertion time like it was when I just used Podcast.new(podcast_params). I'm not sure if I'm doing something wrong, there's something missing or it's a bug.

My simplified model looks like this, I just added the new_record? check too but that didn't help. Also if I'm printing in should_generate_new_friendly_id it doesn't seem to even get called? In my other controller that I didn't migrate to upsert() yet it calls that method of the model so the general setup I have seems to be correct.

class Podcast < ApplicationRecord
  include FriendlyId

  friendly_id :name, use: [:slugged, :history]

  def should_generate_new_friendly_id?
    slug.nil? || name_changed? || new_record?
  end
end

My simplified controller code where I post the payload to create the model to looks like this. I removed the unnecessary fields for brevity.

@returning_podcast = Podcast.upsert({
  external_id: podcast_params[:external_id],
  name: podcast_params[:name],
}, unique_by: :external_id)

Any hints where I could start my debugging journey? Thank you!

What I tried so far

After investigating a bit further I thought this might be related to upsert not initializing models or calling ActiveRecord callbacks so I rewrote it to initialze the model like this:

podcast = Podcast.new do |p|
  p.external_id = podcast_params[:external_id]
  p.name = podcast_params[:name]
end

ap podcast
# We serialize our model, but then use reject to remove all values that are nil. Otherwise it tries to insert "id = null" which
# breaks our not-null constraint in the DB
@returning_podcast = Podcast.upsert(podcast.attributes.reject { |_k, v| v.nil? }, unique_by: :external_id)

I had to do this reject workaround as otherwise it tries to insert a NULL id. I'm not sure if that's the proper way but it works. In the previous upsert approach I just passed in the parameters one-by-one so I didn't have this issue. I don't think that's related to the problem though.

parndt commented 4 years ago

@dewey interesting; unfortunately if it doesn't trigger any callbacks, I'm not sure what we can do. We rely on that for FriendlyId to function.

The best workarounds I can think of:

dewey commented 4 years ago

Hey @parndt, thanks for your reply. I now switched to find_or_create_by for the models that should support slugs and it works like that so far.

Could you give me a hint how the second approach would work? I maybe want to revisit that if I refactor it to use upserts again in the future? Thank you!

parndt commented 4 years ago

In your model you'd have to override the method with something that touched the records afterward, like this:

def self.upsert(attributes, returning: nil, unique_by: nil)
  return_value = super # or something
  return_value.each { |primary_key| find(primary_key).touch } if return_value.present?
  return_value
end

^ this is extremely messy, but hopefully illustrates how I expect it would work. At this point, we'd have to question why we'd even be using upsert as we're manually finding the records, and invoking callbacks, which is the main upside to upsert?

dewey commented 4 years ago

Thanks for that @parndt, appreciate the input!

I've now switched everything back to find_or_create_by as it seems to be the most used solution to the problem. I'm coming from a Go background where there's less ORM like behavior and I mostly use raw SQL upsert queries to save the extra trip to the (PG) DB.

I'll see if something changes in regards to upserting with Rails in the coming releases, it's still pretty new.

Thanks again for your assistance and I'll close the issue for now.

parndt commented 4 years ago

Thanks @dewey :+1: