From 430d15f32d6e3130be856c4f90ddccf837c2e06e Mon Sep 17 00:00:00 2001 From: tiagosalvado10 <9038083+tiagosalvado10@users.noreply.github.com> Date: Tue, 13 Sep 2022 16:40:23 +0100 Subject: [PATCH] [MNT-21901] Prevent update preferences concurrency errors (#821) * [MNT-21901] Added PersistenceException to retry exceptions. Added validation to get retry cause * [MNT-21901] Check exception message only if it is JavaScriptException * [MNT-21901] Removed unit test from repository. Added new test on remote-api. Changed ExceptionStackUtil validation. * [MNT-21901] Improved JavascriptException validation. Added exception delimiter to prevent accepting exceptions with the same partial name. --- .../alfresco/error/ExceptionStackUtil.java | 14 +++- .../preference/PreferenceServiceTest.java | 69 +++++++++++++++++-- .../RetryingTransactionHelper.java | 4 +- .../script/test_preferenceService2.js | 11 +++ 4 files changed, 91 insertions(+), 7 deletions(-) create mode 100644 repository/src/test/resources/org/alfresco/repo/preference/script/test_preferenceService2.js diff --git a/core/src/main/java/org/alfresco/error/ExceptionStackUtil.java b/core/src/main/java/org/alfresco/error/ExceptionStackUtil.java index 8ff83c2452..7967722f19 100644 --- a/core/src/main/java/org/alfresco/error/ExceptionStackUtil.java +++ b/core/src/main/java/org/alfresco/error/ExceptionStackUtil.java @@ -25,6 +25,9 @@ package org.alfresco.error; */ public class ExceptionStackUtil { + private static final String JAVASCRIPT_EXCEPTION = "org.mozilla.javascript.JavaScriptException"; + private static final String EXCEPTION_DELIMITER = ":"; + /** * Searches through the exception stack of the given throwable to find any instance * of the possible cause. The top-level throwable will also be tested. @@ -38,10 +41,17 @@ public class ExceptionStackUtil { while (throwable != null) { + Class throwableClass = throwable.getClass(); + + boolean isJavaScriptException = throwableClass.getName().contains(JAVASCRIPT_EXCEPTION); + String throwableMsg = throwable.getMessage() != null ? throwable.getMessage() : ""; + for (Class possibleCauseClass : possibleCauses) { - Class throwableClass = throwable.getClass(); - if (possibleCauseClass.isAssignableFrom(throwableClass)) + String possibleCauseClassName = possibleCauseClass.getName(); + + if (possibleCauseClass.isAssignableFrom(throwableClass) + || (isJavaScriptException && throwableMsg.contains(possibleCauseClassName + EXCEPTION_DELIMITER))) { // We have a match return throwable; diff --git a/remote-api/src/test/java/org/alfresco/repo/web/scripts/preference/PreferenceServiceTest.java b/remote-api/src/test/java/org/alfresco/repo/web/scripts/preference/PreferenceServiceTest.java index abd5faaeb4..ea20595684 100644 --- a/remote-api/src/test/java/org/alfresco/repo/web/scripts/preference/PreferenceServiceTest.java +++ b/remote-api/src/test/java/org/alfresco/repo/web/scripts/preference/PreferenceServiceTest.java @@ -55,10 +55,14 @@ public class PreferenceServiceTest extends BaseWebScriptTest private PersonService personService; private static final String USER_ONE = "PreferenceTestOne" + System.currentTimeMillis(); - + private static final String USER_TWO = "PreferenceTestTwo" + System.currentTimeMillis(); private static final String USER_BAD = "PreferenceTestBad" + System.currentTimeMillis(); - private static final String URL = "/api/people/" + USER_ONE + "/preferences";; + private static final String URL = "/api/people/" + USER_ONE + "/preferences"; + private static final String URL2 = "/api/people/" + USER_TWO + "/preferences"; + + private static final int NUM_THREADS = 3; + private static Integer THREADS_FINISHED = 0; @Override protected void setUp() throws Exception @@ -75,6 +79,7 @@ public class PreferenceServiceTest extends BaseWebScriptTest // Create users createUser(USER_ONE); + createUser(USER_TWO); createUser(USER_BAD); // Do tests as user one @@ -103,7 +108,6 @@ public class PreferenceServiceTest extends BaseWebScriptTest { super.tearDown(); this.authenticationComponent.setCurrentUser(AuthenticationUtil.getAdminUserName()); - } public void testPreferences() throws Exception @@ -193,6 +197,64 @@ public class PreferenceServiceTest extends BaseWebScriptTest sendRequest(new GetRequest("/api/people/noExistUser/preferences"), 404); } + public void testPreferencesMNT21901() throws Exception + { + String[] body = { + "{\"org\":{\"alfresco\":{\"share\":{\"forum\":{\"summary\":{\"dashlet\":{\"component-1-5\":{\"topics\":\"mine\"}}}}}}}}", + "{\"org\":{\"alfresco\":{\"share\":{\"forum\":{\"summary\":{\"dashlet\":{\"component-2-5\":{\"topics\":\"mine\"}}}}}}}}", + "{\"org\":{\"alfresco\":{\"share\":{\"forum\":{\"summary\":{\"dashlet\":{\"component-2-5\":{\"history\":\"1\"}}}}}}}}" + }; + + Thread[] threads = new Thread[NUM_THREADS]; + + for (int i = 0; i < NUM_THREADS; i++) + { + UpdatePreferencesThread t = new UpdatePreferencesThread(i, body[i]); + threads[i] = t; + t.start(); + } + + for (int i = 0; i < threads.length; i++) + { + threads[i].join(); + } + + authenticationComponent.setCurrentUser(USER_ONE); + + if (THREADS_FINISHED != NUM_THREADS) + { + fail("An error has occurred when updating preferences in concurrency context"); + } + } + + private class UpdatePreferencesThread extends Thread + { + private String body; + + UpdatePreferencesThread(int threadNum, String body) + { + super(UpdatePreferencesThread.class.getSimpleName() + "-" + threadNum); + this.body = body; + } + + @Override + public void run() + { + authenticationComponent.setCurrentUser(USER_TWO); + + try + { + Response resp = sendRequest(new PostRequest(URL2, body, "application/json"), 200); + assertEquals(0, resp.getContentLength()); + THREADS_FINISHED++; + } + catch (Exception e) + { + // Intentionally empty + } + } + } + private JSONObject getPreferenceObj() throws JSONException { JSONObject jsonObject = new JSONObject(); @@ -208,5 +270,4 @@ public class PreferenceServiceTest extends BaseWebScriptTest assertEquals(10, jsonObject.get("numberValue")); assertEquals(BigDecimal.valueOf(3.142), jsonObject.get("numberValue2")); } - } diff --git a/repository/src/main/java/org/alfresco/repo/transaction/RetryingTransactionHelper.java b/repository/src/main/java/org/alfresco/repo/transaction/RetryingTransactionHelper.java index 5e77b943b5..4a4c4d950e 100644 --- a/repository/src/main/java/org/alfresco/repo/transaction/RetryingTransactionHelper.java +++ b/repository/src/main/java/org/alfresco/repo/transaction/RetryingTransactionHelper.java @@ -51,6 +51,7 @@ import org.alfresco.service.license.LicenseIntegrityException; import org.alfresco.util.LockHelper.LockTryException; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import org.apache.ibatis.exceptions.PersistenceException; import org.apache.ibatis.exceptions.TooManyResultsException; import org.springframework.aop.MethodBeforeAdvice; import org.springframework.aop.framework.ProxyFactory; @@ -105,7 +106,8 @@ public class RetryingTransactionHelper DataIntegrityViolationException.class, LicenseIntegrityException.class, TooManyResultsException.class, // Expected one result but found multiple (bad key alert) - LockTryException.class + LockTryException.class, + PersistenceException.class }; List> retryExceptions = new ArrayList>(); diff --git a/repository/src/test/resources/org/alfresco/repo/preference/script/test_preferenceService2.js b/repository/src/test/resources/org/alfresco/repo/preference/script/test_preferenceService2.js new file mode 100644 index 0000000000..a4088e7cce --- /dev/null +++ b/repository/src/test/resources/org/alfresco/repo/preference/script/test_preferenceService2.js @@ -0,0 +1,11 @@ +function testPreferences() +{ + var preferences = { + "org.alfresco.share.forum.summary.dashlet.component-1-3.history": "1" + }; + + preferenceService.setPreferences(username, preferences); +} + +// Execute tests +testPreferences(); \ No newline at end of file