opener-project / callback-handler

Tool for handling different callback URLs based on their protocol and/or syntax.
Other
0 stars 2 forks source link

Callback Chain Still 400 #3

Open DylanGriffith opened 9 years ago

DylanGriffith commented 9 years ago

I don't believe that the fixes for #1 actually fixed the issue. If I could reopen that issue I would but alas I cannot. I am still having the same problem after updating the gem.

Based on the HttpClient documentation it would appear that client.post(url, body, headers) is exactly equivalent to client.post(url, :body => body, :header => headers). So the patch submitted should not have changed anything.

The problem is not the :body => body in #post since that is just stylistic, the problem is actually in the fact that the webservice is wrapping the argument in :body => and then callback-handler is not expecting that.

Again see opener-webservice (2.0.0):

callback_handler.post(url, :body => output, :header => headers)

This results in params being the hash :body => output, :header => headers in the following:

        def process(url, params = {})
          body = JSON.dump(params)

          http.post(url, body, HEADERS)
        end

Thus the JSON that gets posted looks like {"body":{"input":"...},"header".

You may not be running into this issue with the later version of webservice (I have not been able to use that with the other gems).

It is not clear to me if the #process method should be handling the spliting of :body and :header from params. I see 3 solutions:

  1. If it is meant to receive params as a hash like :body => something, :header => something (as it is from webservice 2.0.0) then the following should work:
        def process(url, params = {})
          body_data = params[:body] || {}
          header_data = params[:header] || {}
          body = JSON.dump(body_data)

          http.post(url, body, HEADERS.merge(header_data))
        end
  1. If 1 should be supported and the process method must remain compatible with a hash input that is not wrapped in :body => then the following should work:
        def process(url, params = {})
          body_data = params[:body] || params
          header_data = params[:header] || {}
          body = JSON.dump(body_data)

          http.post(url, body, HEADERS.merge(header_data))
        end
  1. If the process method is not meant to be receiving params that looks like {:body => something, :header => something_else} then the webservice needs to change. Probably to:
callback_handler.post(url, output)
yorickpeterse commented 9 years ago

You have to use 2.1 of opener-webservice in combination with opener-callback-handler, version 2.0 (or older) doesn't work with this combination.

The current version of opener-webservice uses the following code for submitting data to a callback URL: https://github.com/opener-project/webservice/blob/e3b2abbd1c973f42ec5063fa7ea46cb0f06634d8/lib/opener/webservice/server.rb#L262. In combination with the latest webservice versions this results in proper input bodies, either when using JSON or regular POST fields.

Basically what we're changing across the entire setup is:

  1. Better error handling, logging and monitoring
  2. Support for JSON input/output
  3. Support for using input/output URLs instead of throwing documents around directly (this vastly reduces the size of POST payloads)
  4. Support for using whatever callbacks opener-callback-handler supports
  5. A much better way of running/maintaining daemons
  6. Support for uploading output to Amazon S3 (see item 3)

This however requires a bunch of changes that aren't compatible with the older versions of the webservices, hence the major version bumps across the project.

DylanGriffith commented 9 years ago

Ok well then the bug appears to be in the webservice. It should declare a dependency on a version of opener-callback-handler with which it is compatible. I figured the newer version of webservice might fix the problem, but that doesn't help when none of the other projects have been cut over to using the newer webservice.

Until such time that the other projects are actually able to use the new webservice (which requires at least changing all MyServer < Webservice to MyServer < Webservice::Server) then I won't actually be able to work with these projects. If there were an interim fix to make things play nicely together that may be more helpful.

I appreciate your responsiveness, but it is probably helpful to others that might run into the same issue as me (and possibly may be discouraged from working with this project) if we introduce an interim fix for incompatible versions of these various gems.

DylanGriffith commented 9 years ago

Although in saying that it is now apparent to me that you have just changed all the other gems I need. Thank you.

Still it might be worth noting that webservice (2.0.0) is broken (since it does not work with all the gems that it says it will in it's dependency list). That may hopefully save somebody else all the screwing around I did to troubleshoot the broken callback chain.

yorickpeterse commented 9 years ago

Ok well then the bug appears to be in the webservice. It should declare a dependency on a version of opener-callback-handler with which it is compatible. I figured the newer version of webservice might fix the problem, but that doesn't help when none of the other projects have been cut over to using the newer webservice.

All components providing webservices have been updated to use the latest versions of opener-webservice and opener-callback-handler (and various other Gems they depend on). I finished doing this earlier today so perhaps I fixed it after you looked into this particular problem (meaning you didn't have access yet to the changes in question).

If you have a specific list of components of which you know they haven't been updated yet please do tell. Considering the list is quite large it's certainly possible I missed a few.

I appreciate your responsiveness, but it is probably helpful to others that might run into the same issue as me (and possibly may be discouraged from working with this project) if we introduce an interim fix for incompatible versions of these various gems.

Existing releases can't be changed, so I don't really see what can be done about this. We're also not going to lock down to specific minor/patch releases as this makes management extremely painful. Due to incompatibility changes major versions were incremented, instead of minor versions. Some of the version increments were a bit sloppy in the past but this should be sorted out by now.