You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "uschindler (via GitHub)" <gi...@apache.org> on 2023/05/18 10:23:39 UTC
[GitHub] [lucene] uschindler opened a new pull request, #12308: Wrap Query rewrite backwards layer with AccessController
uschindler opened a new pull request, #12308:
URL: https://github.com/apache/lucene/pull/12308
This fixes #12304 for query.
It also adds a note to changes and puts more documentation into `VirtualMethod`. The documentation changes will be forward-ported to main branch.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on pull request #12308: Wrap Query rewrite backwards layer with AccessController
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on PR #12308:
URL: https://github.com/apache/lucene/pull/12308#issuecomment-1553715929
Reverted it. I left the fixes of a test calling the deprecated rewrite.
I will merge this tomorrow and forward-port the documentation changes to main.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on pull request #12308: Wrap Query rewrite backwards layer with AccessController
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on PR #12308:
URL: https://github.com/apache/lucene/pull/12308#issuecomment-1552863855
Actually to reduce impact of this more we could add an if statement in the Query constructor to not do the override/distance check for Lucene's own classes (as we have all ported to use new API). The problem only affects external classes (and the corresponding Test). We could add a !this.getClass().getName().startsWith("org.apache.lucene.") && AccessController(...) check there.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on a diff in pull request #12308: Wrap Query rewrite backwards layer with AccessController
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on code in PR #12308:
URL: https://github.com/apache/lucene/pull/12308#discussion_r1198197916
##########
lucene/core/src/java/org/apache/lucene/search/Query.java:
##########
@@ -50,8 +52,30 @@ public abstract class Query {
new VirtualMethod<>(Query.class, "rewrite", IndexReader.class);
private static final VirtualMethod<Query> newMethod =
new VirtualMethod<>(Query.class, "rewrite", IndexSearcher.class);
- private final boolean isDeprecatedRewriteMethodOverridden =
- VirtualMethod.compareImplementationDistance(this.getClass(), oldMethod, newMethod) > 0;
+ private final boolean isDeprecatedRewriteMethodOverridden;
+
+ /** Constructor for query classes. */
+ public Query() {
+ var clazz = this.getClass();
+ if (clazz.getName().startsWith("org.apache.lucene.")) {
Review Comment:
That's an argument to not do this optimization. I also tend to revert my last commit.
I was just thinking about a solution that does not need the extra permission.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on pull request #12308: Wrap Query rewrite backwards layer with AccessController
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on PR #12308:
URL: https://github.com/apache/lucene/pull/12308#issuecomment-1553429260
Thanks for the feedback. In the meantime I have tried out some code to "optimize" the case of Lucene's own classes.
If the classname starts with "org.apache.lucene.", it will assume that the deprecated method is never overridden. Unfortunately, I found 2 tests (in TestKnnFloatQueryVector) using the deprecated method in 9.x, but nothing overriding it.
The whole patch is a bit hacky, but works and spares the refection for Lucene's own classes. I committed the change, if we should not add the complexity I can revert it.
Pros:
- no reflection needed for Lucene's own classes
- if you do not subclass Lucene classes, theres no need to add runtime permission
Cons:
- We have to be careful that Lucene's own classes never ever override deprecated rewrite(), as it is asumed they don't do it
- If somebody subclasses a lucene class in Lucene's namespace it won't be detected
Info:
- If somebody shaded Lucene the optimization may not work, so its not needed.
What do you think? Revert or not?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] reta commented on pull request #12308: Wrap Query rewrite backwards layer with AccessController
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on PR #12308:
URL: https://github.com/apache/lucene/pull/12308#issuecomment-1553186693
Tested locally, the issue is gone, tests pass normally, thanks @uschindler !
--
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.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler merged pull request #12308: Wrap Query rewrite backwards layer with AccessController
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler merged PR #12308:
URL: https://github.com/apache/lucene/pull/12308
--
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.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] reta commented on a diff in pull request #12308: Wrap Query rewrite backwards layer with AccessController
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on code in PR #12308:
URL: https://github.com/apache/lucene/pull/12308#discussion_r1198196299
##########
lucene/core/src/java/org/apache/lucene/search/Query.java:
##########
@@ -50,8 +52,30 @@ public abstract class Query {
new VirtualMethod<>(Query.class, "rewrite", IndexReader.class);
private static final VirtualMethod<Query> newMethod =
new VirtualMethod<>(Query.class, "rewrite", IndexSearcher.class);
- private final boolean isDeprecatedRewriteMethodOverridden =
- VirtualMethod.compareImplementationDistance(this.getClass(), oldMethod, newMethod) > 0;
+ private final boolean isDeprecatedRewriteMethodOverridden;
+
+ /** Constructor for query classes. */
+ public Query() {
+ var clazz = this.getClass();
+ if (clazz.getName().startsWith("org.apache.lucene.")) {
Review Comment:
Just to note here, both OpenSearch [1] and Elasticsearch [2] host own classes under `org.apache.lucene.` package as well:
[1] https://github.com/opensearch-project/OpenSearch/tree/main/server/src/main/java/org/apache/lucene
[2] https://github.com/elastic/elasticsearch/tree/7.17/server/src/main/java/org/apache/lucene
--
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.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org