Added support for Authorization and User-Agent HTTP headers #2

Merged
merged 6 commits into from Jan 27, 2012

Conversation

Projects
None yet
2 participants
Contributor

sweemer commented Jan 5, 2012

I am using spring-android for an Android application. It works great except it was missing two things I needed:

  • A way to customize the User-Agent HTTP header
  • Support for HTTP Basic Authentication

I've added those two features and confirmed that it works on my Android application. Would you consider merging these commits into the main repository?

Member

royclarkson commented Jan 11, 2012

Thanks for the addition! We'll evaluate including it in the upcoming release.

Contributor

sweemer commented Jan 24, 2012

Commit eb3f8b3 fixes a serious issue, where MediaType was throwing an IllegalArgumentException due to "Invalid token character". You can use the following java code to test that my commit fixes the issue:

import org.springframework.http.MediaType;

public class MediaTypeTest {
    public static void main(String[] args) {
        System.out.println(MediaType.ALL);
        System.out.println(MediaType.APPLICATION_ATOM_XML);
        System.out.println(MediaType.APPLICATION_RSS_XML);
        System.out.println(MediaType.APPLICATION_FORM_URLENCODED);
        System.out.println(MediaType.APPLICATION_JSON);
        System.out.println(MediaType.APPLICATION_OCTET_STREAM);
        System.out.println(MediaType.APPLICATION_XHTML_XML);
        System.out.println(MediaType.APPLICATION_XML);
        System.out.println(MediaType.APPLICATION_WILDCARD_XML);
        System.out.println(MediaType.IMAGE_GIF);
        System.out.println(MediaType.IMAGE_JPEG);
        System.out.println(MediaType.IMAGE_PNG);
        System.out.println(MediaType.MULTIPART_FORM_DATA);
        System.out.println(MediaType.TEXT_HTML);
        System.out.println(MediaType.TEXT_PLAIN);
        System.out.println(MediaType.TEXT_XML);
    }
}

Also I don't understand why the public static final String values that I removed were added in the first place. Surely the toString() method should have the final say on the media type value and not a hardcoded constant?

Member

royclarkson commented Jan 24, 2012

Indeed, I found this error last night also when updating sample projects. These constants were added in Spring Framework 3.1.0, so they made their way into Spring Android when I merged the updates from that release. I'll follow up on the reasoning for adding them this way. Thanks again for the contribution.

Member

royclarkson commented Jan 25, 2012

I've corrected the MediaType problems in master. This was a result of a bad manual merge, and unfortunately the unit test projects not updating correctly when I ran them.

Contributor

sweemer commented Jan 25, 2012

Ok, thanks for the quick action Roy. I've just merged your changes to my repo and everything seems to work. My repo is up to date with the SpringSource one in case you're still interested in pulling in the commits with the new HTTP headers.

Member

royclarkson commented Jan 25, 2012

Thanks for updating your merge request and the feedback. My current thinking is that the HttpHeaders additions are fine being merged in. However we created the spring-android-auth project to incapsulate authentication functionality. Granted, right now it only has spring-social compatible oauth stuff in it, but I think the basic Http Authentication could also be added in there. That's the route I'm considering, as I would like to keep with that modularity.

Contributor

sweemer commented Jan 25, 2012

Perhaps it would be optimal to include both implementations of HTTP basic authentication - one through the HttpHeaders class, and one in the spring-android-auth project. In this case it's a tradeoff between simplicity and encapsulation, and for the sake of flexibility it may be best to adopt a both-and approach to the tradeoff rather than an either-or one. However I leave the final decision up to you.

Member

royclarkson commented Jan 25, 2012

After reviewing the code and the HTTP specification some more, I think I've changed my mind on my initial thoughts and I agree with you. Being part of the HTTP spec, it would be consistent to include it in the same package as all the other http elements.

@royclarkson royclarkson merged commit 715cae8 into spring-projects:master Jan 27, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment