Better workaround for performance around unknown attributes?

[jamesst20]

Hi,

Let's say I have an ActiveRecord model MyModel with the following

class MyModel < ApplicationRecord
  attribute :configuration, ApiModels::Configuration.to_type
end

module ApiModels
  class Configuration
    include StoreModel::Model
  
      attribute :id, :number
      attribute :subject, :string
      attribute :sections, ConfigurationSection.to_array_type
    end
end

...

Doing the following

record = MyModel.find_by(...)
record.update(configuration: api_returned_payload)

Will be super slow if my payload contains a lot of unknown keys.

Doing the following would fix the performance issue:

record.update(configuration:  ApiModels::Configuration.from_value(api_returned_payload).as_json(serialize_unknown_attributes: false))

Analyzing using RubyProf gave me this

Is there a better way?

[DmitryTsepelev]

Hi!

I guess the problem is not in the method missing itself, but at fact that Ruby has to go through all fields (as you said, there are many of them) and do some work to persist them. There's no "static" way to do this (because attributes are unknown :) ).

First thing you can do is to turn it on globally StoreModel.config.serialize_unknown_attributes = false.
Secondly, if you do not care about those fields, you can just filter them somewhere before saving (e.g., api_returned_payload.slice(:permitted1, permitted2))

Also, I think we could try to patch the update method to accept serialize_unknown_attributes: and pass it down, but I'd prefer to not mess with the ActiveRecord code.

[jamesst20]

Hi!

I guess the problem is not in the method missing itself, but at fact that Ruby has to go through all fields (as you said, there are many of them) and do some work to persist them. There's no "static" way to do this (because attributes are unknown :) ).

First thing you can do is to turn it on globally StoreModel.config.serialize_unknown_attributes = false. Secondly, if you do not care about those fields, you can just filter them somewhere before saving (e.g., api_returned_payload.slice(:permitted1, permitted2))

Also, I think we could try to patch the update method to accept serialize_unknown_attributes: and pass it down, but I'd prefer to not mess with the ActiveRecord code.

Hi,

Thanks for the quick reply!

I believe the workaround I proposed is more maintainable then using .slice(...) because it doesn't force me to duplicate the list of expected attributes (it can stay in the "models") and it also works for nested attributes without extra effort.

Doing StoreModel.config.serialize_unknown_attributes = false doesn't not improve performance because it still try to persist everything into the database and has only impact to the .as_json method. It will still spam call the undefined method/attribute catch.

Best case scenario would be an extra configuration such as StoreModel.config.persist_unknown_attributes = false that would take care of it.

Maybe this would be possible without patching ActiveRecord? When I pass a StoreModel object to the update method, some method must be called so ActiveRecord can convert it into an SQL value?

[jamesst20]

To clarify about performance, the difference is very huge. Goes from 70 seconds to less than a second. The payload isn't that huge, it's not like I am trying to persist a 1MB payload, but there are a lot of nested stuffs with unknown attributes

[DmitryTsepelev]

Got it, makes a lot of sense 🙂 I guess we could make that shorter with minor patch to AR #attribute method. It will check that the passed type is a StoreModel type and, if it's true — redefine a setter. The redefined setter will do the trick above:

def "#{attribute_name}="(value)
  from_value(value).as_json
end

Also, it might make sense to allow configuring serialize_unknown_attributes per class (because there's no other way to pass it down to that hacky setter).

What do you think? Do you want to try to build it? 🙂

[jamesst20]

Got it, makes a lot of sense 🙂 I guess we could make that shorter with minor patch to AR #attribute method. It will check that the passed type is a StoreModel type and, if it's true — redefine a setter. The redefined setter will do the trick above:

def "#{attribute_name}="(value)
from_value(value).as_json
end
Also, it might make sense to allow configuring serialize_unknown_attributes per class (because there's no other way to pass it down to that hacky setter).

What do you think? Do you want to try to build it? 🙂

I have done some more digging into this and I noticed from_value(value).as_json is not ideal at all because it will append extra attributes.

Let's say I have the following

Model

class MyModel < ApplicaitonRecord
  include StoreModel::NestedAttributes

  attribute :organization, Organization.to_type

  accepts_nested_attributes :organization, reject_if: :all_blank
end

Database

organization = { id: 123, name: "Org 1", email: "email@example.com" }

Then we go ahead and make a request PUT /my_model/1

{ id: 1, organization_attributes: { id: 123, email: "new_email@example.com"} }

I would be expecting to have my email updated and have the name: "Org 1" untouched, but this will nullify the name and any other attributes because the as_json will include NULL values for every defined attributes in the class that is not present in the parameters of the request.

Also, re-assigning those parameters in update(...) will also re-assign the hash directly and overwrite the entire payload despite the fact i'm passing organization _attributes.

Overwritting the setter like so

def organization=(value)
  if self.organization.present? && value.is_a?(Hash)
    self.organization.assign_attributes(value)
  else
    super
  end
end

will take cake for the issue but will bring back the performance issue.

Do you have other idea?

[DmitryTsepelev]

I might be wrong, but there was a quite common trick in forms when you just always resend all attributes — in that case you don't care about empty values, because old values will always be sent. That's prone to races of course, but works.