opendev/gerrit
Code Issues Proposed changes

PluginAPI: Don't convert to String in RestApi.get() method

Browse Source 
RestApi helper class makes use of ActionContext that makes use of
RestApi in Gerrit core to route Rest calls. Because ActionContext was
optimized for invocations from JavaScript plugin hooks, it's converting
NativeString to String.

The easiest way to see why this behaviour is needed is to investigate
the call graph:

  ActionButton => ActionContext.call() => ActionContext.get() =>
  RestApiCore.get() => return NativeString.asString() =>
  alert() called from JavaSrcipt that dump string content.

An example of this approach is implemented in cookbook-plugin:

1  function onSayHelloProject(c) {
2    var f = c.textfield();
3    var t = c.checkbox();
4    var b = c.button('Say hello', {onclick: function(){
5      c.call(
6        {message: f.value, french: t.checked},
7        function(r) {
8          c.hide();
9          window.alert(r);
10         c.refresh();
11       });
12   }});

Without this implicit conversion the output on the line 9 in the
example above would not be: "Hello dude..." but "[Object]".

While this implicit conversion is needed and correct in this use case,
when calling Rest API invocations from ActionButton, it is not wanted
and wrong when called from normal plugin screen through RestApiPlugin,
that makes itself use of the same implementation of ActionContext, to
route the RestAPI calls to RestApiCore implementation. The call graph:

  PluginScreen => RestApiPlugin.get() => ActionContext.get() =>
  RestApiCore.get() => return NativeString.asString() => type error

That's because in this use case, no implicit conversion should happen,
as the callback expects NativeString and not String, e.g.:

  private void retrieveGerritVersion(final Screen screen) {
    new RestApi("config").view("server").view("version")
    .get(new AsyncCallback<NativeString>() {
      @Override
      public void onSuccess(NativeString r) {
        screen.setPageTitle("cookbook index@" + r.asString());
      }
  [...]

Conclusion: while correct and expected in one use case, it's wrong and
leads to breakage in another use case. To rectify it, avoid conversion
when called from RestApiPlugin, as it's never wanted:

  PluginScreen => RestApiPlugin.get() => ActionContext.get2() =>
  RestApiCore.get() => return NativeString

The same problem is fixed for the other Rest methods
{put|post|delete}.

Change-Id: Id6c042478cab797838c1808602712a48039ec062
This commit is contained in:
David Ostrovsky 2015-06-28 21:55:27 +02:00 committed by Edwin Kempin
parent 9b8240b0f6
commit a4bccbfafe
3 changed files with 129 additions and 7 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

73
gerrit-gwtui/src/main/java/com/google/gerrit/client/api/ActionContext.java
View File

@ -157,6 +157,14 @@ public class ActionContext extends JavaScriptObject {
api.get(wrap(cb));
}
/**
* The same as {@link #get(RestApi, JavaScriptObject)} but without converting
* a {@link NativeString} result to String.
*/
static final void getRaw(RestApi api, final JavaScriptObject cb) {
api.get(wrapRaw(cb));
}
static final void post(RestApi api, JavaScriptObject in, JavaScriptObject cb) {
if (NativeString.is(in)) {
post(api, ((NativeString) in).asString(), cb);
@ -165,14 +173,42 @@ public class ActionContext extends JavaScriptObject {
}
}
/**
* The same as {@link #post(RestApi, JavaScriptObject, JavaScriptObject)} but
* without converting a {@link NativeString} result to String.
*/
static final void postRaw(RestApi api, JavaScriptObject in, JavaScriptObject cb) {
if (NativeString.is(in)) {
postRaw(api, ((NativeString) in).asString(), cb);
} else {
api.post(in, wrapRaw(cb));
}
}
static final void post(RestApi api, String in, JavaScriptObject cb) {
api.post(in, wrap(cb));
}
/**
* The same as {@link #post(RestApi, String, JavaScriptObject)} but without
* converting a {@link NativeString} result to String.
*/
static final void postRaw(RestApi api, String in, JavaScriptObject cb) {
api.post(in, wrapRaw(cb));
}
static final void put(RestApi api, JavaScriptObject cb) {
api.put(wrap(cb));
}
/**
* The same as {@link #put(RestApi, JavaScriptObject)} but without converting
* a {@link NativeString} result to String.
*/
static final void putRaw(RestApi api, JavaScriptObject cb) {
api.put(wrapRaw(cb));
}
static final void put(RestApi api, JavaScriptObject in, JavaScriptObject cb) {
if (NativeString.is(in)) {
put(api, ((NativeString) in).asString(), cb);
@ -181,14 +217,42 @@ public class ActionContext extends JavaScriptObject {
}
}
/**
* The same as {@link #put(RestApi, JavaScriptObject, JavaScriptObject)} but
* without converting a {@link NativeString} result to String.
*/
static final void putRaw(RestApi api, JavaScriptObject in, JavaScriptObject cb) {
if (NativeString.is(in)) {
putRaw(api, ((NativeString) in).asString(), cb);
} else {
api.put(in, wrapRaw(cb));
}
}
static final void put(RestApi api, String in, JavaScriptObject cb) {
api.put(in, wrap(cb));
}
/**
* The same as {@link #put(RestApi, String, JavaScriptObject)} but without
* converting a {@link NativeString} result to String.
*/
static final void putRaw(RestApi api, String in, JavaScriptObject cb) {
api.put(in, wrapRaw(cb));
}
static final void delete(RestApi api, JavaScriptObject cb) {
api.delete(wrap(cb));
}
/**
* The same as {@link #delete(RestApi, JavaScriptObject)} but without
* converting a {@link NativeString} result to String.
*/
static final void deleteRaw(RestApi api, JavaScriptObject cb) {
api.delete(wrapRaw(cb));
}
private static GerritCallback<JavaScriptObject> wrap(final JavaScriptObject cb) {
return new GerritCallback<JavaScriptObject>() {
@Override
@ -202,4 +266,13 @@ public class ActionContext extends JavaScriptObject {
}
};
}
private static GerritCallback<JavaScriptObject> wrapRaw(final JavaScriptObject cb) {
return new GerritCallback<JavaScriptObject>() {
@Override
public void onSuccess(JavaScriptObject result) {
ApiGlue.invoke(cb, result);
}
};
}
}

49
gerrit-gwtui/src/main/java/com/google/gerrit/client/api/ApiGlue.java
View File

@ -102,6 +102,12 @@ public class ApiGlue {
Lcom/google/gwt/core/client/JavaScriptObject;)
(this._api(u), b);
},
get_raw: function(u,b) {
@com.google.gerrit.client.api.ActionContext::getRaw(
Lcom/google/gerrit/client/rpc/RestApi;
Lcom/google/gwt/core/client/JavaScriptObject;)
(this._api(u), b);
},
post: function(u,i,b) {
if (typeof i == 'string') {
@com.google.gerrit.client.api.ActionContext::post(
@ -117,6 +123,21 @@ public class ApiGlue {
(this._api(u), i, b);
}
},
post_raw: function(u,i,b) {
if (typeof i == 'string') {
@com.google.gerrit.client.api.ActionContext::postRaw(
Lcom/google/gerrit/client/rpc/RestApi;
Ljava/lang/String;
Lcom/google/gwt/core/client/JavaScriptObject;)
(this._api(u), i, b);
} else {
@com.google.gerrit.client.api.ActionContext::postRaw(
Lcom/google/gerrit/client/rpc/RestApi;
Lcom/google/gwt/core/client/JavaScriptObject;
Lcom/google/gwt/core/client/JavaScriptObject;)
(this._api(u), i, b);
}
},
put: function(u,i,b) {
if (b) {
if (typeof i == 'string') {
@ -139,6 +160,28 @@ public class ApiGlue {
(this._api(u), i);
}
},
put_raw: function(u,i,b) {
if (b) {
if (typeof i == 'string') {
@com.google.gerrit.client.api.ActionContext::putRaw(
Lcom/google/gerrit/client/rpc/RestApi;
Ljava/lang/String;
Lcom/google/gwt/core/client/JavaScriptObject;)
(this._api(u), i, b);
} else {
@com.google.gerrit.client.api.ActionContext::putRaw(
Lcom/google/gerrit/client/rpc/RestApi;
Lcom/google/gwt/core/client/JavaScriptObject;
Lcom/google/gwt/core/client/JavaScriptObject;)
(this._api(u), i, b);
}
} else {
@com.google.gerrit.client.api.ActionContext::putRaw(
Lcom/google/gerrit/client/rpc/RestApi;
Lcom/google/gwt/core/client/JavaScriptObject;)
(this._api(u), i);
}
},
'delete': function(u,b) {
@com.google.gerrit.client.api.ActionContext::delete(
Lcom/google/gerrit/client/rpc/RestApi;
@ -151,6 +194,12 @@ public class ApiGlue {
Lcom/google/gwt/core/client/JavaScriptObject;)
(this._api(u), b);
},
del_raw: function(u,b) {
@com.google.gerrit.client.api.ActionContext::deleteRaw(
Lcom/google/gerrit/client/rpc/RestApi;
Lcom/google/gwt/core/client/JavaScriptObject;)
(this._api(u), b);
},
};
}-*/;

14
gerrit-plugin-gwtui/src/main/java/com/google/gerrit/plugin/client/rpc/RestApi.java
View File

@ -110,7 +110,7 @@ public class RestApi {
}
private static native void get(String p, JavaScriptObject r)
/*-{ $wnd.Gerrit.get(p, r) }-*/;
/*-{ $wnd.Gerrit.get_raw(p, r) }-*/;
public <T extends JavaScriptObject>
void put(AsyncCallback<T> cb) {
@ -118,7 +118,7 @@ public class RestApi {
}
private static native void put(String p, JavaScriptObject r)
/*-{ $wnd.Gerrit.put(p, r) }-*/;
/*-{ $wnd.Gerrit.put_raw(p, r) }-*/;
public <T extends JavaScriptObject>
void put(String content, AsyncCallback<T> cb) {
@ -127,7 +127,7 @@ public class RestApi {
private static native
void put(String p, String c, JavaScriptObject r)
/*-{ $wnd.Gerrit.put(p, c, r) }-*/;
/*-{ $wnd.Gerrit.put_raw(p, c, r) }-*/;
public <T extends JavaScriptObject>
void put(JavaScriptObject content, AsyncCallback<T> cb) {
@ -136,7 +136,7 @@ public class RestApi {
private static native
void put(String p, JavaScriptObject c, JavaScriptObject r)
/*-{ $wnd.Gerrit.put(p, c, r) }-*/;
/*-{ $wnd.Gerrit.put_raw(p, c, r) }-*/;
public <T extends JavaScriptObject>
void post(String content, AsyncCallback<T> cb) {
@ -145,7 +145,7 @@ public class RestApi {
private static native
void post(String p, String c, JavaScriptObject r)
/*-{ $wnd.Gerrit.post(p, c, r) }-*/;
/*-{ $wnd.Gerrit.post_raw(p, c, r) }-*/;
public <T extends JavaScriptObject>
void post(JavaScriptObject content, AsyncCallback<T> cb) {
@ -154,14 +154,14 @@ public class RestApi {
private static native
void post(String p, JavaScriptObject c, JavaScriptObject r)
/*-{ $wnd.Gerrit.post(p, c, r) }-*/;
/*-{ $wnd.Gerrit.post_raw(p, c, r) }-*/;
public void delete(AsyncCallback<NoContent> cb) {
delete(path(), wrap(cb));
}
private static native void delete(String p, JavaScriptObject r)
/*-{ $wnd.Gerrit.del(p, r) }-*/;
/*-{ $wnd.Gerrit.del_raw(p, r) }-*/;
private static native <T extends JavaScriptObject>
JavaScriptObject wrap(AsyncCallback<T> b) /*-{