Skip to content

Conversation

@dmorrow
Copy link

@dmorrow dmorrow commented Dec 21, 2017

Description of the pull request

By overriding bodyFactory, it is now possible to send JSON objects in the body, rather than only "Content-Type: application/x-www-form-urlencoded" strings.
Users can also set the correct headers to be sent - for example "Content-Type: application/json", and are able to override the "charset" header as well.

Why is the change necessary?

#164

CC @pusher/mobile

By overriding bodyFactory, it is now possible to send JSON objects in the body, rather than only "Content-Type: application/x-www-form-urlencoded" strings.
Users can also set the correct headers to be sent - for example "Content-Type: application/json", and are able to override the "charset" header as well.
@zmarkan
Copy link
Contributor

zmarkan commented Jan 2, 2018

Thank you, this looks interesting. I'll test this out!

@zmarkan
Copy link
Contributor

zmarkan commented Jan 2, 2018

I tried getting this to work, but couldn't.
Some thoughts:

I don't think the URL params get transformed into an actual JSON body for starters. Shouldn't they need different names?
The automated build also fails, due to an exception that's declared but not handled.
Lastly, we'd need to update the README with new usage samples on how to change the body to use a different payload.

WDYT?

@dmorrow
Copy link
Author

dmorrow commented Jan 2, 2018

@zmarkan sorry - I banged this code out without checking for errors. This code was more of a sketch than functionally complete.

Conceptually, it should function exactly as your current library (and not handle a JSON body). It would be up to the developer to handle subclassing HttpAuthorizer.bodyFactory to send a JSON body as required by their backend.

An alternative approach would be to have a switch based on a "Content-Type" header and include a jsonBodyFactory method like this (I've made the channelNameKey and socketIdKey constants rather than Strings here):

private String jsonBodyFactory(final String channelName, final String socketId) {
    JSONObject json = new JSONObject();
    try {
        json.put(channelNameKey, channelName);
        json.put(socketIdKey, socketId);
    } catch (JSONException e) {
        e.printStackTrace();
    }
    return json.toString();
}

All of this was more of a suggestion - if you agree with the concept, and like one approach over the other, I would be happy to update the PR.

@zmarkan
Copy link
Contributor

zmarkan commented Jan 2, 2018

No problem. I see it can be valuable, and I'll be happy to merge it if you got it working.

Few things however - instead of subclassing I think it's better to have a BodyFactory instance as an constructor parameter that you pass to the HTTPAuthorizer, along with the content type.
If you omit passing it as a parameter it could function the same as currently - with Form URL encoding, and if not, it could be overridden to support whatever body you want.

In regards to the Content-Type switch , why not have BodyFactory have a getContentType() method that gets called to inject the headers?

WDYT?

@dmorrow
Copy link
Author

dmorrow commented Jan 22, 2018

@zmarkan #170 is the new PR for this functionality. I'm going to close this one.

@dmorrow dmorrow closed this Jan 22, 2018
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.

2 participants