Pass request headers to receive_web_request #1415

albertsun · 2016-04-09T00:01:02Z

For some web services (like one I'm writing to receive Twilio text messages and will send a pull for) info in the headers is important in order to verify or understand the request.

This adds a fourth options headers={} to receive_web_request. For any custom agents people might have that aren't committed here, it does an arity check to not send headers if the method doesn't support them.

I also wonder if we should just pass the entire request object http://guides.rubyonrails.org/action_controller_overview.html#the-request-object instead of splitting it across method, format and headers arguments.

dsander · 2016-04-09T09:07:27Z

app/models/agent.rb

    if respond_to?(:receive_webhook)
      Rails.logger.warn "DEPRECATED: The .receive_webhook method is deprecated, please switch your Agent to use .receive_web_request."
      receive_webhook(params).tap do
        self.last_web_request_at = Time.now
        save!
      end
-    else
+    elsif method(:receive_web_request).arity == 3


It's a nice thought to avoid breaking custom agents, but as of now we do not support agents in gems and normally just ensure that we do not break agent configurations with updates. I think it would be fine to modify the existing agents and always assume an arity of 4. What do you think @cantino ?

dsander · 2016-04-09T09:09:16Z

Good idea! Maybe you could pass the request object as a fifth argument for agents that need access to everything, but keep the simplicity for the other agents?

albertsun · 2016-04-09T15:54:26Z

Good idea. Added the request object as a fifth argument. The thought for keeping the arity check was kind of in case anyone is using agents that haven't been merged upstream here or for future agents to be able to write receive_web_request in a simpler manner. But could go either way on that, happy to just have it always support 5 arguments.

cantino · 2016-04-10T17:42:34Z

app/models/agents/data_output_agent.rb

@@ -160,7 +160,7 @@ def push_hubs
      interpolated['push_hubs'].presence || []
    end

-    def receive_web_request(params, method, format)
+    def receive_web_request(params, method, format, headers={}, request=ActionDispatch::Request.new({}))


This could just be receive_web_request(params, method, format), right? Since we support both?

Yep, it and other current agents could be like that.

cantino · 2016-04-10T17:43:15Z

Good idea! We'll need to update the wiki too.

…uest

…bject

…er_location agents

albertsun · 2016-04-11T22:28:06Z

Ok, so I've rebased the branch off latest master. Everything look ok to merge now? After merging I can update the documentation on that wiki page.

dsander · 2016-04-11T22:57:47Z

I am still not a huge fan of the arity check as it introduces complexity that we probably never be able to remove afterwards. If we go with it couldn't the receive_web_request method definition of existing agents stay the same as before?

albertsun · 2016-04-11T23:10:56Z

I don't feel strongly about the arity check either way. Thought it could be nice to preserve some agents with the simpler syntax. Happy to take it out and just update every receive_web_hook call to take 5 arguments if everyone agrees that's the way to go. @cantino thoughts on that?

knu · 2016-04-12T11:13:27Z

I'd consider five arguments as way too many. Given the fact that the request object should include everything including params, method, format and headers, I'd go with receive_web_request(request).

knu · 2016-04-12T11:46:23Z

I think retaining compatibility by checking the arity is preferable here, because one with an in-house/personal agent wouldn't even know it got broken by the method signature change until it received the first request after upgrading and then found a failure in the application log afterwards.

As for @dsander's concern, we could probably add a check in Agent loader and show a banner to admins after login that urges them to update their agents if the check fails. After a certain amount of time (grace period) it'd be fine to lift it and remove the arity check.

cantino · 2016-04-13T04:44:09Z

I'd probably do this:

if method(:receive_web_request).arity == 3
  handled_request = receive_web_request(params, method, format)
else
  handled_request = receive_web_request(request)
end

Maybe not worry about the banner for now, though?

dsander · 2016-04-13T08:15:42Z

I think retaining compatibility by checking the arity is preferable here, because one with an in-house/personal agent wouldn't even know it got broken by the method signature change until it received the first request after upgrading and then found a failure in the application log afterwards.

I would assume the in house agent had specs which would run and fail before the Huginn instance is updated :)

As for @dsander's concern, we could probably add a check in Agent loader and show a banner to admins after login that urges them to update their agents if the check fails. After a certain amount of time (grace period) it'd be fine to lift it and remove the arity check.

That would still break for users that update very infrequently. If we really want to maintain backwards compatibility we can never remove it.

Since we do not support external Agents yet I consider our Concern an internal API and thus not worry about breaking something external. If we go with it I would prefer @cantino approach without modifying the existing agents.

knu · 2016-04-13T09:04:18Z

I would assume the in house agent had specs which would run and fail before the Huginn instance is updated :)

I'd really hope so, but even the fact that many users are running Huginn in development was beyond my expectation, and I wouldn't dare to assume much. ;D

That would still break for users that update very infrequently. If we really want to maintain backwards compatibility we can never remove it.

I mean after a grace period backward compatibility code should be removed and the arity check in Agent loader be turned into part of sanity check, refusing to boot on failure instead of relying on backward compatibility code and showing a banner. I hopefully think it is not asking too much that infrequent updates sometimes fail and require admins to take action.

One problem we have is we do not really have a channel for announcing breaking changes to users/admins. I first thought we could add a migration to perform the arity check for existing agents, but it turned out that it'd be unrealistic to load all agents in a migration process. So, I thought it could be an option to take a grace period and show a prewarning, if not this time.

albertsun · 2016-04-13T15:25:30Z

Ok. I've modified it to take either 1 argument (request) or 3 arguments (same signature as before) and left the existing agents using 3 arguments.

I wonder if for now we just keep this as a non-breaking change but that at some point in the future we can batch together a whole bunch of breaking changes at once and push them as a versioned release or something, once there is a method for announcing breaking changes and the like.

cantino · 2016-04-13T19:57:46Z

I agree, we should bundle some breaking stuff into a versioned update sometime soon. (How does one version a Rails application?)

Is this ready to go?

cantino · 2016-04-13T19:58:03Z

app/models/agent.rb

@@ -100,6 +100,10 @@ def receive_web_request(params, method, format)
    ["not implemented", 404]
  end

+  # alternate method signature for receive_web_request


Do we need this comment?

I think it helps anyone reading the code to see that there's an alternate way for them to define receive_web_request but if you think it muddies things up too much I'll remove it.

albertsun · 2016-04-13T20:40:50Z

Other than whether or not to leave that comment in, I believe this is ready to go

albertsun · 2016-04-14T13:29:15Z

Closing/re-opening to trigger a travis re-build to hopefully clear up a transient bundler install failure

dsander · 2016-04-15T08:21:16Z

Looks good to me, I don't feel strongly about the comment.

knu · 2016-04-15T11:02:26Z

app/models/agent.rb

@@ -149,15 +153,21 @@ def update_event_expirations!
    end
  end

-  def trigger_web_request(params, method, format)
+  def trigger_web_request(request)
+    params = request.params.except(:action, :controller, :agent_id, :user_id, :format)
    if respond_to?(:receive_webhook)


Do we still need this?

It's being used below.

I mean, should we still offer this backward compatibility? It's been there with a deprecation warning for years, so I thought it might be the time to get rid of it.

cantino · 2016-04-16T03:55:56Z

Looks good to me!

albertsun · 2016-04-17T20:38:04Z

Great, thanks!

dsander reviewed Apr 9, 2016
View reviewed changes

albertsun force-pushed the feature/request-headers-in-web-request branch from fb9d315 to 2a3fa43 Compare April 10, 2016 16:51

albertsun mentioned this pull request Apr 10, 2016

Twilio receive text message agent #1418

Merged

cantino reviewed Apr 10, 2016
View reviewed changes

albertsun added 4 commits April 11, 2016 18:16

Pass request headers through to agents that implement receive_web_req…

58f6307

…uest

convert Agents' receive_web_request to have headers optional

758573f

receive_web_request can also now receive the ActionDispatch request o…

9ddb4e9

…bject

use the simpler version of receive_web_request for data_output and us…

a4e0112

…er_location agents

albertsun force-pushed the feature/request-headers-in-web-request branch from 05c35db to a4e0112 Compare April 11, 2016 22:26

modify receive_web_request to take either 1 or 3 arguments

5da4053

cantino reviewed Apr 13, 2016
View reviewed changes

format.to_s based on @cantino review

6a9ad21

albertsun closed this Apr 14, 2016

albertsun reopened this Apr 14, 2016

knu reviewed Apr 15, 2016
View reviewed changes

cantino merged commit ee504ba into huginn:master Apr 16, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass request headers to receive_web_request #1415

Pass request headers to receive_web_request #1415

albertsun commented Apr 9, 2016

dsander Apr 9, 2016

dsander commented Apr 9, 2016

albertsun commented Apr 9, 2016

cantino Apr 10, 2016

albertsun Apr 11, 2016

cantino commented Apr 10, 2016

albertsun commented Apr 11, 2016

dsander commented Apr 11, 2016

albertsun commented Apr 11, 2016

knu commented Apr 12, 2016

knu commented Apr 12, 2016

cantino commented Apr 13, 2016

dsander commented Apr 13, 2016

knu commented Apr 13, 2016

albertsun commented Apr 13, 2016

cantino commented Apr 13, 2016

cantino Apr 13, 2016

albertsun Apr 13, 2016

albertsun commented Apr 13, 2016

albertsun commented Apr 14, 2016

dsander commented Apr 15, 2016

knu Apr 15, 2016

cantino Apr 16, 2016

knu Apr 18, 2016

cantino commented Apr 16, 2016

albertsun commented Apr 17, 2016

Pass request headers to receive_web_request #1415

Pass request headers to receive_web_request #1415

Conversation

albertsun commented Apr 9, 2016

Choose a reason for hiding this comment

dsander commented Apr 9, 2016

albertsun commented Apr 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cantino commented Apr 10, 2016

albertsun commented Apr 11, 2016

dsander commented Apr 11, 2016

albertsun commented Apr 11, 2016

knu commented Apr 12, 2016

knu commented Apr 12, 2016

cantino commented Apr 13, 2016

dsander commented Apr 13, 2016

knu commented Apr 13, 2016

albertsun commented Apr 13, 2016

cantino commented Apr 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertsun commented Apr 13, 2016

albertsun commented Apr 14, 2016

dsander commented Apr 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cantino commented Apr 16, 2016

albertsun commented Apr 17, 2016