Issue when payloads have no action

[Napo2k]

Gitapps based on this that want to do something with push payloads (for instance) will fail here:

https://github.com/bradshjg/flask-githubapp/blob/master/flask_githubapp/core.py#L170

I know because I have just suffered this! :D A simple change could be, I think, to do a get with a default value of None action = request.json.get('action', None)

[bradshjg]

Thanks so much for your help. I should be able to recreate this and put in a fix today.

[bradshjg]

Hey @Napo2k looking a bit more at this I believe that since request.json is a dictionary the default behavior should be to return None in the case a key isn't found (which I think is what we want). It looks like later code doesn't expect that key to necessarily exist either, but I could be wrong.

Sorry for the trouble, but could you share a bit more about the circumstances in which you were able to see an error?

[Napo2k]

Hello!

This is my requirements file:

Flask==1.1.2
flask-githubapp     <----- actually pointing to a local version in which I made the changes detailed in issue #14 on this repo, but based on latest master as of 2021/01/13 (that is YYYY/MM/DD)

This is my python version:

~/Work/gitbot: git@master! [09:56:24] ± python --version
Python 3.9.1

I got the error (after doing the local changes required, that I described in https://github.com/bradshjg/flask-githubapp/issues/14) when sending the payload generated from a push event. These payloads have the headers -H "X-GitHub-Event: push" -H "Content-Type: application/json" so the event is a push, however push payloads have no action element.

I was not aware of dict now supporting a default when running .get. Generally speaking I tend to explicitly add that so as to avoid these issues (much more comfortable than having to check if the dict contains the key every time!)

[bradshjg]

Hey,

I'm still a bit curious what the specific error was, but I also understand that it might be painful to try and reproduce (because you're already forked) so let's see if we can move forward together instead 😄.

I came around to your side regarding allows disabling verification 😄 , and now GITHUBAPP_SECRET can be set to False to enable that behavior. I've yet to release this version because I wanted to check with you on it regarding the interface and docs for enabling that behavior. (I'll bring that up in the other Issue though).

I also added a test for push payloads to help prevent any regressions regarding hooks w/o actions (previously we were only testing issue events which do have actions).

[Napo2k]

Ah! Sorry, the error was something along the lines of request.json has no member of name 'action' on line action = request.json.get('action') Let me clone this again and try

[Napo2k]

I have errors!

This is my app

@github_app.on('push')
def push():
    print("I have received a push")

@github_app.on('issue_comment.created')
def issue_comment_created():
    print("I have received an issue_comment.created")

@github_app.on('delete')
def delete():
    print("I have received a delete")

@github_app.on('pull_request.opened')
def pull_request_opened():
    print("I have received a pull_request.opened")

X-GitHub-Enterprise-Version: 2.22.5

A push payload yields the following

[2021-01-18 11:36:12,725] ERROR in app: Exception on / [POST]
Traceback (most recent call last):
File "/usr/local/lib/python3.8/dist-packages/flask/app.py", line 2447, in wsgi_app
response = self.full_dispatch_request()
File "/usr/local/lib/python3.8/dist-packages/flask/app.py", line 1952, in full_dispatch_request
rv = self.handle_user_exception(e)
File "/usr/local/lib/python3.8/dist-packages/flask/app.py", line 1821, in handle_user_exception
reraise(exc_type, exc_value, tb)
File "/usr/local/lib/python3.8/dist-packages/flask/_compat.py", line 39, in reraise
raise value
File "/usr/local/lib/python3.8/dist-packages/flask/app.py", line 1950, in full_dispatch_request
rv = self.dispatch_request()
File "/usr/local/lib/python3.8/dist-packages/flask/app.py", line 1936, in dispatch_request
return self.view_functions[rule.endpoint](**req.view_args)
File "/usr/local/lib/python3.8/dist-packages/flask_githubapp/core.py", line 171, in _flask_view_func
action = request.json.get('action')
AttributeError: 'NoneType' object has no attribute 'get'

which is... puzzling :D The webhook config in Github shows json in the payload! It should be using version 0.3.0

[bradshjg]

Awesome, thanks so much for digging into this. My initial guess (I'll dig into a bit more) is that request.json returns None when the request payload does not pass the is_json test.

I definitely think that it would be much more kind to let the user know what's happening via a specific error message in this case. I'll update the code to test for the presence of JSON before accessing it and log/throw if the payload is not json (either wrong content type or unparseable). I like getting to write more tests 😄

I really appreciate you taking the time to flesh out some of the issues here.

[Napo2k]

It's my interest, I want to use this! :P I don't really know how that request could not have json... not sure what went wrong there. For what is worth, my Github webhook has SSL verification enabled... ah... and Content type application/x-www-form-unencoded. Which maybe explains it :D Let me try!

EDIT Changing that changed all those error 500 into 400...

EDIT(2): I am trying with an issue_comment.created payload, by the way

[bradshjg]

Ah, yeah that is a tricky one and I'm not very happy with the error handling in the project. In most cases we don't provide very helpful info and folks are forced to dig into the implementation to figure out what's going wrong.

I am verify interested in simplifying the registration process which might help folks avoid those kinds of issues in the future.

There's also three things I really love about the probot project that I think could help folks:

1. straightforward GitHub App registration using the app-manifest flow.
2. built-in webhook tunneling via the smee service.
3. issue and pull_request convenience methods that keeps us from having to dig through payloads to corral common data.

I'm mostly happy with the interfaces. Some of the names I'm not a huge fan of anymore but I think maintaining backwards compatibility is important and I'm not even sure there are names I want bad enough to create aliases.

I would love to refactor the design to make it more modular to make it extensible and easier to contribute to though. Right now I'm kinda forced to hold the whole implementation in my head whenever I work on it (the tests help a bit, but they're a little fragile in some cases). I imagine it's even harder for other folks.

I guess this is a way of saying that if any of the above sounds useful to you I'd be very interested in helping out in any way I can whether that's on the implementation side or review and refactoring of tests to better support contributions 😄

[Napo2k]

Apologies for the radio silence :D

Well, last time I checked I was still getting the 400. I'll redirect to my test, debug-running pod to see if it works fine. But yeah, I'd love for this to improve. I really don't like nodejs :D

[Napo2k]

This is still a problem.

In any case, feel free to close this ticket after merging the open PR. I will try to figure out what the hell is wrong with my application right now.

[bradshjg]

Please feel free to post any tracebacks here and I might be able to help, I've probably hit most of the errors...just didn't think to handle them more elegantly at the time 😅