[GitHub] [lucene-solr] uschindler commented on a change in pull request #1624: use MethodHandles in AnnotatedAPI
uschindler commented on a change in pull request #1624:
URL: https://github.com/apache/lucene-solr/pull/1624#discussion_r447650963
##
File path: solr/core/src/java/org/apache/solr/api/AnnotatedApi.java
##
@@ -87,16 +83,18 @@ public EndPoint getEndPoint() {
public static List getApis(Object obj) {
return getApis(obj.getClass(), obj);
}
- public static List getApis(Class klas , Object obj) {
-if (!Modifier.isPublic(klas.getModifiers())) {
- throw new RuntimeException(klas.getName() + " is not public");
+ public static List getApis(Class theClass , Object
obj) {
+Class klas = null;
Review comment:
this should be declared `final` and not nulled.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] uschindler commented on a change in pull request #1624: use MethodHandles in AnnotatedAPI
uschindler commented on a change in pull request #1624:
URL: https://github.com/apache/lucene-solr/pull/1624#discussion_r446890033
##
File path: solr/core/src/java/org/apache/solr/api/AnnotatedApi.java
##
@@ -306,7 +313,7 @@ void invoke(SolrQueryRequest req, SolrQueryResponse rsp,
CommandOperation cmd) {
} catch (InvocationTargetException ite) {
Review comment:
This catch block can go away!
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] uschindler commented on a change in pull request #1624: use MethodHandles in AnnotatedAPI
uschindler commented on a change in pull request #1624:
URL: https://github.com/apache/lucene-solr/pull/1624#discussion_r446632932
##
File path: solr/core/src/java/org/apache/solr/api/AnnotatedApi.java
##
@@ -113,7 +116,7 @@ public EndPoint getEndPoint() {
return Collections.singletonList(new AnnotatedApi(specProvider,
endPoint, commands, null));
} else {
List apis = new ArrayList<>();
- for (Method m : klas.getDeclaredMethods()) {
+ for (Method m : klas.getMethods()) {
EndPoint endPoint = m.getAnnotation(EndPoint.class);
if (endPoint == null) continue;
if (!Modifier.isPublic(m.getModifiers())) {
Review comment:
The if statement is not needed, because the public checks are done by
unreflect().
This code is from reflection times to catch error early. By using method
handles everything is checked and linked early, so you can't get any surprises
later. So the public checks is complete obsolete, all this is handled by
unreflect.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] uschindler commented on a change in pull request #1624: use MethodHandles in AnnotatedAPI
uschindler commented on a change in pull request #1624:
URL: https://github.com/apache/lucene-solr/pull/1624#discussion_r446632680
##
File path: solr/core/src/java/org/apache/solr/api/AnnotatedApi.java
##
@@ -225,7 +228,11 @@ public void call(SolrQueryRequest req, SolrQueryResponse
rsp) {
if (Modifier.isPublic(method.getModifiers())) {
this.command = command;
this.obj = obj;
-this.method = method;
+try {
+ this.method = MethodHandles.publicLookup().unreflect(method);
+} catch (IllegalAccessException e) {
+ throw new RuntimeException("Unable to unlookup method");
Review comment:
The exception text should not mention internal details. It should just
say that the method cannot be called, as it's not accessible (non public, class
not visible, different classloader,...)
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] uschindler commented on a change in pull request #1624: use MethodHandles in AnnotatedAPI
uschindler commented on a change in pull request #1624:
URL: https://github.com/apache/lucene-solr/pull/1624#discussion_r446632522
##
File path: solr/core/src/java/org/apache/solr/api/AnnotatedApi.java
##
@@ -113,7 +116,7 @@ public EndPoint getEndPoint() {
return Collections.singletonList(new AnnotatedApi(specProvider,
endPoint, commands, null));
} else {
List apis = new ArrayList<>();
- for (Method m : klas.getDeclaredMethods()) {
+ for (Method m : klas.getMethods()) {
EndPoint endPoint = m.getAnnotation(EndPoint.class);
if (endPoint == null) continue;
if (!Modifier.isPublic(m.getModifiers())) {
Review comment:
Yes. For both branches.
In 8.x we have to fix the class accessibility check (unrelated to the if
statement).
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] uschindler commented on a change in pull request #1624: use MethodHandles in AnnotatedAPI
uschindler commented on a change in pull request #1624:
URL: https://github.com/apache/lucene-solr/pull/1624#discussion_r446632093
##
File path: solr/core/src/java/org/apache/solr/api/AnnotatedApi.java
##
@@ -87,16 +88,18 @@ public EndPoint getEndPoint() {
public static List getApis(Object obj) {
return getApis(obj.getClass(), obj);
}
- public static List getApis(Class klas , Object obj) {
-if (!Modifier.isPublic(klas.getModifiers())) {
- throw new RuntimeException(klas.getName() + " is not public");
+ public static List getApis(Class theClass , Object
obj) {
+Class klas = null;
+try {
+ klas = MethodHandles.publicLookup().accessClass(theClass);
+} catch (IllegalAccessException e) {
+ throw new RuntimeException(klas.getName() + " is not public", e);
Review comment:
Good catch. The code here is a mess, as you declare the variable as
null. You should declare it final before catch block and not assign null. Then
compiler would have been complained.
The current code is a horrible antipattern. Always declare variables as
final before try and assign once.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] uschindler commented on a change in pull request #1624: use MethodHandles in AnnotatedAPI
uschindler commented on a change in pull request #1624:
URL: https://github.com/apache/lucene-solr/pull/1624#discussion_r446631519
##
File path: solr/core/src/java/org/apache/solr/api/AnnotatedApi.java
##
@@ -306,7 +313,7 @@ void invoke(SolrQueryRequest req, SolrQueryResponse rsp,
CommandOperation cmd) {
} catch (InvocationTargetException ite) {
log.error("Error executing command ", ite);
throw new SolrException(SolrException.ErrorCode.SERVER_ERROR,
ite.getCause());
- } catch (Exception e) {
+ } catch (Throwable e) {
Review comment:
There are two things to do:
- have the try-catch only around the invoke and nothing else, so we don't
accidentally catch any unwanted exception in the surrounding code
- only checked exceptions should be wrapped, everything else rethrown. This
keeps stack traces clean.
The reason for the declaration of "Throwable" comes from the fact that the
method handle always throws the original Exception and it's now wrapped like
for reflection. As checked exceptions is a compiler thing, normal code using
method handles in bytecode or invokedynamic never needs to catch Throwable.
It's only the stupid Java compiler that enforces us to catch everything,
because it can't know if the handle might possibly throw a checked exception.
When writing scripting languages you just ignore the Throwable when you produce
bytecode. 🤓
Here we should catch all possible exceptions that are commonly thrown by
plugins: SolrException, Runtime exception, Error and rethrown those. The last
catch block would catch Throwable and log it separately as some fatal error.
The code above should check the possible exceptions a method may throw and
forbid e.g. a plugin to throw Interrupted exception. So maybe get all throws
clauses of method and have whitelist. In the catch clause then throw
AssertionError if the "unknown" exception happens.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] uschindler commented on a change in pull request #1624: use MethodHandles in AnnotatedAPI
uschindler commented on a change in pull request #1624:
URL: https://github.com/apache/lucene-solr/pull/1624#discussion_r446631519
##
File path: solr/core/src/java/org/apache/solr/api/AnnotatedApi.java
##
@@ -306,7 +313,7 @@ void invoke(SolrQueryRequest req, SolrQueryResponse rsp,
CommandOperation cmd) {
} catch (InvocationTargetException ite) {
log.error("Error executing command ", ite);
throw new SolrException(SolrException.ErrorCode.SERVER_ERROR,
ite.getCause());
- } catch (Exception e) {
+ } catch (Throwable e) {
Review comment:
There are two things to do:
- have they try catch only around the invoke and nothing else, so we don't
accidentally catch any unwanted exception in the surrounding code
- only checked exceptions should be wrapped, everything else rethrown. This
keeps stack traces clean.
The reason for the declaration of "Throwable" comes from the fact that the
method handle always throws the original Exception and it's now wrapped like
for reflection. As checked exceptions is a compiler thing, normal code using
method handles in bytecode or invokedynamic never needs to catch Throwable.
It's only the stupid Java compiler that enforces us to catch everything,
because it can't know if the handle might possibly throw a checked exception.
When writing scripting languages you just ignore the Throwable when you produce
bytecode. 🤓
Here we should catch all possible exceptions that are commonly thrown by
plugins: SolrException, Runtime exception, Error and rethrown those. The last
catch block would catch Throwable and log it separately as some fatal error.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] uschindler commented on a change in pull request #1624: use MethodHandles in AnnotatedAPI
uschindler commented on a change in pull request #1624:
URL: https://github.com/apache/lucene-solr/pull/1624#discussion_r446630480
##
File path: solr/core/src/java/org/apache/solr/api/AnnotatedApi.java
##
@@ -113,7 +116,7 @@ public EndPoint getEndPoint() {
return Collections.singletonList(new AnnotatedApi(specProvider,
endPoint, commands, null));
} else {
List apis = new ArrayList<>();
- for (Method m : klas.getDeclaredMethods()) {
+ for (Method m : klas.getMethods()) {
EndPoint endPoint = m.getAnnotation(EndPoint.class);
if (endPoint == null) continue;
if (!Modifier.isPublic(m.getModifiers())) {
Review comment:
Yes, we still need some way to check public methods. The accessClass()
method above returns itsself, but the check for public is not needed if we use
getMethods() instead of getDeclaredMethods().
The accessClass() check can't be backported to 8.x as it is new in Java 9.
So we need to somehow do the check for accessibility manually.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
