Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add github resource #15

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

dkubb
Copy link
Owner

@dkubb dkubb commented Jul 4, 2015

This branch adds a github resource. It will receive all push notifications from github, log them to a database and (in a future PR) trigger a build.

TODO

  • Add endpoint to receive Github webhook push notifications
    • Test the X-Hub-Signature (signature) header is valid
      • Return 403 if not valid
    • Test the message body size
      • Return 413 if message body is too large
    • Test the X-Github-Event (the event type) header is supported
      • Return 400 if the event type is not supported
    • Test if the request body is valid JSON
      • Return 400 if the body is syntactically invalid JSON
    • Parse the request body with aeson
      • Store the raw request headers and body in a database (for logging/replay)
      • Return 422 if the body is semantically invalid JSON
    • Create a record to represent the commit state
      • Store the X-Github-Delivery (request id) as a UUID

@dkubb dkubb self-assigned this Jul 4, 2015
@dkubb dkubb force-pushed the master branch 4 times, most recently from fc8e89c to 54011b7 Compare July 4, 2015 07:10
@dkubb dkubb force-pushed the feature/master/github-resource branch 3 times, most recently from effbd14 to 5bfe41d Compare July 4, 2015 07:24
@dkubb
Copy link
Owner Author

dkubb commented Jul 4, 2015

@mbj would you mind doing an initial TODO and (light) code review on this?

@dkubb dkubb force-pushed the feature/master/github-resource branch 10 times, most recently from db22a70 to acc2cf4 Compare July 4, 2015 09:50
@mbj
Copy link
Collaborator

mbj commented Jul 4, 2015

Test if the request body is valid JSON
Parse the request body with aeson

I think that these both steps are the same. Afaik there is no way to validate correct JSON without parsing it.

@mbj
Copy link
Collaborator

mbj commented Jul 4, 2015

@dkubb I'd suggest to add a dedicated step validating the request content type and returning 415 Unsupported Media Type on mismatch. This might be part of the web frameworks behavior. But I did not look at the code at the time of this writing.

@mbj
Copy link
Collaborator

mbj commented Jul 4, 2015

@dkubb Code wise I do not have flags right now.

What I wounder is that so many types require to be composed by the IO monad. But thats a library choice by Airship.

@dkubb
Copy link
Owner Author

dkubb commented Jul 4, 2015

What I wounder is that so many types require to be composed by the IO monad. But thats a library choice by Airship.

The s variable is for state. I think it's for application-wide state, but it could be per-request state. For now I am just using Void for it because I don't have anything to pass around.

@dkubb
Copy link
Owner Author

dkubb commented Jul 4, 2015

I'd suggest to add a dedicated step validating the request content type and returning 415 Unsupported Media Type on mismatch.

This is handled by the framework. I register the allowed input media types, as well as provide a map of the input media types to handlers. There's a similar mechanism to specify output media types so the ideal representation can be negotiated for the client.

@dkubb
Copy link
Owner Author

dkubb commented Jul 4, 2015

Test if the request body is valid JSON
Parse the request body with aeson

I think that these both steps are the same. Afaik there is no way to validate correct JSON without parsing it.

The idea I had was to see if the request payload was syntactically valid. So if it was valid JSON at all. From there I was going to see if the JSON parsed with aeson into some predefined types; and that the JSON matches the expected schema.

It may be that aeson forces me to do both at once, which I guess would be alright provided it fails fast in the presence of syntactic errors.

@mbj
Copy link
Collaborator

mbj commented Jul 4, 2015

It may be that aeson forces me to do both at once, which I guess would be alright provided it fails fast in the presence of syntactic errors.

AFAIK it allows exactly that.

@dkubb
Copy link
Owner Author

dkubb commented Jul 5, 2015

@mbj I remember now why I wanted to break syntactic and semantic validation; depending on which fails you want to return a 400 Bad Request or 422 Unprocessable Entity. I wonder if aeson has a way of reporting more detail about the failure, whether it was completely invalid JSON or if there was a type mismatch. From what I understood it just returned Maybe a, and Nothing represented parse failure.

@mbj
Copy link
Collaborator

mbj commented Jul 5, 2015

@mbj I remember now why I wanted to break syntactic and semantic validation; depending on which fails you want to return a 400 Bad Request or 422 Unprocessable Entity. I wonder if aeson has a way of reporting more detail about the failure, whether it was completely invalid JSON or if there was a type mismatch. From what I understood it just returned Maybe a, and Nothing represented parse failure.

AFAIK aeson has an API that allows you to distinguish between a JSON syntax error and a JSON structure mismatch.

@mbj
Copy link
Collaborator

mbj commented Jul 5, 2015

AFAIK aeson has an API that allows you to distinguish between a JSON syntax error and a JSON structure mismatch.

I'll try to look it up.

@dkubb dkubb force-pushed the feature/master/github-resource branch 2 times, most recently from a8469a5 to 670c5e9 Compare July 7, 2015 07:21
@dkubb dkubb force-pushed the feature/master/github-resource branch 6 times, most recently from 4d3206b to 1b3e27d Compare July 18, 2015 23:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants