[jira] [Commented] (SLING-8407) JobManagerImpl.findJobs should prevent traversal

[Thomas Mueller (JIRA)]

    [ 
https://issues.apache.org/jira/browse/SLING-8407?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16836410#comment-16836410
 ] 

Thomas Mueller commented on SLING-8407:
---------------------------------------

Regarding the configuration option would this patch work? The option is enabled 
by default (given that the same option is used in SLING-7544, and there are no 
tests for older versions of Oak, I think the point of supporting older Oak 
version is moot.)

{noformat}
--- 
a/src/main/java/org/apache/sling/event/impl/jobs/config/JobManagerConfiguration.java
+++ 
b/src/main/java/org/apache/sling/event/impl/jobs/config/JobManagerConfiguration.java
@@ -117,6 +117,9 @@ public class JobManagerConfiguration {
     /** Configuration property for the scheduled jobs path. */
     public static final String PROPERTY_SCHEDULED_JOBS_PATH = 
"job.scheduled.jobs.path";
 
+    /** Configuration property to fail traversal queries. */
+    public static final String PROPERTY_FAIL_TRAVERSAL = "job.failTraversal";
+
     /** The jobs base path with a slash. */
     private String jobsBasePathWithSlash;
 
@@ -178,6 +181,8 @@ public class JobManagerConfiguration {
     /** The topology capabilities. */
     private volatile TopologyCapabilities topologyCapabilities;
 
+    private boolean failTraversal;
+
     /**
      * Activate this component.
      * @param props Configuration properties
@@ -207,6 +212,8 @@ public class JobManagerConfiguration {
             DEFAULT_SCHEDULED_JOBS_PATH);
         this.scheduledJobsPathWithSlash = this.scheduledJobsPath + "/";
 
+        this.failTraversal = 
PropertiesUtil.toBoolean(props.get(PROPERTY_FAIL_TRAVERSAL), true);
+
         this.historyCleanUpRemovedJobs = config.cleanup_period();
 
         // create initial resources
@@ -460,6 +467,15 @@ public class JobManagerConfiguration {
         return (slash ? this.scheduledJobsPathWithSlash : 
this.scheduledJobsPath);
     }
 
+    /**
+     * Whether to fail traversal when querying for jobs.
+     *
+     * @return true if enabled
+     */
+    public boolean isFailTraversal() {
+        return failTraversal;
+    }
+
     /**
      * Stop processing
      */

--- a/src/main/java/org/apache/sling/event/impl/jobs/JobManagerImpl.java
+++ b/src/main/java/org/apache/sling/event/impl/jobs/JobManagerImpl.java
@@ -520,6 +520,9 @@ public class JobManagerImpl
                 buf.append(Job.PROPERTY_JOB_CREATED);
                 buf.append(" ascending");
             }
+            if (configuration.isFailTraversal()) {
+                buf.append(" option(traversal fail)");
+            }
             final Iterator<Resource> iter = 
resolver.findResources(buf.toString(), "xpath");
             long count = 0;
 
@@ -535,7 +538,11 @@ public class JobManagerImpl
                 }
              }
         } catch (final QuerySyntaxException qse) {
-            logger.warn("Query syntax wrong " + buf.toString(), qse);
+            // no index is available
+            String message = "Query " + buf.toString() + " failed: " + 
qse.getMessage();
+            logger.debug(message, qse);
+            logger.info(message);
+            throw new IllegalStateException();
         } finally {
             resolver.close();
         }
{noformat}

> JobManagerImpl.findJobs should prevent traversal
> ------------------------------------------------
>
>                 Key: SLING-8407
>                 URL: https://issues.apache.org/jira/browse/SLING-8407
>             Project: Sling
>          Issue Type: Improvement
>          Components: Event
>            Reporter: Thomas Mueller
>            Priority: Major
>
> The method 
> [JobManagerImpl.findJobs|https://github.com/apache/sling-org-apache-sling-event/blob/master/src/main/java/org/apache/sling/event/impl/jobs/JobManagerImpl.java#L373]
>  runs a JCR query to find all jobs for a topic.
> It is possible that such a query is running while the repository isn't 
> initialized yet, meaning while the index isn't available yet. What is 
> happening in this case is that the query is traversing all nodes below that 
> path, triggering a warning that the query doesn't use an index. It is 
> sometimes happening when a health check is running before the repository is 
> initialized (ReplicationQueueHealthCheck and DistributionQueueHealthCheck).
> It doesn't make sense that the query traverses the nodes. It should use an 
> index. If the index isn't available yet, it should fail. Therefore, the query 
> should use "option(traversal fail)". That would result in an exception that 
> can be caught.  I will log a related issue to change the health checks to 
> process this exception and return HEALTH_CHECK_ERROR for this case.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)