joshfrench / rakismet

Easy Akismet and TypePad AntiSpam integration for Rails
MIT License
355 stars 46 forks source link

How does the model have access to the request environment? #44

Open jjb opened 7 years ago

jjb commented 7 years ago

(hi Josh!)

The readme says:

This means that if you are not storing the request variables, you must call spam? from within the controller action that handles comment submissions. That way the IP, user agent, and referer will belong to the person submitting the comment.

I chased user_ip around the code in a loop and am too stupid to figure out how a model can have access to the request environment. Where does this happen?

joshfrench commented 7 years ago

Hey John!

If you're using Rails there's a default middleware which sets those vars for the current request: https://github.com/joshfrench/rakismet/blob/master/lib/rakismet/middleware.rb (Which is why you need to call spam? in situ, or store the details for later recall.)

If you're not using Rails, you'd have to adapt that or set those vars manually.

jjb commented 7 years ago

Gotcha — but where/how in the model does it access those vars? Is that a normal thing that an AR object can do when it's instantiated in a controller action?

joshfrench commented 7 years ago

So that middleware sets the current request data as class-level vars on the Rakismet class, which is just a plain old Ruby object: https://github.com/joshfrench/rakismet/blob/master/lib/rakismet.rb#L30

Then spam? and friends just look up whatever's currently stored in those Rakismet class vars: https://github.com/joshfrench/rakismet/blob/master/lib/rakismet/model.rb#L76

Rakismet has no knowledge of AR, you could include rakismet/model into any kind of object and get the same behavior.

jjb commented 7 years ago

Ahhh.

So I suppose that means rakismet isn't thread-safe?

joshfrench commented 7 years ago

It's not. There's an out-of-date PR that I'd gladly merge if someone wanted to bring that branch up to speed with master: https://github.com/joshfrench/rakismet/pull/35

(Hint hint, nudge nudge.)

jjb commented 7 years ago

Instead of explicitly supporting it with thread-local variables, what do you think of these ideas…

  1. pass the request object into spam? and friends, which will then invoke .user_ip etc on it. this can even be used outside of the context of a controller by passing in some other object with the same api.

    thing = Thing.new(params)
    raise "spam!" if thing.spam?(request)
    thing.save
  2. have include Rakismet::Model invoke attr_accessor :user_ip, :user_agent, :referrer (or maybe namespaced with something safer like rakismet_ or request_). then there is the option to set these non-persited values in whatever context, be it a controller or otherwise. this might simplify the case of

    thing = Thing.new(params)
    thing.user_ip, thing.user_agent, thing.referrer = request.user_ip, request.user_agent, request.referer
    raise "spam!" if thing.spam?
    thing.save

I like 1 a lot better but maybe there are reasons that 2 is more flexible or something.

WDYT?

joshfrench commented 7 years ago

They're both breaking changes, but in hindsight I think explicitly passing the request data in some form is preferable to magically gleaning it from the environment.

The first option would be my choice, as it relieves the user of needing to know which request vars are important.

Open question: if I decide to store my own request info --

@comment = Comment.new({ user_ip: foo, ... })

And later I supply a different request env:

@comment.spam?({ user_ip: bar, ... })

Which value would you expect to take precedence?