From cd38dfbfebfd88fbee184378e5c99eaa842f4e39 Mon Sep 17 00:00:00 2001 From: Neil McErlean Date: Wed, 18 Sep 2013 15:27:51 +0000 Subject: [PATCH] Fix for ALF-20023 Recent Sites and Favourite Sites in copy/move pickers empty So the bug was caused by 2 problems: one in ScriptPreferenceService and one in person.sites.get.js The first problem was that ScriptPreferenceService constructs raw Mozilla NativeObjects - a very unusual practice in itself - and it does not provide a default value as required by ECMA 9.1 from the ECMA standard. I've added this code to the NativeObject (can't change the type to ScriptableHashMap or similar as the API is published). I've also fixed the same bug (unreported, possibly never apparent) in a jbpm class. The second problem was that person.sites.get.js simply could never return preferences data if the caller provided a filter (favourites or recents) and did not also provide a page size. There was a logic error in the algorithm such that the size defaulted to 0 thus providing a 'page' of zero results as a default. I assume this is a merge error from cloud, but I don't know. I added tests for the JavaScript API's favourites/recents filter calls and logging here and there. Also deleted some dead code. git-svn-id: https://svn.alfresco.com/repos/alfresco-enterprise/alfresco/HEAD/root@55474 c4b6b30b-aa2e-2d43-bbcb-ca4b014f7261 --- .../preference/PreferenceServiceImpl.java | 18 +- .../script/ScriptPreferenceService.java | 16 +- .../repo/workflow/jbpm/NullScope.java | 6 +- .../preference/PreferenceServiceImplTest.java | 5 +- .../script/test_preferenceService.js | 157 ++++++++++++------ 5 files changed, 137 insertions(+), 65 deletions(-) diff --git a/source/java/org/alfresco/repo/preference/PreferenceServiceImpl.java b/source/java/org/alfresco/repo/preference/PreferenceServiceImpl.java index 75cf994869..6a7f412e32 100644 --- a/source/java/org/alfresco/repo/preference/PreferenceServiceImpl.java +++ b/source/java/org/alfresco/repo/preference/PreferenceServiceImpl.java @@ -21,11 +21,9 @@ package org.alfresco.repo.preference; import java.io.Serializable; import java.util.ArrayList; import java.util.Date; -import java.util.HashSet; import java.util.Iterator; import java.util.List; import java.util.Map; -import java.util.Set; import java.util.TreeMap; import org.alfresco.error.AlfrescoRuntimeException; @@ -50,6 +48,8 @@ import org.alfresco.service.cmr.security.PermissionService; import org.alfresco.service.cmr.security.PersonService; import org.alfresco.util.ISO8601DateFormat; import org.alfresco.util.Pair; +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; import org.json.JSONException; import org.json.JSONObject; @@ -60,7 +60,9 @@ import org.json.JSONObject; */ public class PreferenceServiceImpl implements PreferenceService { - private static final String SHARE_SITES_PREFERENCE_KEY = "org.alfresco.share.sites.favourites."; + private static final Log log = LogFactory.getLog(PreferenceServiceImpl.class); + + private static final String SHARE_SITES_PREFERENCE_KEY = "org.alfresco.share.sites.favourites."; private static final int SHARE_SITES_PREFERENCE_KEY_LEN = SHARE_SITES_PREFERENCE_KEY.length(); private static final String EXT_SITES_PREFERENCE_KEY = "org.alfresco.ext.sites.favourites."; @@ -192,12 +194,12 @@ public class PreferenceServiceImpl implements PreferenceService @SuppressWarnings({ "unchecked" }) public Map getPreferences(String userName, String preferenceFilter) { + if (log.isTraceEnabled()) { log.trace("getPreferences(" + userName + ", " + preferenceFilter + ")"); } + Map preferences = new TreeMap(); try { - Set siteIds = new HashSet(); - JSONObject jsonPrefs = getPreferencesObject(userName); if(jsonPrefs != null) { @@ -220,7 +222,6 @@ public class PreferenceServiceImpl implements PreferenceService String siteId = key.substring(SHARE_SITES_PREFERENCE_KEY_LEN, idx); StringBuilder sb = new StringBuilder(SHARE_SITES_PREFERENCE_KEY); sb.append(siteId); - siteIds.add(siteId); key = sb.toString(); } @@ -231,7 +232,6 @@ public class PreferenceServiceImpl implements PreferenceService StringBuilder sb = new StringBuilder(EXT_SITES_PREFERENCE_KEY); sb.append(siteId); sb.append(".createdAt"); - siteIds.add(siteId); key = sb.toString(); } else if(preferences.containsKey(key)) @@ -254,9 +254,11 @@ public class PreferenceServiceImpl implements PreferenceService } catch (JSONException exception) { - throw new AlfrescoRuntimeException("Can not get preferences for " + userName + " because there was an error pasing the JSON data.", exception); + throw new AlfrescoRuntimeException("Can not get preferences for " + userName + " because there was an error parsing the JSON data.", exception); } + if (log.isTraceEnabled()) { log.trace("result = " + preferences); } + return preferences; } diff --git a/source/java/org/alfresco/repo/preference/script/ScriptPreferenceService.java b/source/java/org/alfresco/repo/preference/script/ScriptPreferenceService.java index 77dca21728..f3f3a3fd32 100644 --- a/source/java/org/alfresco/repo/preference/script/ScriptPreferenceService.java +++ b/source/java/org/alfresco/repo/preference/script/ScriptPreferenceService.java @@ -68,8 +68,10 @@ public class ScriptPreferenceService extends BaseScopableProcessorExtension public NativeObject getPreferences(String userName, String preferenceFilter) { + // It's a tad unusual to return a NativeObject like this - at least within Alfresco. + // But we can't change it to e.g. a ScriptableHashMap as the API is published. Map prefs = this.preferenceService.getPreferences(userName, preferenceFilter); - NativeObject result = new NativeObject(); + NativeObject result = new NativeObjectDV(); for (Map.Entry entry : prefs.entrySet()) { @@ -80,6 +82,16 @@ public class ScriptPreferenceService extends BaseScopableProcessorExtension return result; } + /** + * This extension of NativeObject adds a default value. See ALF-20023 for some background. + */ + private static class NativeObjectDV extends NativeObject + { + private static final long serialVersionUID = 1L; + + @Override public Object getDefaultValue(@SuppressWarnings("rawtypes") Class typeHint) { return toString(); } + } + private void setPrefValue(String[] keys, Serializable value, NativeObject object) { NativeObject currentObject = object; @@ -96,7 +108,7 @@ public class ScriptPreferenceService extends BaseScopableProcessorExtension Object temp = currentObject.get(key, currentObject); if (temp == null || temp instanceof NativeObject == false) { - newObject = new NativeObject(); + newObject = new NativeObjectDV(); currentObject.put(key, currentObject, newObject); } else diff --git a/source/java/org/alfresco/repo/workflow/jbpm/NullScope.java b/source/java/org/alfresco/repo/workflow/jbpm/NullScope.java index b6aa12a107..59117d003a 100644 --- a/source/java/org/alfresco/repo/workflow/jbpm/NullScope.java +++ b/source/java/org/alfresco/repo/workflow/jbpm/NullScope.java @@ -1,5 +1,5 @@ /* - * Copyright (C) 2005-2010 Alfresco Software Limited. + * Copyright (C) 2005-2013 Alfresco Software Limited. * * This file is part of Alfresco * @@ -37,4 +37,8 @@ public class NullScope extends NativeObject return INSTANCE; } + @Override public Object getDefaultValue(@SuppressWarnings("rawtypes") Class hint) + { + return INSTANCE.toString(); + } } diff --git a/source/test-java/org/alfresco/repo/preference/PreferenceServiceImplTest.java b/source/test-java/org/alfresco/repo/preference/PreferenceServiceImplTest.java index b04129aac0..11928edabc 100644 --- a/source/test-java/org/alfresco/repo/preference/PreferenceServiceImplTest.java +++ b/source/test-java/org/alfresco/repo/preference/PreferenceServiceImplTest.java @@ -69,7 +69,7 @@ public class PreferenceServiceImplTest // JUnit rule to initialise the default Alfresco spring configuration @ClassRule public static ApplicationContextInit APP_CONTEXT_INIT = new ApplicationContextInit(); - private static final String USERNAME2 = "userBad"; + private static final String USERNAME2 = "username2"; // Rules to create test users. Note that this class is unusual in that we do *NOT* want to reuse users across test methods. public AlfrescoPerson testUser1 = new AlfrescoPerson(APP_CONTEXT_INIT); @@ -219,7 +219,8 @@ public class PreferenceServiceImplTest { // This test is running as user1 and the JavaScript needs to know that. Map model = new HashMap(); - model.put("username", testUser1.getUsername()); + model.put("username1", testUser1.getUsername()); + model.put("username2", testUser2.getUsername()); ScriptLocation location = new ClasspathScriptLocation("org/alfresco/repo/preference/script/test_preferenceService.js"); SCRIPT_SERVICE.executeScript(location, model); diff --git a/source/test-resources/org/alfresco/repo/preference/script/test_preferenceService.js b/source/test-resources/org/alfresco/repo/preference/script/test_preferenceService.js index 55abae91fb..8504b72bc0 100644 --- a/source/test-resources/org/alfresco/repo/preference/script/test_preferenceService.js +++ b/source/test-resources/org/alfresco/repo/preference/script/test_preferenceService.js @@ -1,56 +1,109 @@ function testPreferences() { - var preferences = new Object(); - preferences.myValue = "myValue"; - preferences.comp1 = new Object(); - preferences.comp1.value1 = "value1"; - preferences.comp1.value2 = 12; - - preferenceService.setPreferences(username, preferences); - - var result = preferenceService.getPreferences(username); - - test.assertNotNull(result); - test.assertEquals("myValue", result.myValue); - test.assertEquals("value1", result.comp1.value1); - test.assertEquals(12, result.comp1.value2); - - preferences = new Object(); - preferences.comp2 = new Object(); - preferences.comp2.value1 = "value1"; - preferences.comp2.value2 = 3.142; - preferences.comp1 = new Object(); - preferences.comp1.value1 = "changed"; - preferences.comp1.value2 = 1001; - - preferenceService.setPreferences(username, preferences); - - result = preferenceService.getPreferences(username); - test.assertNotNull(result); - test.assertEquals("myValue", result.myValue); - test.assertEquals("changed", result.comp1.value1); - test.assertEquals(1001, result.comp1.value2); - test.assertEquals("value1", result.comp2.value1); - test.assertEquals(3.142, result.comp2.value2); - - preferenceService.clearPreferences(username, "comp1"); - - result = preferenceService.getPreferences(username); - test.assertNotNull(result); - test.assertEquals("myValue", result.myValue); - test.assertEquals("undefined", result.comp1); - test.assertEquals("value1", result.comp2.value1); - test.assertEquals(3.142, result.comp2.value2); - - preferenceService.clearPreferences(username); - - result = preferenceService.getPreferences(username); - test.assertNotNull(result); - test.assertEquals("undefined", result.myValue); - test.assertEquals("undefined", result.comp1); - test.assertEquals("undefined", result.comp2); - + var preferences = new Object(); + preferences.myValue = "myValue"; + preferences.comp1 = new Object(); + preferences.comp1.value1 = "value1"; + preferences.comp1.value2 = 12; + + preferenceService.setPreferences(username1, preferences); + + var result = preferenceService.getPreferences(username1); + + test.assertNotNull(result); + test.assertEquals("myValue", result.myValue); + test.assertEquals("value1", result.comp1.value1); + test.assertEquals(12, result.comp1.value2); + + preferences = new Object(); + preferences.comp2 = new Object(); + preferences.comp2.value1 = "value1"; + preferences.comp2.value2 = 3.142; + preferences.comp1 = new Object(); + preferences.comp1.value1 = "changed"; + preferences.comp1.value2 = 1001; + + preferenceService.setPreferences(username1, preferences); + + result = preferenceService.getPreferences(username1); + test.assertNotNull(result); + test.assertEquals("myValue", result.myValue); + test.assertEquals("changed", result.comp1.value1); + test.assertEquals(1001, result.comp1.value2); + test.assertEquals("value1", result.comp2.value1); + test.assertEquals(3.142, result.comp2.value2); + + preferenceService.clearPreferences(username1, "comp1"); + + result = preferenceService.getPreferences(username1); + test.assertNotNull(result); + test.assertEquals("myValue", result.myValue); + test.assertEquals("undefined", result.comp1); + test.assertEquals("value1", result.comp2.value1); + test.assertEquals(3.142, result.comp2.value2); + + preferenceService.clearPreferences(username1); + + result = preferenceService.getPreferences(username1); + test.assertNotNull(result); + test.assertEquals("undefined", result.myValue); + test.assertEquals("undefined", result.comp1); + test.assertEquals("undefined", result.comp2); } -// Execute test's -testPreferences(); \ No newline at end of file +function testGetPreferencesWithFilters_Alf20023() +{ + // Intentionally using a string rather than Date instance below as debugging shows that that is what the Java service receives. + var preferences = + { + "org.alfresco.share.sites.favourites.one" : true, + "org.alfresco.ext.sites.favourites.one.createdAt" : "2013-09-16T08:45:27.246Z" + }; + + preferenceService.setPreferences(username1, preferences); + + var result = preferenceService.getPreferences(username1); + test.assertNotNull(result, 'get preferences returned null'); + // Note that the PreferenceService will restructure some Alfresco-specific JSON keys into deep object structures. + // See CLOUD-1518 for some details on why this is. + // + // 1. Test the restructured data is there + test.assertEquals(true, result.org.alfresco.share.sites.favourites.one); + test.assertEquals("2013-09-16T08:45:27.246Z", result.org.alfresco.ext.sites.favourites.one.createdAt); + // 2. And now test that the API correctly returns data whose key triggered a restructure. + // So get by key, specifying a 'flat' key, which still returns a deep JSON object. + var favouriteSites = preferenceService.getPreferences(username1, "org.alfresco.share.sites.favourites"); + test.assertEquals(true, favouriteSites.org.alfresco.share.sites.favourites.one); + + preferences = + { + "org.alfresco.share.sites.recent._0" : "one" + }; + + preferenceService.setPreferences(username1, preferences); + result = preferenceService.getPreferences(username1); + test.assertNotNull(result, 'get preferences returned null'); + test.assertEquals("one", result.org.alfresco.share.sites.recent._0); + + // Simply getting these values is enough to ensure ALF-20023 is not regressed. + var favs = preferenceService.getPreferences(username1, 'org.alfresco.share.sites.favourites'); + var recents = preferenceService.getPreferences(username1, 'org.alfresco.share.sites.recent'); +} + +function testGettingAnotherUsersPreferencesShouldRaiseAnException() +{ + try + { + preferenceService.getPreferences(username2); + } + catch (e) + { + return; + } + test.fail("Expected exception not thrown: should not be able to access other users' preferences."); +} + +// Execute tests +testPreferences(); +testGettingAnotherUsersPreferencesShouldRaiseAnException(); +testGetPreferencesWithFilters_Alf20023(); \ No newline at end of file