rakismet icon indicating copy to clipboard operation
rakismet copied to clipboard

How does the model have access to the request environment?

Open jjb opened this issue 8 years ago • 7 comments

(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?

jjb avatar Oct 30 '16 19:10 jjb

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.

joshfrench avatar Oct 30 '16 20:10 joshfrench

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?

jjb avatar Oct 30 '16 21:10 jjb

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.

joshfrench avatar Oct 30 '16 22:10 joshfrench

Ahhh.

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

jjb avatar Oct 31 '16 00:10 jjb

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.)

joshfrench avatar Oct 31 '16 02:10 joshfrench

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?

jjb avatar Oct 31 '16 02:10 jjb

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?

joshfrench avatar Oct 31 '16 17:10 joshfrench