Unnecessary saves called on ActiveRecord belongs_to relation

[zpencerq]

When creating a new object that has a relationship, for example (using the employee-directory example app)

{
  "data": {
    "type": "teams",
    "attributes": {
      "name": "Foobar Ltd."
    },
    "relationships": {
      "employees": {
        "data": [
          {
            "type": "employees",
            "id": "101"
          }
        ]
      },
      "department": {
        "data": {
          "type": "departments",
          "id": "4"
        }
      }
    }
  }
}

Causes calls to Employee#save and Department#save. Obviously these calls to #save are no-ops but it requires a work-around to support using object level authorization (as these cause authorization errors if the user isn't allowed to create/update these objects).

I haven't figured out what's causing it yet but my work-around so far is to guard my authorize with #changed?, like so:

class ApplicationResource < Graphiti::Resource
  include Pundit

  ...

  before_save do |model|
    action = (
      if model.new_record?
         :create?
      else
         :update?
      end
    )

    authorize(model, action) if model.changed?
  end
end

[caseyprovost]

@zpencerq I believe these persistence calls are wrapped in a transaction so performance impacts should be negligible. However, I am happy to dig into the logic if it is causing problems.

[zpencerq]

@caseyprovost I managed to work around this as shown since it was affecting my authorization scheme. It just seemed to be a logic bug as I don't think we expected a save to be called on those related models. 🤷‍♂

[richmolj]

I think this might be WAD. I think you'd want this code to validate persistence operations in any case, except for the if model.changed?. We call save when no changes because you might want side effects, like "increment team_count when a team is adding an employee".

That said, I can see how this violates principle of least surprise and an option to only save when attributes are present would make sense to me.

[wagenet]

Adding the following to the top of persist_with_relationships may resolve this issue, though it feels a little hacky.

# If all we have is an id we're not really updating it
if meta[:method] == :update && attributes.keys == [:id] && relationships.empty?
  return self.class._find(attributes).data
end

[aribouius]

Another related issue I've noticed is that the associated model(s) also get validated. While negligible in most cases, some models may have expensive validation procedures, which could impact performance.

We actually had an issue recently where we had a DB record that had become invalid due to a change in its model validation rules. No one noticed until a separate model attempted to associate a new record to it, which would fail due to the extended validation.

[richmolj]

Yeah, this issue is definitely on my radar as high priority, but there are other high priorities so not sure when I'll get around to it. Happy to look at a PR if you get the time! ❤️