fix(mssql): replace or with union in external task to improve perform…

…ance
xmgdtc · Oct 30, 2024 · 3151c40 · 3151c40
1 parent b1c8dd3
commit 3151c40
Show file tree

Hide file tree

Showing 4 changed files with 94 additions and 3 deletions.
diff --git a/engine/src/main/java/org/camunda/bpm/engine/impl/cfg/ProcessEngineConfigurationImpl.java b/engine/src/main/java/org/camunda/bpm/engine/impl/cfg/ProcessEngineConfigurationImpl.java
@@ -1941,6 +1941,7 @@ public static void initSqlSessionFactoryProperties(Properties properties, String
       properties.put("authJoin1Separator", DbSqlSessionFactory.databaseSpecificAuth1JoinSeparator.get(databaseType));
 
       properties.put("extractTimeUnitFromDate", DbSqlSessionFactory.databaseSpecificExtractTimeUnitFromDate.get(databaseType));
+      properties.put("authCheckMethodSuffix", DbSqlSessionFactory.databaseSpecificAuthCheckMethodSuffix.getOrDefault(databaseType, ""));
 
       Map<String, String> constants = DbSqlSessionFactory.dbSpecificConstants.get(databaseType);
       for (Entry<String, String> entry : constants.entrySet()) {

diff --git a/engine/src/main/java/org/camunda/bpm/engine/impl/db/sql/DbSqlSessionFactory.java b/engine/src/main/java/org/camunda/bpm/engine/impl/db/sql/DbSqlSessionFactory.java
@@ -106,6 +106,8 @@ public class DbSqlSessionFactory implements SessionFactory {
   public static final Map<String, String> databaseSpecificAuth1JoinSeparator = new HashMap<>();
 
   public static final Map<String, String> databaseSpecificExtractTimeUnitFromDate = new HashMap<>();
+  public static final Map<String, String> databaseSpecificAuthCheckMethodSuffix = new HashMap<>();
+
 
   /*
    * On SQL server, the overall maximum number of parameters in a prepared statement
@@ -729,6 +731,7 @@ public class DbSqlSessionFactory implements SessionFactory {
     databaseSpecificAuthJoinStart.put(MSSQL, defaultAuthOnStart);
     databaseSpecificAuthJoinEnd.put(MSSQL, defaultAuthOnEnd);
     databaseSpecificAuthJoinSeparator.put(MSSQL, defaultAuthOnSeparator);
+    databaseSpecificAuthCheckMethodSuffix.put(MSSQL, "_mssql");
 
     databaseSpecificAuth1JoinStart.put(MSSQL, defaultAuthOnStart);
     databaseSpecificAuth1JoinEnd.put(MSSQL, defaultAuthOnEnd);

diff --git a/engine/src/main/resources/org/camunda/bpm/engine/impl/mapping/entity/Authorization.xml b/engine/src/main/resources/org/camunda/bpm/engine/impl/mapping/entity/Authorization.xml
@@ -844,7 +844,8 @@
     <bind name="atomicChecks" value="authCheck.permissionChecks.atomicChecks" />
     <bind name="disjunctive" value="authCheck.permissionChecks.disjunctive" />
     <bind name="useLeftJoin" value="authCheck.useLeftJoin" />
-    <include refid="org.camunda.bpm.engine.impl.persistence.entity.AuthorizationEntity.authCheckJoinWithoutOnClauseWithBinding" />
+    <include refid="org.camunda.bpm.engine.impl.persistence.entity.AuthorizationEntity.authCheckJoinWithoutOnClauseWithBinding${authCheckMethodSuffix}" />
+    <!-- The suffix above is added to handle mssql differently (with UNION instead of OR for groups) -->
   </sql>
 
   <sql id="authCheckJoinWithoutOnClauseWithBinding">
@@ -861,6 +862,34 @@
       OR A.GROUP_ID_ IN <foreach item="item" index="index" collection="authGroupIds" open="(" separator="," close=")">#{item}</foreach>
     </if>
     )
+    <include refid="org.camunda.bpm.engine.impl.persistence.entity.AuthorizationEntity.atomicChecksOnResourceTypeAndPerms"></include>
+    )
+  </sql>
+
+  <sql id="authCheckJoinWithoutOnClauseWithBinding_mssql">
+    <choose>
+      <when test="useLeftJoin != null &amp;&amp; useLeftJoin == true">left join</when>
+      <otherwise>inner join</otherwise>
+    </choose>
+    (
+    SELECT A.*
+    FROM ${prefix}ACT_RU_AUTHORIZATION A
+    WHERE A.TYPE_ &lt; 2
+    AND A.USER_ID_ in ( #{authCheck.authUserId, jdbcType=VARCHAR}, '*')
+    <include refid="org.camunda.bpm.engine.impl.persistence.entity.AuthorizationEntity.atomicChecksOnResourceTypeAndPerms"></include>
+    <if test="authGroupIds != null &amp;&amp; authGroupIds.size > 0">
+      UNION (
+      SELECT A.*
+      FROM ${prefix}ACT_RU_AUTHORIZATION A
+      WHERE A.TYPE_ &lt; 2
+      AND A.GROUP_ID_ IN <foreach item="item" index="index" collection="authGroupIds" open="(" separator="," close=")">#{item}</foreach>
+      <include refid="org.camunda.bpm.engine.impl.persistence.entity.AuthorizationEntity.atomicChecksOnResourceTypeAndPerms"></include>
+      )
+    </if>
+    )
+  </sql>
+
+  <sql id="atomicChecksOnResourceTypeAndPerms">
     <if test="atomicChecks != null &amp;&amp; atomicChecks.size > 0">
       AND (
       <if test="disjunctive">
@@ -874,8 +903,7 @@
         </foreach>
       </if>
       )
-    </if>  
-    )
+    </if>
   </sql>
 
   <!-- 

diff --git a/...da/bpm/engine/test/api/authorization/externaltask/ExternalTaskQueryAuthorizationTest.java b/...da/bpm/engine/test/api/authorization/externaltask/ExternalTaskQueryAuthorizationTest.java
@@ -16,16 +16,22 @@
  */
 package org.camunda.bpm.engine.test.api.authorization.externaltask;
 
+import static org.assertj.core.api.Assertions.assertThat;
 import static org.camunda.bpm.engine.authorization.Authorization.ANY;
+import static org.camunda.bpm.engine.authorization.Permissions.ALL;
 import static org.camunda.bpm.engine.authorization.Permissions.READ;
 import static org.camunda.bpm.engine.authorization.Permissions.READ_INSTANCE;
 import static org.camunda.bpm.engine.authorization.Resources.PROCESS_DEFINITION;
 import static org.camunda.bpm.engine.authorization.Resources.PROCESS_INSTANCE;
 import static org.junit.Assert.assertEquals;
 
+import java.util.List;
 import org.camunda.bpm.engine.externaltask.ExternalTaskQuery;
+import org.camunda.bpm.engine.externaltask.LockedExternalTask;
 import org.camunda.bpm.engine.test.api.authorization.AuthorizationTest;
+import org.camunda.commons.testing.ProcessEngineLoggingRule;
 import org.junit.Before;
+import org.junit.Rule;
 import org.junit.Test;
 
 /**
@@ -34,10 +40,15 @@
  */
 public class ExternalTaskQueryAuthorizationTest extends AuthorizationTest {
 
+  protected static final String WORKER_ID = "aWorkerId";
+  protected static final String EXTERNAL_TASK_TOPIC = "externalTaskTopic";
   protected String deploymentId;
   protected String instance1Id;
   protected String instance2Id;
 
+  @Rule
+  public ProcessEngineLoggingRule loggingRule = new ProcessEngineLoggingRule();
+
   @Override
   @Before
   public void setUp() throws Exception {
@@ -135,4 +146,52 @@ public void shouldNotFindTaskWithRevokedReadOnProcessInstance() {
     // then
     verifyQueryResults(query, 1);
   }
+
+  @Test
+  public void shouldFetchAndLockWhenUserAuthorized() {
+    // given
+    createGrantAuthorization(PROCESS_DEFINITION, ANY, userId, ALL);
+
+    // when
+    List<LockedExternalTask> externalTasks = externalTaskService
+        .fetchAndLock(1, WORKER_ID)
+        .topic(EXTERNAL_TASK_TOPIC, 10)
+        .execute();
+
+    // then
+    assertThat(externalTasks).hasSize(1);
+    assertThat(externalTasks).extracting(LockedExternalTask::getTopicName).first().isEqualTo(EXTERNAL_TASK_TOPIC);
+  }
+
+  @Test
+  public void shouldFetchAndLockWhenGroupAuthorized() {
+    // given
+    createGrantAuthorizationGroup(PROCESS_DEFINITION, ANY, groupId, ALL);
+
+    // when
+    List<LockedExternalTask> externalTasks = externalTaskService
+        .fetchAndLock(1, WORKER_ID)
+        .topic(EXTERNAL_TASK_TOPIC, 10)
+        .execute();
+
+    // then
+    assertThat(externalTasks).hasSize(1);
+    assertThat(externalTasks).extracting(LockedExternalTask::getTopicName).first().isEqualTo(EXTERNAL_TASK_TOPIC);
+  }
+
+  @Test
+  public void shouldNotFetchAndLockWhenUnauthorized() {
+    // given
+    createGrantAuthorization(PROCESS_DEFINITION, ANY, userId, READ_INSTANCE);
+    createGrantAuthorizationGroup(PROCESS_DEFINITION, ANY, groupId, READ_INSTANCE);
+
+    // when
+    List<LockedExternalTask> externalTasks = externalTaskService
+        .fetchAndLock(1, WORKER_ID)
+        .topic(EXTERNAL_TASK_TOPIC, 10)
+        .execute();
+
+    // then
+    assertThat(externalTasks).isEmpty();
+  }
 }