Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix SystemStackError when extending the reload method with Module#prepend #457

Merged
merged 1 commit into from
Sep 16, 2024

Conversation

willnet
Copy link
Contributor

@willnet willnet commented Sep 12, 2024

For example, when using the master branch of activerecord-multi-tenant, if activerecord-multi-tenant and attr_encrypted are listed in the Gemfile in that order, calling the reload method raises a SystemStackError. This happens because activerecord-multi-tenant extends Active Record’s reload method using prepend, while attr_encrypted extends it using an alias method.

Here’s an example of how extending the same method with both prepend and alias methods in that order can result in a SystemStackError

class Hello
  def hello
    'hello'
  end
end

Hello.prepend(Module.new do
  def hello
    super
  end
end)

Hello.class_eval do
  alias orig_hello hello

  def hello
    "#{orig_hello} world"
  end
end

Hello.new.hello #=> SystemStackError

However, reversing the order works:

class Hello
  def hello
    'hello'
  end
end

Hello.class_eval do
  alias orig_hello hello

  def hello
    "#{orig_hello} world"
  end
end

Hello.prepend(Module.new do
  def hello
    super
  end
end)

Hello.new.hello #=> "hello world"

This issue can be resolved by standardizing the method extension to use prepend to avoid conflicts.

…pend

For example, when using the master branch of activerecord-multi-tenant, if activerecord-multi-tenant and attr_encrypted are listed in the Gemfile in that order, calling the reload method raises a SystemStackError. This happens because activerecord-multi-tenant extends Active Record’s reload method using prepend, while attr_encrypted extends it using an alias method.

Here’s an example of how extending the same method with both prepend and alias methods in that order can result in a SystemStackError

```
class Hello
  def hello
    'hello'
  end
end

Hello.prepend(Module.new do
  def hello
    super
  end
end)

Hello.class_eval do
  alias orig_hello hello

  def hello
    "#{orig_hello} world"
  end
end

Hello.new.hello #=> SystemStackError
```

However, reversing the order works:

```
class Hello
  def hello
    'hello'
  end
end

Hello.class_eval do
  alias orig_hello hello

  def hello
    "#{orig_hello} world"
  end
end

Hello.prepend(Module.new do
  def hello
    super
  end
end)

Hello.new.hello #=> "hello world"
```

This issue can be resolved by standardizing the method extension to use prepend to avoid conflicts.
@joshbranham joshbranham self-requested a review September 12, 2024 17:21
Copy link
Member

@joshbranham joshbranham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution @willnet! Are you using official releases or a fork? I can try and get a release cut soon.

@joshbranham joshbranham merged commit e96f4ad into attr-encrypted:master Sep 16, 2024
19 checks passed
@willnet willnet deleted the prepend-reload branch September 20, 2024 07:44
@willnet
Copy link
Contributor Author

willnet commented Sep 20, 2024

@joshbranham Thank you for merging! I'm using attr_encrypted v4.1.0 now. I would appreciate it if you could release the new version to rubygems.org

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants