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

Auto plug middleware for simpler installation #431

Merged
merged 3 commits into from
Oct 9, 2019

Conversation

fatkodima
Copy link
Contributor

Closes #429 and several related issues.

It is hard to test that middleware is actually included by default without generating dummy apps inside spec folder for every tested rails version.
@grzuy maybe you have some ideas how to test this?

Copy link
Collaborator

@grzuy grzuy left a comment

Choose a reason for hiding this comment

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

This is great! Thank you.

I don't think we would be able to escape having some sort of Rails integration testing... I mean, we want to have a failing test if, for example, Rails 6.1 changes the MiddlewareStack behavior and Rack::Attack::Railtie no longer works as expected.

Let me think a bit more about it.

lib/rack/attack.rb Outdated Show resolved Hide resolved
lib/rack/attack/railtie.rb Outdated Show resolved Hide resolved
lib/rack/attack/railtie.rb Outdated Show resolved Hide resolved
@fatkodima
Copy link
Contributor Author

Updated. I dunno how i tested that before locally, but initial implementation was not fully working. We can not use after_initialize to setup middleware because at the moment when that callback is called, middleware stack is already built and frozen. So we can not add our own.

There is a solution: to use initializer. But we can not use simple initializer block (without before, after parts) because, from docs

There is no guarantee that your initializers will run after all the gem initializers, so any initialization code that depends on a given gem having been initialized should go into a config.after_initialize block.

And also there is no way to check if middleware was previously included/removed without using rails' private api, so implementation become a little more complicated than I was expected.

Unfortunately, for some unknown reason, can't make new tests pass for rails 4.2, while everything works as expected while manually testing in a local app.

@grzuy
Copy link
Collaborator

grzuy commented Oct 1, 2019

Unfortunately, for some unknown reason, can't make new tests pass for rails 4.2, while everything works as expected while manually testing in a local app.

I suspect this change (rails/rails@d951558) has something to do with the failing tests in 4.2 and passing in 5.0+.

@grzuy
Copy link
Collaborator

grzuy commented Oct 1, 2019

Thanks for updating and adding tests!

@fatkodima
Copy link
Contributor Author

Maybe, not sure. It's weird to me, that in brand new rails 4.2 app while testing manually in console - everything works, but can't make tests pass. For other versions everything works here and there. I think, this is safe to merge as is.

@grzuy
Copy link
Collaborator

grzuy commented Oct 7, 2019

I understand that it works on Rails 4.2 for you by testing manually via console as of today, but by not having test coverage we are prone to introducing regressions in any future update or refactor.

So I think to be safe we'll want either a working test case or to not make the auto-plug for rails 4.2.

@fatkodima
Copy link
Contributor Author

@grzuy Correct, you are right. Updated to not auto-plug it for rails < 5.

@grzuy
Copy link
Collaborator

grzuy commented Oct 9, 2019

Nice!

Thank you very much for the PR and all the updates!

@grzuy grzuy merged commit 95347e3 into rack:master Oct 9, 2019
@fatkodima
Copy link
Contributor Author

@grzuy You probably should configure github's merge button to merge with squash/rebase to make history cleaner for future debuggings.

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.

Auto plug middleware for simpler installation
2 participants