Vojtech Szocs has posted comments on this change. Change subject: webadmin: Store rest api session id in http session ......................................................................
Patch Set 2: (7 comments) For now, this change sounds reasonable, I have some inline comments for your consideration. In future, we will move to REST API so we might need to think of alternative storage mechanism (instead of HttpSession which requires Java servlet context). http://gerrit.ovirt.org/#/c/25987/2/frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/communication/CommunicationProvider.java File frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/communication/CommunicationProvider.java: Line 31: */ Line 32: void logout(Object userObject, UserCallback<?> callback); Line 33: Line 34: /** Line 35: * Store a {@code String} key value pair on the server side. Maybe we can mention here that given key/value pair will be stored in HttpSession in context of UI web application (WebAdmin/UserPortal) Java backend. This also means that lifetime of such key/value pairs is bound to Engine server runtime, i.e. such pairs are not persisted across Engine server restarts. On a side note, it might be useful to have similar API to manage key/value pairs which are persisted on Engine (i.e. using properties files sync'ed on each key/value update), but this is of course out of scope for this patch. Line 36: * @param key The key. Line 37: * @param value The value. Line 38: * @param callback The callback to call once the value has been stored. Line 39: */ Line 36: * @param key The key. Line 37: * @param value The value. Line 38: * @param callback The callback to call once the value has been stored. Line 39: */ Line 40: void store(String key, String value, StorageCallback callback); Based on my comment above, please consider naming this method to "storeInHttpSession" and below method to "retrieveFromHttpSession" (or "getFromHttpSession"). Line 41: Line 42: /** Line 43: * Retrieve the value associated with the key. Line 44: * @param key The key http://gerrit.ovirt.org/#/c/25987/2/frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/communication/GWTRPCCommunicationProvider.java File frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/communication/GWTRPCCommunicationProvider.java: Line 380: } Line 381: Line 382: @Override Line 383: public void store(final String key, final String value, final StorageCallback callback) { Line 384: getService().storeInHttpSession(key, value, new AsyncCallback<Void>() { It would be nice if GenericApiGWTServiceAsync methods, {storeIn,retrieveFrom,getFrom}HttpSession would be used (declared with same name) also on other classes that ultimately delegate to GenericApiGWTServiceAsync, i.e. in Frontend, CommunicationProvider, OperationProcessor, VdcOperationManager. Line 385: @Override Line 386: public void onSuccess(final Void result) { Line 387: callback.onSuccess(null); Line 388: } http://gerrit.ovirt.org/#/c/25987/2/frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/server/gwt/GenericApiGWTServiceImpl.java File frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/server/gwt/GenericApiGWTServiceImpl.java: Line 188: @Override Line 189: public void storeInHttpSession(String key, String value) { Line 190: HttpServletRequest request = this.getThreadLocalRequest(); Line 191: HttpSession session = request.getSession(); Line 192: session.setAttribute(key, value); To avoid potential clashes with other session attributes, I suggest to use some naming convention for attribute name, for example: ui_{key} Otherwise, we would allow UI to override arbitrary session attribute, even ones that are potentially used by other systems/components (not meant to be accessed by UI). Line 193: } Line 194: Line 195: @Override Line 196: public String getFromHttpSession(String key) { Line 196: public String getFromHttpSession(String key) { Line 197: HttpServletRequest request = this.getThreadLocalRequest(); Line 198: HttpSession session = request.getSession(); Line 199: Object value = session.getAttribute(key); Line 200: assert value instanceof String : "Retrieving non string value from session"; //$NON-NLS-1$ I'd prefer having log.error() message instead of assert here. This is because AFAIK we need to explicitly enable assertions in JBoss and because that in traditional Java (EE) server-side code, asserts can yield AssertionError and are therefore not really suitable for client input validation. (Java Error and subclasses indicates serious problems that a reasonable application should not try to catch.) Line 201: return (String)value; Line 202: } Line 203: Line 204: @Override http://gerrit.ovirt.org/#/c/25987/2/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/plugin/restapi/RestApiSessionManager.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/plugin/restapi/RestApiSessionManager.java: Line 161: Line 162: /** Line 163: * Attempts to reuse existing REST API session that was previously {@linkplain #acquireSession acquired}. Line 164: */ Line 165: public void reuseSession() { Question: in the normal login scenario, reuseSession is called right after setSessionId. As both reuseSession (using getSessionId) & setSessionId are async now, I think we should utilize result callback when doing setSessionId, and only after setSessionId completes (result callback executes), we should proceed with reuseSession. This complicates the current code a bit, I can help with modifications/testing here. Line 166: getSessionId(new StorageCallback() { Line 167: Line 168: @Override Line 169: public void onSuccess(String result) { Line 177: } Line 178: Line 179: @Override Line 180: public void onFailure(Throwable caught) { Line 181: RestApiSessionManager.logger.severe("Engine REST API session ID is not available"); //$NON-NLS-1$ Suggestion: RestApiSessionManager.logger.severe("Failed to retrieve Engine REST API session ID", caught); Line 182: } Line 183: }); Line 184: } Line 185: -- To view, visit http://gerrit.ovirt.org/25987 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6fe9af54054aefe694876b1805a4a44f9bba0482 Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Alexander Wels <aw...@redhat.com> Gerrit-Reviewer: Alexander Wels <aw...@redhat.com> Gerrit-Reviewer: Einav Cohen <eco...@redhat.com> Gerrit-Reviewer: Greg Sheremeta <gsher...@redhat.com> Gerrit-Reviewer: Vojtech Szocs <vsz...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
