You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jclouds.apache.org by Everett Toews <no...@github.com> on 2013/12/10 03:59:25 UTC
[jclouds] Allow the HeaderParam annotation to be used in a Caller.
(#226)
This allows jclouds to factor out common headers into the Caller so they don't
have to be repeated in the Callee.
You can merge this Pull Request by running:
git pull https://github.com/rackspace/jclouds caller-header-param
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds/pull/226
-- Commit Summary --
* Allow the HeaderParam annotation to be used in a Caller.
-- File Changes --
M core/src/main/java/org/jclouds/rest/internal/RestAnnotationProcessor.java (13)
M core/src/test/java/org/jclouds/rest/internal/RestAnnotationProcessorTest.java (34)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/226.patch
https://github.com/jclouds/jclouds/pull/226.diff
Re: [jclouds] Allow the HeaderParam annotation to be used in a
Caller. (#226)
Posted by Andrew Phillips <no...@github.com>.
> Multimap<String, Object> queryParams = addQueryParams(tokenValues, invocation);
> - Multimap<String, String> headers = buildHeaders(tokenValues, invocation);
> +
> + Multimap<String, String> headers;
> +
> + if (caller != null) {
> + headers = buildHeaders(tokenValues, caller);
> + headers.putAll(buildHeaders(tokenValues, invocation));
> + } else {
> + headers = buildHeaders(tokenValues, invocation);
To avoid the duplication of the `buildHeaders` call, something like"
```
Multimap<String, String> headers;
if (caller == null) {
headers = LinkedHashMultimap.create();
} else {
headers = buildHeaders(tokenValues, caller);
}
// invocation values override caller values
headers.putAll(buildHeaders(tokenValues, invocation));
```
?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/226/files#r8223202
Re: [jclouds] Allow the HeaderParam annotation to be used in a
Caller. (#226)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #923](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/923/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/226#issuecomment-30259537
Re: [jclouds] Allow the HeaderParam annotation to be used in a
Caller. (#226)
Posted by Andrew Phillips <no...@github.com>.
> Agree, but as long as we are including them in the "header propagation logic", we should add the tests to make
> sure that (even if rarely used) the code actually works as expected for them too.
Indeed. Do we also have tests on the _callee_ side to verify expected header params which would indicate if we break stuff by adding the propagation?
I'm also fine to merge without those and then fix as problems arise, but in that case I'd probably prefer to wait for 1.7.1. @everett-toews: is this required for a change that we're trying to get in for 1.7.0?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/226#issuecomment-30247514
Re: [jclouds] Allow the HeaderParam annotation to be used in a
Caller. (#226)
Posted by Andrew Phillips <no...@github.com>.
Committed to [master](https://git-wip-us.apache.org/repos/asf?p=jclouds.git;a=commit;h=c40dc996d96b05ce755f28728a4fb2bcd632b247). Thanks, @everett-toews and @nacx!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/226#issuecomment-30316146
Re: [jclouds] Allow the HeaderParam annotation to be used in a
Caller. (#226)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #461](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/461/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/226#issuecomment-30259348
Re: [jclouds] Allow the HeaderParam annotation to be used in a
Caller. (#226)
Posted by Ignasi Barrera <no...@github.com>.
@everett-toews Tests look great. Thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/226#issuecomment-30266501
Re: [jclouds] Allow the HeaderParam annotation to be used in a
Caller. (#226)
Posted by Ignasi Barrera <no...@github.com>.
Personally (no offense) I wouldn't like to merge this without a complete test set. We should take special care when changing stuff in jclouds/core, as all providers and apis are going to be affected. I think having the appropriate tests should be a must to merge this.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/226#issuecomment-30248144
Re: [jclouds] Allow the HeaderParam annotation to be used in a
Caller. (#226)
Posted by Everett Toews <no...@github.com>.
@demobox I'm adding the tests @nacx suggested. This is necessary for 1.7.0.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/226#issuecomment-30250404
Re: [jclouds] Allow the HeaderParam annotation to be used in a
Caller. (#226)
Posted by Andrew Phillips <no...@github.com>.
> Multimap<String, Object> queryParams = addQueryParams(tokenValues, invocation);
> - Multimap<String, String> headers = buildHeaders(tokenValues, invocation);
> +
> + Multimap<String, String> headers;
> +
> + if (caller != null) {
> + headers = buildHeaders(tokenValues, caller);
> I did a find all usages of the HeaderParam annotation and there aren't any in Callers.
Clear, thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/226/files#r8235818
Re: [jclouds] Allow the HeaderParam annotation to be used in a
Caller. (#226)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #451](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/451/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/226#issuecomment-30197372
Re: [jclouds] Allow the HeaderParam annotation to be used in a
Caller. (#226)
Posted by Ignasi Barrera <no...@github.com>.
If I am not wrong, the Caller headers will be always overridden by the Callee ones (line 254). Anyway, I think we could add a test for that just to make sure that is not broken. Something like:
Test 1 (Calle headers win)
* A Caller setting the Consumes and Produces.
* A Callee setting different ones in the delegate method
Test2 (Callee headers win)
* A Caller setting the Consumes and Produces.
* A Callee setting different ones at class level but not at method level
Test3 (Caller headers win)
* A Caller setting the Consumes and Produces.
* A Callee without Consumes/Produces
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/226#issuecomment-30245597
Re: [jclouds] Allow the HeaderParam annotation to be used in a
Caller. (#226)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #689](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/689/) FAILURE
Looks like there's a problem with this pull request
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/226#issuecomment-30255943
Re: [jclouds] Allow the HeaderParam annotation to be used in a
Caller. (#226)
Posted by Andrew Phillips <no...@github.com>.
> Just to be sure this is not unnoticed, this change will also propagate the @Produces and @Consumes annotations
I guess the only place we might need to worry about this is where a call currently assumes default produces/consumes (i.e. doesn't set anything explicitly), but might now be overridden by the values coming from the caller?
Is that possible, or am I getting hold of the wrong (end of) stick here? ;-)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/226#issuecomment-30239864
Re: [jclouds] Allow the HeaderParam annotation to be used in a
Caller. (#226)
Posted by Everett Toews <no...@github.com>.
Sharp eyes @nacx. It was not intentional. I completely glazed over the lines.
addProducesIfPresentOnTypeOrMethod(headers, invocation);
addConsumesIfPresentOnTypeOrMethod(headers, invocation);
in `buildHeaders`. I agree that it's okay too. I'll update the commit messsage and JIRA too.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/226#issuecomment-30237787
Re: [jclouds] Allow the HeaderParam annotation to be used in a
Caller. (#226)
Posted by Everett Toews <no...@github.com>.
Produces/Consumes annotation are vary rarely used in the Caller. When they are it's for "direct call" methods like DynECTApi.getJob() or CloudDNSApi.getJob().
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/226#issuecomment-30245918
Re: [jclouds] Allow the HeaderParam annotation to be used in a
Caller. (#226)
Posted by Andrew Phillips <no...@github.com>.
> Multimap<String, Object> queryParams = addQueryParams(tokenValues, invocation);
> - Multimap<String, String> headers = buildHeaders(tokenValues, invocation);
> +
> + Multimap<String, String> headers;
> +
> + if (caller != null) {
> + headers = buildHeaders(tokenValues, caller);
> + headers.putAll(buildHeaders(tokenValues, invocation));
> + } else {
> + headers = buildHeaders(tokenValues, invocation);
> I found 2 other places within the apply method that had the style I eventually used
Makes sense, let's stick with that, then. Thanks for explaining!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/226/files#r8235803
Re: [jclouds] Allow the HeaderParam annotation to be used in a
Caller. (#226)
Posted by Everett Toews <no...@github.com>.
> Multimap<String, Object> queryParams = addQueryParams(tokenValues, invocation);
> - Multimap<String, String> headers = buildHeaders(tokenValues, invocation);
> +
> + Multimap<String, String> headers;
> +
> + if (caller != null) {
> + headers = buildHeaders(tokenValues, caller);
There aren't any examples of this in the current code. I did a find all usages of the HeaderParam annotation and there aren't any in Callers.
I needed this for my work in openstack-marconi in jclouds-labs-openstack and provided an example in the JIRA issue.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/226/files#r8235275
Re: [jclouds] Allow the HeaderParam annotation to be used in a
Caller. (#226)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #913](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/913/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/226#issuecomment-30197361
Re: [jclouds] Allow the HeaderParam annotation to be used in a
Caller. (#226)
Posted by Ignasi Barrera <no...@github.com>.
Just to be sure this is not unnoticed, this change will also propagate the `@Produces` and `@Consumes` annotations (which IMO it is OK). Is it intentional? Just wanted to note it as the description of the issue only mentions the `@HeaderParam` annotation.
If this is the case, and this PR aims to "propagate all headers" to the Callee, should the headers in the [HttpRequestOptions method parameters of the Caller](https://github.com/rackspace/jclouds/blob/c49cbe638fe6aaaa1ba6e7d5aa8206d3fab84f7f/core/src/main/java/org/jclouds/rest/internal/RestAnnotationProcessor.java#L263-L265) be propagated too?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/226#issuecomment-30207853
Re: [jclouds] Allow the HeaderParam annotation to be used in a
Caller. (#226)
Posted by Ignasi Barrera <no...@github.com>.
>Produces/Consumes annotation are vary rarely used in the Caller. When they are it's for "direct call" methods like DynECTApi.getJob() or CloudDNSApi.getJob().
Agree, but as long as we are including them in the "header propagation logic", we should add the tests to make sure that (even if rarely used) the code actually works as expected for them too.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/226#issuecomment-30246208
Re: [jclouds] Allow the HeaderParam annotation to be used in a
Caller. (#226)
Posted by Everett Toews <no...@github.com>.
Thanks for the commit and reviews @demobox and @nacx.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/226#issuecomment-30331125
Re: [jclouds] Allow the HeaderParam annotation to be used in a
Caller. (#226)
Posted by Andrew Phillips <no...@github.com>.
> Multimap<String, Object> queryParams = addQueryParams(tokenValues, invocation);
> - Multimap<String, String> headers = buildHeaders(tokenValues, invocation);
> +
> + Multimap<String, String> headers;
> +
> + if (caller != null) {
> + headers = buildHeaders(tokenValues, caller);
Just to ensure we're backwards-compatible here: do we know of any examples in the current code where this would actually do something already? I.e. is there anywhere in the current code where this change could end up altering functionality?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/226/files#r8223127
Re: [jclouds] Allow the HeaderParam annotation to be used in a
Caller. (#226)
Posted by Andrew Phillips <no...@github.com>.
> Personally (no offense) I wouldn't like to merge this without a complete test set.
No offence taken ;-) I certainly agree we should have tests to ensure the change works as intended.
I was just wondering whether we have tests that would be able to verify that the new behaviour doesn't _break_ anything (assuming it is possible to break something by adding this possible override)...and if we don't, how we address that.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/226#issuecomment-30249211
Re: [jclouds] Allow the HeaderParam annotation to be used in a
Caller. (#226)
Posted by Everett Toews <no...@github.com>.
BuildHive sez
ERROR: Processing failed due to a bug in the code. Please report this to jenkinsci-users@googlegroups.com
java.lang.UnsupportedOperationException
at java.util.AbstractMap.put(AbstractMap.java:186)
at hudson.maven.MavenModuleSetBuild$RunnerImpl.parsePoms(MavenModuleSetBuild.java:958)
at hudson.maven.MavenModuleSetBuild$RunnerImpl.doRun(MavenModuleSetBuild.java:656)
BuildHive just seems to be completely borked at this point.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/226#issuecomment-30258324
Re: [jclouds] Allow the HeaderParam annotation to be used in a
Caller. (#226)
Posted by Everett Toews <no...@github.com>.
> Multimap<String, Object> queryParams = addQueryParams(tokenValues, invocation);
> - Multimap<String, String> headers = buildHeaders(tokenValues, invocation);
> +
> + Multimap<String, String> headers;
> +
> + if (caller != null) {
> + headers = buildHeaders(tokenValues, caller);
> + headers.putAll(buildHeaders(tokenValues, invocation));
> + } else {
> + headers = buildHeaders(tokenValues, invocation);
I originally had something like that but then I found 2 other places within the apply method that had the style I eventually used. I opted to go for consistency rather than leaving the reader to guess why 3 things that were functionally equivalent were written differently. We could always refactor all 3 as a next step.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/226/files#r8235183
Re: [jclouds] Allow the HeaderParam annotation to be used in a
Caller. (#226)
Posted by Andrew Phillips <no...@github.com>.
Looks good to me, too. @nacx: good to go on this one?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/226#issuecomment-30303663
Re: [jclouds] Allow the HeaderParam annotation to be used in a
Caller. (#226)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #680](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/680/) UNSTABLE
Looks like there's a problem with this pull request
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/226#issuecomment-30198328