Vojtech Szocs has uploaded a new change for review.
Change subject: webadmin,userportal: ClientStorage improvements
......................................................................
webadmin,userportal: ClientStorage improvements
1. All key/value pairs persisted through ClientStorage interface
now use application-specific key prefix to avoid clashes between
WebAdmin vs. UserPortal instances running on the same browser
(but not necessarily at the same time) where one instance might
read data originally persisted by another instance.
Example key names:
ENGINE_WebAdmin_MyKey
ENGINE_UserPortal_MyKey
2. ClientStorageImpl behavior modified as following:
* get{Local,Session}Item -- first use prefixed key, if missing
fall back to un-prefixed key for backwards compatibility
* set{Local,Session}Item -- use prefixed key (only)
3. Removed unused ClientStorage interface methods:
* removeLocalItem
* removeSessionItem
4. Added ClientStorageImplTest.
5. In UserPortal SystemModule, removed unnecessary GIN bindings.
Change-Id: Iaef8f72feec5dd083fbe4cc962a222dce4f7a99f
Signed-off-by: Vojtech Szocs <[email protected]>
---
M
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/system/ClientStorage.java
M
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/system/ClientStorageImpl.java
A
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/system/ClientStorageKeyPrefix.java
A
frontend/webadmin/modules/gwt-common/src/test/java/org/ovirt/engine/ui/common/system/ClientStorageImplTest.java
M
frontend/webadmin/modules/userportal-gwtp/src/main/java/org/ovirt/engine/ui/userportal/gin/SystemModule.java
M
frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/gin/SystemModule.java
6 files changed, 193 insertions(+), 37 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/04/41404/1
diff --git
a/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/system/ClientStorage.java
b/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/system/ClientStorage.java
index e4a91ab..0287596 100644
---
a/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/system/ClientStorage.java
+++
b/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/system/ClientStorage.java
@@ -16,13 +16,14 @@
public interface ClientStorage {
/**
- * Returns {@code true} if the browser supports HTML5 Web Storage (both
local storage and session storage APIs).
+ * Returns {@code true} if the browser supports HTML5 Web Storage
+ * (both local storage and session storage APIs).
*/
boolean isWebStorageAvailable();
/**
- * Returns the value for the given key from local (persistent) storage, or
{@code null} if there is no value for
- * such key.
+ * Returns the value for the given key from local (persistent) storage,
+ * or {@code null} if there is no value for such key.
*/
String getLocalItem(String key);
@@ -32,13 +33,8 @@
void setLocalItem(String key, String value);
/**
- * Removes the value associated with the given key from local (persistent)
storage.
- */
- void removeLocalItem(String key);
-
- /**
- * Returns the value for the given key from session (transient) storage,
or {@code null} if there is no value for
- * such key.
+ * Returns the value for the given key from session (transient) storage,
+ * or {@code null} if there is no value for such key.
*/
String getSessionItem(String key);
@@ -46,10 +42,5 @@
* Sets the value for the given key using session (transient) storage.
*/
void setSessionItem(String key, String value);
-
- /**
- * Removes the value associated with the given key from session
(transient) storage.
- */
- void removeSessionItem(String key);
}
diff --git
a/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/system/ClientStorageImpl.java
b/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/system/ClientStorageImpl.java
index b30b044..e75438e 100644
---
a/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/system/ClientStorageImpl.java
+++
b/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/system/ClientStorageImpl.java
@@ -4,24 +4,51 @@
import com.google.gwt.storage.client.Storage;
import com.google.gwt.user.client.Cookies;
+import com.google.inject.Inject;
/**
* Default implementation of {@link ClientStorage} interface.
+ * <p>
+ * Applies an application-specific {@linkplain ClientStorageKeyPrefix prefix}
to all key names.
*/
public class ClientStorageImpl implements ClientStorage {
// Fifty years, in milliseconds
private static final long PERSISTENT_COOKIE_EXPIRATION = 50L * 365L * 24L
* 60L * 60L * 1000L;
- private static final Storage localStorage =
Storage.getLocalStorageIfSupported();
- private static final Storage sessionStorage =
Storage.getSessionStorageIfSupported();
+ private static Storage localStorage;
+ private static Storage sessionStorage;
+
+ private final String keyPrefix;
+
+ @Inject
+ public ClientStorageImpl(@ClientStorageKeyPrefix String keyPrefix) {
+ this.keyPrefix = keyPrefix;
+ initStorage();
+ }
+
+ void initStorage() {
+ localStorage = Storage.getLocalStorageIfSupported();
+ sessionStorage = Storage.getSessionStorageIfSupported();
+ }
+
+ String getPrefixedKey(String key) {
+ return keyPrefix + "_" + key; //$NON-NLS-1$
+ }
@Override
public boolean isWebStorageAvailable() {
return localStorage != null && sessionStorage != null;
}
+ @Override
public String getLocalItem(String key) {
+ String value = getLocalItemImpl(getPrefixedKey(key));
+ // If missing, use un-prefixed key for backwards compatibility
+ return (value != null) ? value : getLocalItemImpl(key);
+ }
+
+ String getLocalItemImpl(String key) {
if (localStorage != null) {
return localStorage.getItem(key);
} else {
@@ -29,7 +56,12 @@
}
}
+ @Override
public void setLocalItem(String key, String value) {
+ setLocalItemImpl(getPrefixedKey(key), value);
+ }
+
+ void setLocalItemImpl(String key, String value) {
if (localStorage != null) {
localStorage.setItem(key, value);
} else {
@@ -38,15 +70,14 @@
}
}
- public void removeLocalItem(String key) {
- if (localStorage != null) {
- localStorage.removeItem(key);
- } else {
- Cookies.removeCookie(key);
- }
+ @Override
+ public String getSessionItem(String key) {
+ String value = getSessionItemImpl(getPrefixedKey(key));
+ // If missing, use un-prefixed key for backwards compatibility
+ return (value != null) ? value : getSessionItemImpl(key);
}
- public String getSessionItem(String key) {
+ String getSessionItemImpl(String key) {
if (sessionStorage != null) {
return sessionStorage.getItem(key);
} else {
@@ -54,20 +85,17 @@
}
}
+ @Override
public void setSessionItem(String key, String value) {
+ setSessionItemImpl(getPrefixedKey(key), value);
+ }
+
+ void setSessionItemImpl(String key, String value) {
if (sessionStorage != null) {
sessionStorage.setItem(key, value);
} else {
// Emulate transient storage using cookies which expire when the
browser session ends
Cookies.setCookie(key, value);
- }
- }
-
- public void removeSessionItem(String key) {
- if (sessionStorage != null) {
- sessionStorage.removeItem(key);
- } else {
- Cookies.removeCookie(key);
}
}
diff --git
a/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/system/ClientStorageKeyPrefix.java
b/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/system/ClientStorageKeyPrefix.java
new file mode 100644
index 0000000..6b90301
--- /dev/null
+++
b/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/system/ClientStorageKeyPrefix.java
@@ -0,0 +1,23 @@
+package org.ovirt.engine.ui.common.system;
+
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+import com.google.inject.BindingAnnotation;
+
+/**
+ * Binding annotation for String constant representing an application-specific
prefix
+ * applied to all {@link ClientStorage} key names.
+ * <p>
+ * Such prefix exists to avoid clashes between WebAdmin vs. UserPortal
instances running
+ * on the same browser (but not necessarily at the same time) where one
instance might
+ * read data originally persisted by another instance.
+ */
+@BindingAnnotation
+@Target({ ElementType.FIELD, ElementType.PARAMETER, ElementType.METHOD })
+@Retention(RetentionPolicy.RUNTIME)
+public @interface ClientStorageKeyPrefix {
+
+}
diff --git
a/frontend/webadmin/modules/gwt-common/src/test/java/org/ovirt/engine/ui/common/system/ClientStorageImplTest.java
b/frontend/webadmin/modules/gwt-common/src/test/java/org/ovirt/engine/ui/common/system/ClientStorageImplTest.java
new file mode 100644
index 0000000..8605378
--- /dev/null
+++
b/frontend/webadmin/modules/gwt-common/src/test/java/org/ovirt/engine/ui/common/system/ClientStorageImplTest.java
@@ -0,0 +1,108 @@
+package org.ovirt.engine.ui.common.system;
+
+import static org.hamcrest.CoreMatchers.equalTo;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertThat;
+import static org.mockito.Matchers.any;
+import static org.mockito.Mockito.doNothing;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.verify;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.runners.MockitoJUnitRunner;
+
+@RunWith(MockitoJUnitRunner.class)
+public class ClientStorageImplTest {
+
+ private static final String KEY_PREFIX = "TestPrefix"; //$NON-NLS-1$
+
+ private ClientStorageImpl tested;
+
+ @Before
+ public void setUp() {
+ tested = spy(new ClientStorageImpl(KEY_PREFIX) {
+ @Override
+ void initStorage() {
+ // No-op to avoid GWT.create() calls
+ }
+ });
+
+ // Stub un-testable methods, specific tests should re-stub these as
needed
+ doReturn(true).when(tested).isWebStorageAvailable();
+ doReturn(null).when(tested).getLocalItemImpl(any(String.class));
+ doNothing().when(tested).setLocalItemImpl(any(String.class),
any(String.class));
+ doReturn(null).when(tested).getSessionItemImpl(any(String.class));
+ doNothing().when(tested).setSessionItemImpl(any(String.class),
any(String.class));
+ }
+
+ /**
+ * Verify that prefix is applied to given key.
+ */
+ @Test
+ public void getPrefixedKey() {
+ String prefixedKey = tested.getPrefixedKey("Key"); //$NON-NLS-1$
+ assertThat(prefixedKey, equalTo(KEY_PREFIX + "_Key")); //$NON-NLS-1$
+ }
+
+ /**
+ * When prefixed key exists, return its value.
+ */
+ @Test
+ public void getItem_prefixedKeyExists() {
+ doReturn("LocalValue").when(tested).getLocalItemImpl(KEY_PREFIX +
"_LocalKey"); //$NON-NLS-1$ //$NON-NLS-2$
+ String localValue = tested.getLocalItem("LocalKey"); //$NON-NLS-1$
+ assertThat(localValue, equalTo("LocalValue")); //$NON-NLS-1$
+
+ doReturn("SessionValue").when(tested).getSessionItemImpl(KEY_PREFIX +
"_SessionKey"); //$NON-NLS-1$ //$NON-NLS-2$
+ String sessionValue = tested.getSessionItem("SessionKey");
//$NON-NLS-1$
+ assertThat(sessionValue, equalTo("SessionValue")); //$NON-NLS-1$
+ }
+
+ /**
+ * When prefixed key is missing but un-prefixed key exists, return value
of un-prefixed key.
+ */
+ @Test
+ public void getItem_prefixedKeyMissing_unPrefixedKeyExists() {
+ doReturn(null).when(tested).getLocalItemImpl(KEY_PREFIX +
"_LocalKey"); //$NON-NLS-1$
+ doReturn("LocalValue").when(tested).getLocalItemImpl("LocalKey");
//$NON-NLS-1$ //$NON-NLS-2$
+ String localValue = tested.getLocalItem("LocalKey"); //$NON-NLS-1$
+ assertThat(localValue, equalTo("LocalValue")); //$NON-NLS-1$
+
+ doReturn(null).when(tested).getSessionItemImpl(KEY_PREFIX +
"_SessionKey"); //$NON-NLS-1$
+
doReturn("SessionValue").when(tested).getSessionItemImpl("SessionKey");
//$NON-NLS-1$ //$NON-NLS-2$
+ String sessionValue = tested.getSessionItem("SessionKey");
//$NON-NLS-1$
+ assertThat(sessionValue, equalTo("SessionValue")); //$NON-NLS-1$
+ }
+
+ /**
+ * When both prefixed and un-prefixed keys are missing, return null.
+ */
+ @Test
+ public void getItem_prefixedKeyMissing_unPrefixedKeyMissing() {
+ doReturn(null).when(tested).getLocalItemImpl(KEY_PREFIX +
"_LocalKey"); //$NON-NLS-1$
+ doReturn(null).when(tested).getLocalItemImpl("LocalKey"); //$NON-NLS-1$
+ String localValue = tested.getLocalItem("LocalKey"); //$NON-NLS-1$
+ assertNull(localValue);
+
+ doReturn(null).when(tested).getSessionItemImpl(KEY_PREFIX +
"_SessionKey"); //$NON-NLS-1$
+ doReturn(null).when(tested).getSessionItemImpl("SessionKey");
//$NON-NLS-1$
+ String sessionValue = tested.getSessionItem("SessionKey");
//$NON-NLS-1$
+ assertNull(sessionValue);
+ }
+
+ /**
+ * Verify that prefix is applied to given key when setting an item.
+ */
+ @Test
+ public void setItem() {
+ tested.setLocalItem("LocalKey", "LocalValue"); //$NON-NLS-1$
//$NON-NLS-2$
+ verify(tested).setLocalItemImpl(KEY_PREFIX + "_LocalKey",
"LocalValue"); //$NON-NLS-1$ //$NON-NLS-2$
+
+ tested.setSessionItem("SessionKey", "SessionValue"); //$NON-NLS-1$
//$NON-NLS-2$
+ verify(tested).setSessionItemImpl(KEY_PREFIX + "_SessionKey",
"SessionValue"); //$NON-NLS-1$ //$NON-NLS-2$
+ }
+
+}
diff --git
a/frontend/webadmin/modules/userportal-gwtp/src/main/java/org/ovirt/engine/ui/userportal/gin/SystemModule.java
b/frontend/webadmin/modules/userportal-gwtp/src/main/java/org/ovirt/engine/ui/userportal/gin/SystemModule.java
index b8552fa..dce8eae 100644
---
a/frontend/webadmin/modules/userportal-gwtp/src/main/java/org/ovirt/engine/ui/userportal/gin/SystemModule.java
+++
b/frontend/webadmin/modules/userportal-gwtp/src/main/java/org/ovirt/engine/ui/userportal/gin/SystemModule.java
@@ -3,6 +3,7 @@
import org.ovirt.engine.ui.common.gin.BaseSystemModule;
import org.ovirt.engine.ui.common.section.DefaultLoginSectionPlace;
import org.ovirt.engine.ui.common.section.DefaultMainSectionPlace;
+import org.ovirt.engine.ui.common.system.ClientStorageKeyPrefix;
import org.ovirt.engine.ui.uicommonweb.auth.CurrentUserRole;
import org.ovirt.engine.ui.uicommonweb.place.UserPortalApplicationPlaces;
import org.ovirt.engine.ui.userportal.ApplicationConstants;
@@ -11,19 +12,19 @@
import org.ovirt.engine.ui.userportal.ApplicationResources;
import org.ovirt.engine.ui.userportal.ApplicationResourcesWithLookup;
import org.ovirt.engine.ui.userportal.ApplicationTemplates;
-import org.ovirt.engine.ui.userportal.auth.UserPortalCurrentUserRole;
import org.ovirt.engine.ui.userportal.auth.LoggedInExtendedPlaceGatekeeper;
+import org.ovirt.engine.ui.userportal.auth.UserPortalCurrentUserRole;
import org.ovirt.engine.ui.userportal.place.UserPortalPlaceManager;
import org.ovirt.engine.ui.userportal.section.DefaultMainSectionExtendedPlace;
import com.google.inject.Singleton;
-import org.ovirt.engine.ui.userportal.system.ApplicationInit;
-
/**
* GIN module containing UserPortal infrastructure and configuration bindings.
*/
public class SystemModule extends BaseSystemModule {
+
+ private static final String CLIENT_STORAGE_KEY_PREFIX =
"ENGINE_UserPortal"; //$NON-NLS-1$
@SuppressWarnings("deprecation")
@Override
@@ -37,8 +38,6 @@
void bindInfrastructure() {
bindCommonInfrastructure(UserPortalPlaceManager.class);
bind(LoggedInExtendedPlaceGatekeeper.class).in(Singleton.class);
- bind(CurrentUserRole.class).in(Singleton.class);
- bind(ApplicationInit.class).asEagerSingleton();
bindTypeAndImplAsSingleton(CurrentUserRole.class,
UserPortalCurrentUserRole.class);
}
@@ -49,6 +48,8 @@
.to(UserPortalApplicationPlaces.DEFAULT_MAIN_SECTION_BASIC_PLACE);
bindConstant().annotatedWith(DefaultMainSectionExtendedPlace.class)
.to(UserPortalApplicationPlaces.DEFAULT_MAIN_SECTION_EXTENDED_PLACE);
+ bindConstant().annotatedWith(ClientStorageKeyPrefix.class)
+ .to(CLIENT_STORAGE_KEY_PREFIX);
bindResourceConfiguration(ApplicationConstants.class,
ApplicationMessages.class,
ApplicationResources.class, ApplicationTemplates.class,
ApplicationDynamicMessages.class);
diff --git
a/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/gin/SystemModule.java
b/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/gin/SystemModule.java
index a1aceb7..77892a7 100644
---
a/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/gin/SystemModule.java
+++
b/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/gin/SystemModule.java
@@ -4,6 +4,7 @@
import org.ovirt.engine.ui.common.gin.BaseSystemModule;
import org.ovirt.engine.ui.common.section.DefaultLoginSectionPlace;
import org.ovirt.engine.ui.common.section.DefaultMainSectionPlace;
+import org.ovirt.engine.ui.common.system.ClientStorageKeyPrefix;
import org.ovirt.engine.ui.uicommonweb.auth.CurrentUserRole;
import org.ovirt.engine.ui.uicommonweb.place.WebAdminApplicationPlaces;
import org.ovirt.engine.ui.webadmin.ApplicationConstants;
@@ -19,6 +20,8 @@
* GIN module containing WebAdmin infrastructure and configuration bindings.
*/
public class SystemModule extends BaseSystemModule {
+
+ private static final String CLIENT_STORAGE_KEY_PREFIX = "ENGINE_WebAdmin";
//$NON-NLS-1$
@SuppressWarnings("deprecation")
@Override
@@ -41,6 +44,8 @@
.to(WebAdminApplicationPlaces.DEFAULT_LOGIN_SECTION_PLACE);
bindConstant().annotatedWith(DefaultMainSectionPlace.class)
.to(WebAdminApplicationPlaces.DEFAULT_MAIN_SECTION_PLACE);
+ bindConstant().annotatedWith(ClientStorageKeyPrefix.class)
+ .to(CLIENT_STORAGE_KEY_PREFIX);
bindResourceConfiguration(ApplicationConstants.class,
ApplicationMessages.class,
ApplicationResources.class, ApplicationTemplates.class,
ApplicationDynamicMessages.class);
--
To view, visit https://gerrit.ovirt.org/41404
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iaef8f72feec5dd083fbe4cc962a222dce4f7a99f
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Vojtech Szocs <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
