Fix stale shared data in tests - call ::InertiaRails.reset! before each rspec example

[coreyaus]

Notes

We're seeing an issue with shared_data making our specs non-deterministic, depending on the running order of our tests (i.e. where a particular controller spec passes individually but fails if it's run after another controller spec).

The example code below illustrates the problem we're seeing

# groups_controller.rb
class GroupsController < ApplicationController
  inertia_share do
    { someGroupsControllerData: true }
  end

  def index
    render inertia: 'Groups/Index', props: { groups: [] }
  end
end

# people_controller.rb
class PeopleController < ApplicationController
  def show
    render inertia: 'People/Show', props: { some_people_data: {} }
  end
end

# people_controller_spec.rb
require 'rails_helper'
describe PeopleController, controller: true do
  describe '#show', inertia: true do
    it 'should not have data from a different controller' do
      expect(inertia.props.keys).to contain_exactly(:some_people_data)
    end
  end
end

When we run the people_controller_spec.rb by itself it all works fine.

However, if we run the test suite and the people_controller_spec.rb runs after the groups_controller_spec.rb then we get the following failure message, indicating that the shared_data is not being successfully cleared between specs:

Failure/Error: expect(inertia.props.keys).to contain_exactly(:some_people_data)

expected collection contained: [:some_people_data]
actual collection contained: [:some_people_data, :someGroupsControllerData]
the extra elements were: [:someGroupsControllerData]

Proposed solution in this PR

You might have preferred ways of resolving this issue but for the sake of suggesting a solution I can confirm that the 1-line change in this PR resolves the issue we're seeing by adding ::InertiaRails.reset! within the config.before(:each, inertia: true) do block (that reset is called in the middleware layer but I'm guessing it's skipped when running in the test environment)

Definitely no dramas if you'd prefer to solve this another way though (i.e. no worries if you need to close this PR unmerged in favour of some other approach!)

Please let me know if it'd be helpful for me to try to add a failing spec to the inertia_rails test suite and I can have a go at making that happen based on the example above.

Cheers for your great work on this excellent library!

[BrandonShar]

This issue is certainly hard to squash... Thanks for creating this @coreyaus ! I think your solution makes sense, but I'd like to chat with @bknoles a bit before we merge because issues very similar to this have cropped up a couple of times and I'm starting to feel like we're chasing our tail a bit.

Let me know if this is a blocker for you since I expect we could merge this regardless of whatever solution we come up with.

[coreyaus]

Thanks @BrandonShar!

That's useful background to understand. This isn't a major blocker for me - I've just tweaked our tests so they're a little less strict (e.g. ensuring all the data we expect to have is definitely included, and ignoring any additional shared props that are unexpectedly leaking through). That's fine for us at the moment and doesn't weaken our tests too much.

Sing out if we can help with anything on this one, and have a wonderful end to the week!

[clemens]

I think at the root here is a potential design problem – or at least something that's IMO unexpected behavior when using this as a seasoned Rails engineer.

What I mean is: The things that are declared as "setters" in the InertiaRails module are de facto global variables that are set during a request and then cleaned up at the end of the request. The presence of both before_action and thread_mattr_accessors for inertia_share also hints that there might be something fishy with this approach.

What I'm unclear about is why this is even needed. Wouldn't this be a classic use case for class_attribute, who's very purpose is to provide inheritable attributes which can be overridden in subclasses without messing with the parent class?

In other words, something like this:

module InertiaRails
  module Controller
    extend ActiveSupport::Concern

    included do
      class_attribute :shared_plain_data, :shared_blocks
      self.shared_plain_data = {}
      self.shared_blocks = []
    end

    class_methods do
      def inertia_share(shares, &block)
        shared_plain_data.merge!(shares) if shares.any?
        shared_blocks << block if block.present?
      end
    end

    def shared_data
      # This can now use self.class.shared_plain_data and self.class.shared_blocks respectively
    end
  end
end

Is there something I'm overlooking? Shouldn't such an approach work?

[BenMorganMY]

I just ran into this today with user scopes leaking into each other from inertia_share current_user: -> { ... }.

@BrandonShar are you able to look into what @clemens has highlighted?

[PedroAugustoRamalhoDuarte]

Hello, @coreyaus @BenMorganMY, i want to help with this issue. Can you guys think we can replicate this with one test case in rspec?

[BenMorganMY]

Looking back, we experienced it when we were trying to set an instance variable that caches the organizations a user belongs to:

class Dashboard::BaseController < ApplicationController
  include InertiaCurrentUser

  before_action :load_organizations

  private

  def load_organizations
    organizations = provider_scope(Organization)
    organization_id = cookies[:organization_id]

    # We load all of the organizations since it will later be used by inertia
    # to be written to the document.
    @organizations = organizations.to_a

    @organization = if organization_id.present?
      organizations.find { _1.id == organization_id } || @organizations.first
    else
      @organizations.first
    end
  end
end

module InertiaCurrentUser
  extend ActiveSupport::Concern

  included do
    inertia_share current_user: lambda {
      # There appears to be a bug with inertia calling this current_user lambda
      # in the devise registrations/confirmations. Return early to prevent calls
      # to `@organizations` or `current_user` both of which will be `nil`.
      return if current_user.nil?

      # Error happens here where it calls `organizations` on what will be `nil`
      organizations = (@organizations || current_user.organizations).map do
        { id: _1.id, description: _1.description }
      end

      current_user.as_json(
        only: %w[id email role first_name last_name api_key],
        include: [
          privilege_set: { except: %i[id user_id created_at updated_at] }
        ]
      ).merge(organizations:)
    }
  end
end

So, at initial appearance, the current_user is nil because the user isn't signed in. But we have noticed that when it should be failing, it is passing and loading the current_user. Also, the inertia_share isn't loaded in the devise controllers. It should never even call this lambda.

[buhrmi]

Just throwing my unqualified question in here: Why is the shared data stored in the global InertiaRails module and not on the controller instance? The controller instance is thrown away after each request, so wouldn't it make more sense to store the shared data there?

[bknoles]

Yea, great discussion here. I think we're in need of a refactor away from storing shared data in a variable on the module.

@clemens comment (echoed by @buhrmi ):

The things that are declared as "setters" in the InertiaRails module are de facto global variables

is spot on, and all our "fixes" are just workarounds for this fundamental design decision. The history behind it is that when we first built the library we ported over what we saw in the Laravel library fairly closely. Ruby != PHP, however, and the fact that classes are cached in a production process leads to all these complications.

Storing data at the controller level was also suggested by @ledermann quite awhile ago.

Backing up a bit, what we really want is for shared data to be part of a request. Is a controller class variable the right place for that? Or are there any other options? What do you think @BrandonShar ?

[PedroAugustoRamalhoDuarte]

@bknoles thanks for the fast response. In my opnion in the rails way the controller is almost the request, i think a class variable is the right place. I will revive the PR from @ledermann to we test in our apps.

Other problem is that if there is a bug is hard to reproduce, the best approach is to make a failing spec, but its hard to accomplish i have try several ways without succeded

[PedroAugustoRamalhoDuarte]

#111

[bknoles]

Actually, I think storing per-request data in a class_attribute has issues also. It's still a global variable; it's just scoped to the controller instead of the library.

As of today, I don't think it will cause problems if you use InertiaRails "correctly", since you can only pass either plain data (which can safely be shared between requests) or a proc that gets executed via instance_exec. But if someone were to conditionally run inertia_share, they'd be leaking data across requests. A contrived example:

class MyController
  before_action :share_some_data, only: [:show]
  
  def index
  end

  def show
  end

  protected

  def share_some_data
    self.class.inertia_share({x: 5})
  end
end

The index action is going to receive {x: 5} unintentionally. A more realistic example would be if unsuspecting Future Us tried to mimic Rails before_action syntax like:

inertia_share {x: 5}, only: [:show]

I feel like this comment is a good framing... class variables should be set at boot time. We want to set shared data dynamically during a single request, which is just something different.

I think the real answer is to change inertia_share to be an instance method that adds to an instance variable. It would be intended to run via the default controller callbacks:

class MyController
  before_action :set_some_shared_data, only: [:certain, :actions] # can safely make this conditional!
  
  protected
  def set_some_shared_data
    inertia_share {x: something_dynamic} # no need to wrap this in a lambda anymore! (ignoring lazy eval)
  end
end

That would be a breaking change, so we'd do a major version bump.

I'm open to other suggestions, but I'm not aware of a way to set instance or per-request data from a class method.

[buhrmi]

class_attributes can be accessed both at the class-level and instance-level. When assigning at the instance-level, the value is scoped to the instance. See https://apidock.com/rails/Class/class_attribute:

Instances may overwrite the class value in the same way:

Base.setting = true
object = Base.new
object.setting # => true
object.setting = false
object.setting # => false
Base.setting # => true

So I would assume that @PedroAugustoRamalhoDuarte's implementation works fine because it only ever assigns on instance-level.

But yeah, I agree with @bknoles that we should use "simple" instance variables instead just to rule out any accidental assignments.

[bknoles]

Thanks for the link @buhrmi

I'd definitely prefer to keep the existing API if we can feel confident that the class_attribute changes are safe. A breaking change is always a bummer.

At a glance, I think this line is setting a class level variable and not an instance variable, but the implementation of class_attribute is a bit tough to parse from the current source.

I think I'm going to actually have to write some code and tests to fully grok it 😅. Thanks everyone for all the good examples and suggestions!

[buhrmi]

Yes, that line is setting a class-level variable. But it's empty. Maybe it should be frozen to prevent mutation. The only place where actual data is assigned is here, which references the class-level and makes it instance-level. At least that's how I parse this whole thing in my head.

[PedroAugustoRamalhoDuarte]

Actually, I think storing per-request data in a class_attribute has issues also. It's still a global variable; it's just scoped to the controller instead of the library.

As of today, I don't think it will cause problems if you use InertiaRails "correctly", since you can only pass either plain data (which can safely be shared between requests) or a proc that gets executed via instance_exec. But if someone were to conditionally run inertia_share, they'd be leaking data across requests. A contrived example:

class MyController
  before_action :share_some_data, only: [:show]
  
  def index
  end

  def show
  end

  protected

  def share_some_data
    self.class.inertia_share({x: 5})
  end
end

The index action is going to receive {x: 5} unintentionally. A more realistic example would be if unsuspecting Future Us tried to mimic Rails before_action syntax like:

inertia_share {x: 5}, only: [:show]

I feel like this comment is a good framing... class variables should be set at boot time. We want to set shared data dynamically during a single request, which is just something different.

I think the real answer is to change inertia_share to be an instance method that adds to an instance variable. It would be intended to run via the default controller callbacks:

class MyController
  before_action :set_some_shared_data, only: [:certain, :actions] # can safely make this conditional!
  
  protected
  def set_some_shared_data
    inertia_share {x: something_dynamic} # no need to wrap this in a lambda anymore! (ignoring lazy eval)
  end
end

That would be a breaking change, so we'd do a major version bump.

I'm open to other suggestions, but I'm not aware of a way to set instance or per-request data from a class method.

Yes, using class_attribute maybe its not the best ideia, there are several created issues with rails with this problem, and its not used for our use case. The use case for class_attribute is more configuration at boot time.

I like you suggestion, using before_action all the time should work, i am searching a solution without breaking changes

[bknoles]

Awesome! Last night I created a failing test last night that demonstrates the issue with class_attribute. It indeed acts as a class level variable that is shared across every controller instance.

I'll check these changes against that test and see what it tells us.

[PedroAugustoRamalhoDuarte]

@bknoles can you share this test with me to I add them to my PR too?

[bknoles]

Sorry all! Couldn't carve out time for this in the last couple weeks.

I'm going to merge this PR and cut a new patch release. It's an improvement, and it doesn't make sense to hold it up. I'll make a new issue to continue the larger refactoring discussion.

[bknoles]

Released in v3.1.4