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(#1975): Allow to use before/after/rescue_from methods in any order when using mount #2384

Merged
merged 2 commits into from
Dec 12, 2023

Conversation

jcagarcia
Copy link
Contributor

This PR tries to "fix" the issue described in #1975 .

Right now, as we can read in our README, declarations as before/after/rescue_from must be placed before mount in a case where they should be inherited.

However, I think this behavior arises from the existing implementation rather than being an intentional design. This occurs because when the mount method is invoked, a new Endpoint instance is generated, inheriting the previously registered before/after/rescue_from methods. However, if additional before/after/rescue_from methods are executed after the mount operation, the mounted endpoints remain unrefreshed.

Considering that:

  1. Whenever a method is run within a Grape::API class, it is stored in the @setup variable in the sequence in which it was executed.
  2. Upon completion of the execution, we will have an array containing the methods that were executed in the precise order they were called.

My proposal to address this issue is:

  1. When executing any method, such as before/after/rescue_from (in fact, any method, not limited to these), verify whether there have been preceding mount method calls for this particular Grape::API class stored within the @setup variable.
  2. If affirmative, re-apply the mounting of the same endpoint to update and refresh the inherited configurations.

By doing so, at the end of this process, all mounted endpoints will inherit all the executed methods within the Grape::API scope.

@grape-bot
Copy link

1 Warning
⚠️ There're library changes, but not tests. That's OK as long as you're refactoring existing code.

Generated by 🚫 Danger

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

👏

lib/grape/api.rb Outdated Show resolved Hide resolved
@dblock
Copy link
Member

dblock commented Dec 12, 2023

Given that this broke no specs (impressive) I say we merge, thank you! Want to rebase CHANGELOG and potentially edit my nit?

@jcagarcia
Copy link
Contributor Author

Given that this broke no specs (impressive) I say we merge, thank you! Want to rebase CHANGELOG and potentially edit my nit?

I've just rebased and applied your suggestions! :) Happy to help!

@dblock dblock merged commit e37831c into ruby-grape:master Dec 12, 2023
31 checks passed
@jcagarcia jcagarcia deleted the issue-1975 branch December 12, 2023 14:25
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.

3 participants