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

allow for bracketed, nested query params as Mailchimp does #65

Closed
wants to merge 1 commit into from

Conversation

@jessfraz
Copy link
Contributor

@jessfraz jessfraz commented Dec 4, 2020

This includes the fix for #64 as well.

Basically Mailchimp has a query they pass through their webhook that is super nested. To allow for this to work we need to use a different library that handles the nested queries better.

You can see Mailchimps format here: https://mailchimp.com/developer/guides/sync-audience-data-with-webhooks/

@jessfraz jessfraz force-pushed the jessfraz:fix-query branch 2 times, most recently from abc85db to 16213f6 Dec 4, 2020
@davepacheco
Copy link
Collaborator

@davepacheco davepacheco commented Dec 7, 2020

@jess is this one ready for review? It seems like a bunch of code was restyled here, which I'm guessing was unintentional, but there are also still some CI failures so maybe this is still in progress?

@jessfraz
Copy link
Contributor Author

@jessfraz jessfraz commented Dec 7, 2020

@jessfraz jessfraz marked this pull request as draft Dec 14, 2020
Base automatically changed from master to main Mar 18, 2021
@jessfraz jessfraz force-pushed the jessfraz:fix-query branch from 955158d to f1417da May 6, 2021
@jessfraz jessfraz marked this pull request as ready for review May 6, 2021
@jessfraz
Copy link
Contributor Author

@jessfraz jessfraz commented May 6, 2021

Finially pulling this out of drafts and rebased.

I updated all my things to use the rebased version.

Copy link
Collaborator

@davepacheco davepacheco left a comment

I wasn't sure if you meant this was ready for review or not. There are still a bunch of CI issues here. Anyway, I took a look and had some questions.

let mut request = rqctx.request.lock().await;
let server = &rqctx.server;

let b = http_read_body(request.body_mut(), server.config.request_body_max_bytes)

This comment has been minimized.

@davepacheco

davepacheco May 6, 2021
Collaborator

It looks like we're reading the entire body here. It also looks like we're no longer looking at the actual querystring that came in with the HTTP request line. Am I misreading that? It seems like this would break anything that used either a querystring or a body.

Are you looking to read the querystring or the body here? If it's the body, I think maybe we want to add a different kind of extractor analogous to TypedBody and UntypedBody for QuerystringBody. Or maybe even better would be to have TypedBody look at the MIME type and decide whether to parse it as JSON or a querystring. What do you think?

This comment has been minimized.

@jessfraz

jessfraz May 7, 2021
Author Contributor

Mailchimp is literally the weirdest so technically this is working (and has been working) for the past few (~10) months. It's definitely in the query string, but I recall when I wrote this awhile back reading the string did not work so I did it this way instead.

This was how I was testing it. Thats how the query actually comes back.

 #[test]
    fn test_mailchimp_parsing() {
        let body = r#"type=subscribe&fired_at=2020-09-07+21%3A31%3A09&data%5Bid%5D=b748506b63&data%5Bemail%[email protected]&data%5Bemail_type%5D=html&data%5Bip_opt%5D=98.128.229.135&data%5Bweb_id%5D=404947702&data%5Bmerges%5D%5BEMAIL%[email protected]&data%5Bmerges%5D%5BFNAME%5D=&data%5Bmerges%5D%5BLNAME%5D=&data%5Bmerges%5D%5BADDRESS%5D=&data%5Bmerges%5D%5BPHONE%5D=&data%5Bmerges%5D%5BBIRTHDAY%5D=&data%5Bmerges%5D%5BCOMPANY%5D=&data%5Bmerges%5D%5BINTEREST%5D=8&data%5Bmerges%5D%5BINTERESTS%5D=Yes&data%5Bmerges%5D%5BGROUPINGS%5D%5B0%5D%5Bid%5D=6197&data%5Bmerges%5D%5BGROUPINGS%5D%5B0%5D%5Bunique_id%5D=458a556058&data%5Bmerges%5D%5BGROUPINGS%5D%5B0%5D%5Bname%5D=Interested+in+On+the+Metal+podcast+updates%3F&data%5Bmerges%5D%5BGROUPINGS%5D%5B0%5D%5Bgroups%5D=Yes&data%5Bmerges%5D%5BGROUPINGS%5D%5B1%5D%5Bid%5D=6245&data%5Bmerges%5D%5BGROUPINGS%5D%5B1%5D%5Bunique_id%5D=f64af23d78&data%5Bmerges%5D%5BGROUPINGS%5D%5B1%5D%5Bname%5D=Interested+in+the+Oxide+newsletter%3F&data%5Bmerges%5D%5BGROUPINGS%5D%5B1%5D%5Bgroups%5D=Yes&data%5Bmerges%5D%5BGROUPINGS%5D%5B2%5D%5Bid%5D=7518&data%5Bmerges%5D%5BGROUPINGS%5D%5B2%5D%5Bunique_id%5D=a9829c90a6&data%5Bmerges%5D%5BGROUPINGS%5D%5B2%5D%5Bname%5D=Interested+in+product+updates%3F&data%5Bmerges%5D%5BGROUPINGS%5D%5B2%5D%5Bgroups%5D=Yes&data%5Blist_id%5D=8a6d823488"#;

        //let body = "type=subscribe&fired_at=2020-09-07+21%3A31%3A09";
        let qs_non_strict = QSConfig::new(10, false);

        // Parse the request body as a MailchimpWebhook.
        let webhook: MailchimpWebhook =
            qs_non_strict.deserialize_bytes(body.as_bytes()).unwrap();

        println!("{:?}", webhook);
    }

This comment has been minimized.

@davepacheco

davepacheco May 7, 2021
Collaborator

Yeah, it makes sense that this works with your Mailchimp hook. It really looks to me like the data is in a similar format to a querystring, but the data's being sent in the body of the request, not the querystring. Your implementation here reads the request body and ignores the querystring. And the doc you linked above says:

The body of the webhook request is sent as application/x-www-form-urlencoded data.

That page doesn't say anything about a querystring. Also, the Node.js example there is parsing the body.

So, I was wrong before when I said this would break any Dropshot user that's using a body or a querystring, but I do believe it would break any Dropshot user that's using a request body and a querystring. That's because the new code will read the whole body before the user has a chance to (so they'll see a corrupt body) and also look at the wrong data for the querystring (looking in the body instead of the querystring). I believe this is what's causing the CI test failures.

So here are a couple of ideas:

  • We could modify the TypedBody extractor to look at the content-type header. If it's application/x-www-form-urlencoded, then we apply a parser for that MIME type (like you've done here) instead of serde_json.
  • We could create a new impl of Extractor called FormEncodedBody that just always uses this MIME type. I think this is less HTTP/REST-idiomatic because it means that you'd need a different path to accept a different input format.
  • I think you could impl your own FormEncodedBody completely outside of Dropshot. This looks just like the previous option but I don't think you need any Dropshot support for this. (I'd still be happy to have it in Dropshot though!)
@jessfraz
Copy link
Contributor Author

@jessfraz jessfraz commented May 7, 2021

good point, ill just do like TypedBody<String> and then parse it myself :)

@jessfraz jessfraz closed this May 7, 2021
@ahl
Copy link
Contributor

@ahl ahl commented May 7, 2021

We could modify the TypedBody extractor to look at the content-type header. If it's application/x-www-form-urlencoded, then we apply a parser for that MIME type (like you've done here) instead of serde_json.

@davepacheco I like this idea .... even if Jess has taken her ball and gone home ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants