Fix raising validation if followable already exist(race condition)

[gonzar11]

What type of PR is this? (check all applicable)

• Refactor
• Feature
• Bug Fix
• Optimization
• Documentation Update

Description

It rescues from ActiveRecord::RecordInvalid assuming that it only be raised in case that we have a race condition trying to create the same Follow model from different threads/request.

Related Tickets & Documents

closes #5793

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

Added to documentation?

• docs.dev.to
• readme
• no documentation needed

[optional] What gif best describes this PR or how it makes you feel?

My first contribution to an open source project.

[citizen428]

We may want to do more than just log an error here, e.g. update a DataDog metric. @mstruve can point you in the right direction.

[mstruve]

I'm torn. I don't really think it is that important but at the same time, I have learned when in doubt log it so let's do DataDogStatsClient.increment("users.invalid_follow") instead of Rails.logger here to track it on the off chance we want the metric in the future for some reason.

[citizen428]

Tbh, I wasn't sure either, hence the "we may want to do more". But like you I lean in the direction of logging more.

[citizen428]

def is an implicit begin, so this could be:

def follow(followable, need_notification: false)
  user_follow = current_user.follow(followable)
rescue ActiveRecord::RecordInvalid => e
  Rails.logger.error(e)
end

See e.g. https://www.rubyguides.com/2019/06/ruby-rescue-exceptions/

[gonzar11]

Thank you @citizen428 . I am seeing that codeclimate/diff-coverage check failed. How can I do to turn this check in green?

[rhymes]

unfortunately the report is not very forthcoming on details, so I think you can ignore it :)

[gonzar11]

@citizen428 If I don't use begin then I have to use return in ensure block. Is this ok for you?

def follow(followable, need_notification: false)
  user_follow = current_user.follow(followable)
rescue ActiveRecord::RecordInvalid => e
  DataDogStatsClient.increment("users.invalid_follow")
ensure
   Notification.send_new_follower_notification(user_follow) if need_notification
   return "followed"
end

[mstruve]

I would set it up like this and actually I think that reads pretty cleanly personally. We will see what @citizen428 thinks.

def follow(followable, need_notification: false)
  user_follow = current_user.follow(followable)
  Notification.send_new_follower_notification(user_follow) if need_notification
rescue ActiveRecord::RecordInvalid => e
  DataDogStatsClient.increment("users.invalid_follow")
ensure
   return "followed"
end

[citizen428]

I'm not sure if we should really return "followed" in case something went wrong. Maybe just have different returns based on whether we succeed or fail?

def follow(followable, need_notification: false)
  user_follow = current_user.follow(followable)
  Notification.send_new_follower_notification(user_follow) if need_notification
  "followed"
rescue ActiveRecord::RecordInvalid => e
  DataDogStatsClient.increment("users.invalid_follow")
  # some other message, e.g. "previously followed"
end

Either way, it's such a small detail I'll leave it to your judgement what you think is best, i.e. if you want to keep as-is, I have no problem with that.

[gonzar11]

Thank you folks for your help. I ended up removing ensure because Rubocop doesn't like to have a return within ensure.

[mstruve]

I think we should add this to our happy path/begin block since we know if the follow is invalid we dont need to send a notification.

[mstruve]

LGTM 🚀

[mstruve]

I know I approved it but it would probably be a good idea to add a spec for the new failure mode

[rhymes]

Looks good, thank you @gonzar11!

[gonzar11]

I know I approved it but it would probably be a good idea to add a spec for the new failure mode

I thought about that but I couldn't figure out how to simulate this race condition. Do you mean by making request from multiple threads o something like that?

[mstruve]

I was thinking by creating one and then attempting to use this endpoint to create a second immediately after. But I can live without it :)

[mstruve]

Oh one last thing, did you sign the CLA?

[gonzar11]

Oh one last thing, did you sign the CLA?

I did it :)

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}