Reset fields on create or update

[film42]

This adds a new #reset_attributes method which will reset the current state back to the .default_attributes_hash.

Then, we use the new reset attributes method to reset the state before assigning attributes from a response. In this case we expect the response to include an updated record containing all fields that would return from regular rpc search.

Why make this change?

Suppose the server always ignores writing an attribute, but the client attempts to update this attribute anyway:

some_model.update(:the_server_ignores_this => true)

The correct behavior here should be to reset the :the_server_ignores_this attribue back to false but that's not what happens. The attribute will stay true because the server did not explicitly set the value to false(it was omitted from the response), so the attribute was missing from response_proto.to_hash.

But, with the new reset, we'll always refresh the model's state to reflect the server. Always.

cc @liveh2o @brianstien @abrandoned @mattnichols

[sshock]

:shipit: I also tested this with the downstream issue we were looking at and it does indeed fix the problem

[liveh2o]

I don't think this is the correct behavior. This gem has always attempted to maintain the same behavior as Active Record. From our Slack conversation:

garrett.thornburg [11:16 AM]
active record does not check db for changes on update (edited)
Scenario:
rails console 1: update user's username (in sand)
rails console 2: update some other attribute.
rails console 2: user.username is still `nil`
rails console 2: user.reload brings the new value (edited)

ah [11:23 AM]
So that means this is working as expected?

garrett.thornburg [11:24 AM]
Yes and no. Yes because active record does not try to refresh your record. But no because
in the case of abacus, your "stale" record will still have the attributes synchronized
correctly.

However, the reason this data is "stale" actually has nothing to do with how Active Record performs updates, or the state the Active Record object when it is serialized and returned in the RPC response. Instead, it's actually because of the implementation of Protobuf::Message#to_hash in Ruby Protobuf gem. It omits the keys of any fields with nil values. That means we only see the "incorrect" behavior when the value in the server is set to nil (which is only made possible by the custom internal gem we created in the first place).

Ideally, Protobuf::Message#to_hash would not omit fields that are nil, or it would provide an alternative method that could be used here to get a hash of all of the fields. That's a pretty big shift, and a breaking change that is unlikely to happen.

As for resetting the entire state of the Active Remote object with each persistence call, beyond the fact that it diverges from Active Record's behavior, I'm also concerned that it will have unexpected side effects.

I also don't think the scenario laid out in the example above (attempting to update an ignored attribute) is valid. With no changes, that code will do exactly what you want it to: the value if :the_server_ignores_this will be set back to false because it is included in the hash returned from Protobuf::Message#to_hash.

Ultimately, the problem we are attempting to solve with this change is due to the fact that our API implementation is wrong. We should not expect to update one field and have another field magically change. We should fix that problem in the API, rather than changing the behavior of the entire gem.

In light of this, I'm going to close this.

[film42]

@liveh2o I believe you have an incorrect understanding of the problem. The fact that active remote and active record can become out of sync implies a deviation of behavior. Correcting this is in fact bringing active remote back inline with active record behavior. And since we might be using the term "behavior" in different ways, I'm defining it as "if something is changed in active record, that change will also be applied in active remote."

I think the point about having protobuf return a list of all attributes whether nil or not is a valid solution, but it will also introduce a performance hit. This implementation is effectively that solution but optimized to reduce that performance cost. In fact, in https://github.com/liveh2o/active_remote/pull/63 I found it this implementation will be 4x faster.

Back to the example about the the_server_ignores_this attribute being ignored: The server did not set the value in the response because in this case, the server never sets it in the response. Therefore, active remote and active record will be out of sync.

[sshock]

Hi guys,

Sorry I wasn't checking my personal email earlier today. I'll try to remember to read your replies tomorrow and see if I can say anything intelligent about it :)

Phillip

On Mon, Jul 16, 2018 at 2:53 PM, Garrett T notifications@github.com wrote:

@liveh2o https://github.com/liveh2o I believe you have an incorrect understanding of the problem. The fact that active remote and active record can become out of sync implies a deviation of behavior. Correcting this is in fact bringing active remote back inline with active record behavior. And since we might be using the term "behavior" in different ways, I'm defining it as "if something is changed in active record, that change will also be applied in active remote."

I think the point about having protobuf return a list of all attributes whether nil or not is a valid solution, but it will also introduce a performance hit. This implementation is effectively that solution but optimized to reduce that performance cost. In fact, in #63 https://github.com/liveh2o/active_remote/pull/63 I found it this implementation will be 4x faster.

Back to the example about the the_server_ignores_this attribute being ignored: The server did not set the value in the response because in this case, the server never sets it in the response. Therefore, active remote and active record will be out of sync.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/liveh2o/active_remote/pull/64#issuecomment-405378433, or mute the thread https://github.com/notifications/unsubscribe-auth/ABnYQXe2Bgll9y0HtS-GBuwoL5nixn7oks5uHP09gaJpZM4VJtOQ .

[sshock]

Very interesting discussion. Sounds like we are all in agreement about the unlikeliness of changing protobuf to return nils as we don't want that performance hit.

As for resetting the object, my only thought is, perhaps the only reason ActiveRecord doesn't already do a reset is because it was just unnecessary, not because it is wrong or would cause any problem.

However, I have to admit that I too am a little worried about unexpected side effects, but mostly just because I am still new to the rails framework, so I am not confident that the active record on the server side will always start out with a fresh copy of all fields from the DB. If that could ever not be the case, this change would result in the active remote object on the client side being all or almost completely empty (all fields nil).

Phillip