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

Fix unmarshalling Webhook and Sample Events, Add SMSStatus Event type #35

Merged
merged 17 commits into from
Mar 15, 2016
Merged

Conversation

VojtechVitek
Copy link
Contributor

A custom Events JSON unmarshaller successfully parses both Webhook Events and Sample Events without any regexp magic.

Listening on Webhook Events is as simple as:

package myhandler

import (
    "net/http"

    spevents "github.com/SparkPost/gosparkpost/events"
)

func ReceiveEvents(w http.ResponseWriter, r *http.Request) {
    body, err := ioutil.ReadAll(r.Body)
    if err != nil {
        http.Error(w, "invalid data", 422)
        return
    }

    var events spevents.Events
    if err := json.Unmarshal(body, &events); err != nil {
        if err == spevents.ErrWebhookValidation {
            w.WriteHeader(200)
            return
        }
        http.Error(w, "invalid data", 422)
        return
    }

    for _, event := range events {
        switch e := event.(type) {
        case *spevents.Bounce:
            // Do something with e.
        case *spevents.Delivery:
            // Do something with e.
        case *spevents.Injection:
            // Do something with e.
        case *spevents.SpamComplaint:
            // Do something with e.
        case *spevents.Click:
            // Do something with e.
        case *spevents.Open:
            // Do something with e.
        case *spevents.ListUnsubscribe:
            // Do something with e.
        case *spevents.LinkUnsubscribe:
            // Do something with e.
        default:
            log.Errorf("Can't parse SparkPost event: %v", e)
        }
    }

    w.WriteHeader(200)
}

@VojtechVitek VojtechVitek changed the title Fix unmarshaling Webhook and Sample Events, Add SMSStatus Event type Fix unmarshalling Webhook and Sample Events, Add SMSStatus Event type Mar 14, 2016
@VojtechVitek
Copy link
Contributor Author

@yepher, @yargevad any feedback would be appreciated. This PR should be API backward-compatible. Are you guys willing to change the API, or is this project considered stable?

The API inconsistently returns float or string as Latitude and
Longitude values. That's why we need a custom unmarshaller to
parse the extra quotes as valid float32.
@yepher
Copy link
Contributor

yepher commented Mar 14, 2016

I think the interface needs to change. For instance message events response is more like this:

type EventsWrapper struct {
    Results    []*EventItem        `json:"results,omitempty"`
    TotalCount int                 `json:"total_count,omitempty"`
    Links      []map[string]string `json:"links,omitempty"`
    Errors     []interface{}       `json:"errors,omitempty"`
    //{"errors":[{"param":"from","message":"From must be before to","value":"2014-07-20T09:00"},{"param":"to","message":"To must be in the format YYYY-MM-DDTHH:mm","value":"now"}]}
}

I also think that message_events.go should use a URL like this
var messageEventsPathFormat = "/api/v%d/message-events"

And we expose search something like this:

// https://developers.sparkpost.com/api/#/reference/message-events/events-samples/search-for-message-events
func (c *Client) SearchMessageEvents(parameters map[string]string) (*EventsWrapper, error) {

    var finalUrl string
    path := fmt.Sprintf(messageEventsPathFormat, c.Config.ApiVersion)
    if parameters == nil || len(parameters) == 0 {
        finalUrl = fmt.Sprintf("%s%s", c.Config.BaseUrl, path)
    } else {
        params := URL.Values{}
        for k, v := range parameters {
            params.Add(k, v)
        }

        finalUrl = fmt.Sprintf("%s%s?%s", c.Config.BaseUrl, path, params.Encode())
    }

    return DoRequest(c, finalUrl)
}

@VojtechVitek
Copy link
Contributor Author

@yepher oh I see, there's another API endpoint for "Event Messages", which has - again - a different data format.. In that case, how about having new type MessageEvents, since this is the only endpoint, which has cursors? We can define methods like func (e *MessageEvents) Next() (MessageEvents, error) to fetch next batch of messages etc.

@yepher
Copy link
Contributor

yepher commented Mar 15, 2016

@VojtechVitek Until I saw the great work you had done I was punting and moving in this direction:

https://github.com/yepher/gosparkpost/blob/302534314e47e3dc5cc50147c2e222cc5cdb11aa/message_events.go

I personally am a little torn between discrete messages (with a lot of overlap) or just one large struct that contains all possible field.

@VojtechVitek
Copy link
Contributor Author

@yepher I know what you mean.. But I think I still prefer discrete struct for each event type, since it's very easy to type-switch-case to filter out events that I really care about. And also, it's easier to understand the underlying data -- for example, I'd expect that OutOfBand event has TransmissionID - but it doesn't. I wouldn't realize that without having the OutOfBand type and it would bite me later, since I'd rely on the missing value.

There's no big overlap, since we only lookup the event type and postpone parsing the actual data with json.RawMessage. This should perform well, even though it's a two-way pass.

Let me have a look at MessageEvents real quick. I think it can be simple struct wrapper around type Events, similar to what you had. Benefit: The parsing will be still as simple as json.Unmarshal(data, MessageEvents) (to get cursors) or json.Unmarshal(data, Events) to get raw first page of results.

@yepher
Copy link
Contributor

yepher commented Mar 15, 2016

@VojtechVitek All that sound really good and I totally agree.

yargevad pushed a commit that referenced this pull request Mar 15, 2016
Fix unmarshalling Webhook and Sample Events, Add SMSStatus Event type
@yargevad yargevad merged commit 64a323b into SparkPost:master Mar 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants