Skip to content

Conversation

@bdewater-thatch
Copy link

I ran into #145, but figured that inside a Rails app the least surprising thing would be for req to be like an ::ActionDispatch::Request instead of patching my own remote_ip method in Rack::Attack::Request.

@grzuy
Copy link
Collaborator

grzuy commented Oct 22, 2025

Hey @bdewater-thatch,

Thanks for the PR!

That's interesting, never though about that.

I think it you're right that would be the most unsurprising implementation if we were doing this for the first time.
Considering the amount of code out there calling methods on these req objects, we need to kind of "make sure" we're not breaking users rule blocks...

Need to take a deeper look to assess that I guess...

@santib thoughts?

@santib
Copy link
Collaborator

santib commented Oct 22, 2025

@grzuy I don't know how we could check that, if even possible. But changing the base class seems like a breaking change to me, so I think we could do this but we'd need to release a new major version and add appropriate warnings in the CHANGELOG.

@bdewater-thatch
Copy link
Author

Hey @grzuy, long time no see :)

I agree there's risk here of breaking changes but hopefully not too bad. The interfaces of both are very similar if not identical.

My first sanity check was:

Rack::Request.instance_methods - ActionDispatch::Request.instance_methods
# => [] 

Then the sources itself:

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