Saving record with save(validate: false) skips generating slug, sometimes

[vfonic]

I just discovered an issue that happened in my app.

When I call record.save!(validate: false) on a record, it doesn't generate slug for it.

However, if I call record.valid? before calling record.save!(validate: false), it does generate the slug.

This is what made me miss this issue at first.

I believe this could be a bug within the friendly_id gem.

[parndt]

Thanks @vfonic can you please post a minimal reproduction case, or test case?

[vfonic]

@parndt here's the minimal repro case:

vfonic@22d923b

It's on this branch on my fork (just 1 commit):
https://github.com/vfonic/friendly_id/tree/generating-slug-without-validation

Here's the output of running the tests:

# Running:

.........................................................................................................................................................................................................F

Failure:
GeneratingSlugWithValidationSkippedTest#test_should_generate_slug_when_skipping_validation [friendly_id/test/slugged_test.rb:498]:
Expected: "bob-timesletter"
  Actual: nil


rails test friendly_id/test/slugged_test.rb:493

......................................................................................................................................................................................................................................................................................................

Finished in 2.123862s, 233.5368 runs/s, 416.2229 assertions/s.
496 runs, 884 assertions, 1 failures, 0 errors, 0 skips

Note that the second test (when calling .valid?) passes

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[vfonic]

@parndt did you have any chance to look into this one? I don't want this issue to get closed because it's "stale".

Thanks! 🙏

[parndt]

@parndt did you have any chance to look into this one? I don't want this issue to get closed because it's "stale".

@vfonic sorry, not yet, available time is so low right now ☹️

It's nothing to do with what #952 fixes is it?

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[vfonic]

@parndt I rebased on top of the master the test I added in my fork and it's still failing.

Going through the code, this line seems to be the issue:
https://github.com/norman/friendly_id/blob/master/lib/friendly_id/slugged.rb#L250

model_class.before_validation :set_slug

The slug is only set if validation occurs. So if you do record.save(validate: false), the validation will never happen, therefore slug will never be set.

I'm not sure what would be the best way around this.

Here's the list of callbacks executed:
https://guides.rubyonrails.org/active_record_callbacks.html#creating-an-object

before_validation and after_validation are skipped on validate: false.

I played around with after_initialize, after_find and after_touch: https://guides.rubyonrails.org/active_record_callbacks.html#after-initialize-and-after-find
...but we need to set slug either when:

1. setting base attribute or
2. before save

I've found out that if we make this change, all the tests pass:

  model_class.before_validation :set_slug
+ model_class.before_save :set_slug
  model_class.after_validation :unset_slug_if_invalid

We still need to keep model_class.before_validation :set_slug because we want the validations to pass. (There are several tests covering this)

Even though all of the tests pass, I'm not sure if this is the best solution. Someone who knows the project better might find some edge cases that are not covered by tests that might break.

Maybe there's a case where we don't want to generate slug. The good news is that set_slug always checks should_generate_new_friendly_id? before doing anything else.

@parndt what do you think?

[vfonic]

@parndt I've force-pushed my fix to vfonic:generating-slug-without-validation and realized there's already a PR open:
Generating slug when validation is skipped #948

The build for this PR is now passing.

[parndt]

Sounds good to me @vfonic. I've merged it so this is closed by #948

[vfonic]

Thank you, Philip! 👍