Alexander Wels has posted comments on this change. Change subject: webadmin: Store rest api session id in http session ......................................................................
Patch Set 2: (7 comments) 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 HttpS Done 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 "storeInHt Done 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,retrieveFro Done 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 Good suggestion, will do 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. Done 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 I don't think we need to use an async version of the setSessionId when we have just set it (our internal variable with have the right value). My patch set will show what I mean. 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: severe in the standard logging mechanism doesn't have a Throwable parameter like the log4j framework. 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
