Skip to content
This repository has been archived by the owner on Apr 5, 2022. It is now read-only.

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

Merged
merged 6 commits into from
Jan 27, 2012
Merged

Conversation

sweemer
Copy link
Contributor

@sweemer 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?

@royclarkson
Copy link
Contributor

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

@sweemer
Copy link
Contributor Author

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?

@royclarkson
Copy link
Contributor

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.

@royclarkson
Copy link
Contributor

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.

@sweemer
Copy link
Contributor Author

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.

@royclarkson
Copy link
Contributor

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.

@royclarkson
Copy link
Contributor

@sweemer
Copy link
Contributor Author

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.

@royclarkson
Copy link
Contributor

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-attic:master Jan 27, 2012
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants