`#update_attributes` uses `#update_all` inside it

[ebihara99999]

Sorcery::Adapters::ActiveRecordAdapter#update_attributes uses #update_all inside instead of simply #update.
I think it intends one active record object to be updated because the code does it, so #updated_all is not necessary.

If I open a PR, I plan to change from

@model.class.where(:"#{primary_key}" => @model.send(:"#{primary_key}")).update_all(attrs)

to

@model.class.find_by(:"#{primary_key}" => @model.send(:"#{primary_key}")).update(attrs)

The good points are:

• keep the code simple
• return boolean, not integer ( seems more understandable to me )

Thank you for watching, I would appreciate it if someone gives me suggestion.

[Ch4s3]

@ebihara99999 sounds good to me.

[ebihara99999]

@Ch4s3
Thank you. I will open a PR soon!

[palkan]

@Ch4s3 @ebihara99999 there are bad points too:

• now it makes 2 queries instead of one
• it invokes validations/callbacks – don't think we need them here
• it can cause race conditions.
  Suppose that we have a record with a = 2 and make two requests with different values:

thread 1	thread 2
find_by # loads the record with a=2	_
_	find_by # loads the record with a=2
update(a: 1) # updates the record	_
_	update(a: 2) # do nothing, 'cause there is no changes AR-side

So, the final value is 1 and not 2.

[Ch4s3]

@palkan interesting. We definitely want to avoid a race condition, I hadn't considered that.

[palkan]

Btw, update_columns should do the trick: it's safe, it doesn't involve callbacks/validations and it returns true / false. Kind of compromise between where.update_all and find_by.update.

[palkan]

UPD: And also, what if the record has been deleted? I.e. when find_by returns nil?

Maybe, it's better to just update existing code like this:

updated_count = @model.class.where(:"#{primary_key}" => @model.send(:"#{primary_key}")).update_all(attrs)
updated_count == 1

If we want update-like behaviour.

[Ch4s3]

@palkan that solution to return a bool might be the way to go.

I think @ebihara99999 might be on vacation, so let's keep this open and revisit it all soon.

[ebihara99999]

@Ch4s3 @palkan
Thank you for comments! I'm back and can deal with the issue.

now it makes 2 queries instead of one

Of course, it isn't good, but there is an advantage - readability, and it's not a big problem for now . It would be kind of compromise between performance and readability (I think comments are needed when #update_all is used. Other people don't know why to use #update_all)

As for race conditions, there would be a better way than using #update_all only to avoid them: had better use #lock! method, which makes it more readable to avoid race conditions. It may be true that #update_all makes 1 query and therefore race conditions will not happen, but #update_all doesn't seem to be intended to avoid race conditions.

However in fact, I hadn't considered race conditions too. So I would appreciate it if you told me if it's popular to use #update_all to avoid race conditions. Then I must fix as @palkan suggests.

[palkan]

@ebihara99999
Using lock! is not a good idea, it can cause performance issues. Library users don't know that there is a lock somewhere inside. That's a very dangerous side effect.

In general, we shouldn't care about user-level race conditions. Yes, #update_all is not so strong as lock, but that's enough to not introduce race conditions internally, within the library.

According to "readability".
Libraries should have readable and user-friendly API, but not internals.

[ebihara99999]

@palkan
I got why that's bad, and then it would be a best way to implement as you suggested.
What do you think @Ch4s3 ? If it's OK, I will open a PR soon!

[Ch4s3]

I agree with @palkan here. I'd rather sacrifice readability internally than risk a race condition.

[joshbuker]

This was resolved by PR #98, but it seems the Github hooks are case-sensitive. Closing this out manually to match.