You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Andrea Turli <no...@github.com> on 2017/01/12 09:29:50 UTC
[jclouds/jclouds-labs] add AddApiVersionToRequest filter (#343)
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds-labs/pull/343
-- Commit Summary --
* add AddApiVersionToRequest filter
-- File Changes --
M packet/src/main/java/org/jclouds/packet/PacketApiMetadata.java (1)
M packet/src/main/java/org/jclouds/packet/features/ProjectApi.java (3)
A packet/src/main/java/org/jclouds/packet/filters/AddApiVersionToRequest.java (47)
M packet/src/test/java/org/jclouds/packet/compute/internal/BasePacketApiMockTest.java (5)
-- Patch Links --
https://github.com/jclouds/jclouds-labs/pull/343.patch
https://github.com/jclouds/jclouds-labs/pull/343.diff
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/343
Re: [jclouds/jclouds-labs] add AddApiVersionToRequest filter (#343)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
> assertEquals(request.getHeader("X-Auth-Token"), X_AUTHORIZATION_TOKEN);
return request;
}
protected RecordedRequest assertSent(MockWebServer server, String method, String path, String json)
throws InterruptedException {
RecordedRequest request = assertSent(server, method, path);
- assertEquals(request.getHeader("Content-Type"), "application/json");
+ assertEquals(request.getHeader("Accept"), "application/json; version=" + ctx.getMetadata().get("apiVersion"));
addressed and rebased, I think it is ready for being merged.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/343
Re: [jclouds/jclouds-labs] add AddApiVersionToRequest filter (#343)
Posted by Ignasi Barrera <no...@github.com>.
nacx approved this pull request.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/343#pullrequestreview-16881170
Re: [jclouds/jclouds-labs] add AddApiVersionToRequest filter (#343)
Posted by Ignasi Barrera <no...@github.com>.
nacx commented on this pull request.
> assertEquals(request.getHeader("X-Auth-Token"), X_AUTHORIZATION_TOKEN);
return request;
}
protected RecordedRequest assertSent(MockWebServer server, String method, String path, String json)
throws InterruptedException {
RecordedRequest request = assertSent(server, method, path);
- assertEquals(request.getHeader("Content-Type"), "application/json");
+ assertEquals(request.getHeader("Accept"), "application/json; version=" + ctx.getMetadata().get("apiVersion"));
This is already checked in the method called in the line above. This one is intended to verify requests with a body and thus it checks they have the right content-type header.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/343#pullrequestreview-16695953
Re: [jclouds/jclouds-labs] add AddApiVersionToRequest filter (#343)
Posted by Andrea Turli <no...@github.com>.
Closed #343.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/343#event-925344400
Re: [jclouds/jclouds-labs] add AddApiVersionToRequest filter (#343)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
> +import static com.google.common.base.Preconditions.checkNotNull;
+
+@Singleton
+public class AddApiVersionToRequest implements HttpRequestFilter {
+
+ private final String apiVersion;
+
+ @Inject
+ public AddApiVersionToRequest(@ApiVersion String apiVersion) {
+ this.apiVersion = checkNotNull(apiVersion, "apiVersion");
+ }
+
+ @Override
+ public HttpRequest filter(final HttpRequest request) throws HttpException {
+ return request.toBuilder()
+ .replaceHeader(HttpHeaders.ACCEPT, String.format("application/json; version=%s", apiVersion))
I've slightly modified the logic, it now gets the previous `accept` header, which must be there because of @Consume, and append the `version=<value>` to it
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/343
Re: [jclouds/jclouds-labs] add AddApiVersionToRequest filter (#343)
Posted by Andrea Turli <no...@github.com>.
@andreaturli pushed 1 commit.
9930d64 address @nacx comments
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/343/files/595ce8b6bb7319c99c9a3df1e7b3e2e4a70d69b4..9930d64ab6373162bd9c5d0ae01be001887d4d5e
Re: [jclouds/jclouds-labs] add AddApiVersionToRequest filter (#343)
Posted by Andrea Turli <no...@github.com>.
@nacx happy with that version?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/343#issuecomment-272909654
Re: [jclouds/jclouds-labs] add AddApiVersionToRequest filter (#343)
Posted by Andrea Turli <no...@github.com>.
merged at [master](http://git-wip-us.apache.org/repos/asf/jclouds-labs/commit/dfd6804c)
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/343#issuecomment-273050363
Re: [jclouds/jclouds-labs] add AddApiVersionToRequest filter (#343)
Posted by Ignasi Barrera <no...@github.com>.
nacx requested changes on this pull request.
> +import org.jclouds.http.HttpRequest;
+import org.jclouds.http.HttpRequestFilter;
+import org.jclouds.rest.annotations.ApiVersion;
+
+import com.google.common.net.HttpHeaders;
+
+import static com.google.common.base.Preconditions.checkNotNull;
+
+@Singleton
+public class AddApiVersionToRequest implements HttpRequestFilter {
+
+ private final String apiVersion;
+
+ @Inject
+ public AddApiVersionToRequest(@ApiVersion String apiVersion) {
+ this.apiVersion = checkNotNull(apiVersion, "apiVersion");
Remove the public modifier from the constructor and remove the null check.
> +import static com.google.common.base.Preconditions.checkNotNull;
+
+@Singleton
+public class AddApiVersionToRequest implements HttpRequestFilter {
+
+ private final String apiVersion;
+
+ @Inject
+ public AddApiVersionToRequest(@ApiVersion String apiVersion) {
+ this.apiVersion = checkNotNull(apiVersion, "apiVersion");
+ }
+
+ @Override
+ public HttpRequest filter(final HttpRequest request) throws HttpException {
+ return request.toBuilder()
+ .replaceHeader(HttpHeaders.ACCEPT, String.format("application/json; version=%s", apiVersion))
Do we want to override the Accept header, whatever it is? Or just append the version if missing? Both approaches LGTM.
If we go for the former, then we could remove the `@Consumes` annotations from the Api classes, since the header will be always put by this filter.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/343#pullrequestreview-16328264