Vojtech Szocs has posted comments on this change. Change subject: webadmin: Store rest api session id in http session ......................................................................
Patch Set 3: Code-Review+1 (10 comments) Some more inline comments for consideration. Alex, did you verify this patch with some sample UI plugin that uses "RestApiSessionAcquired" event handler function? (This function should be invoked with correct session ID value for both normal & auto login scenarios.) I can do the above mentioned UI plugin verification myself, if needed, before merging this patch. http://gerrit.ovirt.org/#/c/25987/3/frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/Frontend.java File frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/Frontend.java: Line 877: getOperationManager().setLoggedIn(false); Line 878: } Line 879: } Line 880: Line 881: public void storeValue(final String key, final String value) { Please consider using consistent names across different layers/components, i.e. this could be named "storeInHttpSession". Line 882: getOperationManager().store(key, value); Line 883: } Line 884: Line 885: public void retrieveValue(final String key, final StorageCallback callback) { Line 881: public void storeValue(final String key, final String value) { Line 882: getOperationManager().store(key, value); Line 883: } Line 884: Line 885: public void retrieveValue(final String key, final StorageCallback callback) { For consistency, this could be named "retrieveFromHttpSession". Line 886: getOperationManager().retrieve(key, callback); Line 887: } Line 888: Line 889: // TODO: Externalize to a better location, should support translation via http://gerrit.ovirt.org/#/c/25987/3/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 40: */ Line 41: void storeInHttpSession(String key, String value, StorageCallback callback); Line 42: Line 43: /** Line 44: * Retrieve the value associated with the key. ... from {@code HttpSession} on the server side. Line 45: * @param key The key Line 46: * @param the callback to call with the result. Line 47: */ Line 48: void retrieveFromHttpSession(String key, StorageCallback callback); http://gerrit.ovirt.org/#/c/25987/3/frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/communication/OperationProcessor.java File frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/communication/OperationProcessor.java: Line 271: * Default store where the caller doesn't care about the result. Line 272: * @param key The key Line 273: * @param value The value to store. Line 274: */ Line 275: public void store(final String key, final String value) { For consistency, this could be named "storeInHttpSession". Line 276: store(key, value, new StorageCallback() { Line 277: Line 278: @Override Line 279: public void onSuccess(String result) { Line 287: Line 288: }); Line 289: } Line 290: Line 291: public void store(final String key, final String value, final StorageCallback callback) { For consistency, this could be named "storeInHttpSession". Line 292: communicationProvider.storeInHttpSession(key, value, callback); Line 293: } Line 294: Line 295: public void retrieve(final String key, final StorageCallback callback) { Line 291: public void store(final String key, final String value, final StorageCallback callback) { Line 292: communicationProvider.storeInHttpSession(key, value, callback); Line 293: } Line 294: Line 295: public void retrieve(final String key, final StorageCallback callback) { For consistency, this could be named "retrieveFromHttpSession". Line 296: communicationProvider.retrieveFromHttpSession(key, callback); Line 297: } http://gerrit.ovirt.org/#/c/25987/3/frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/communication/VdcOperationManager.java File frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/communication/VdcOperationManager.java: Line 156: * Store a value on the back-end in the session. Line 157: * @param key The key. Line 158: * @param value The value. Line 159: */ Line 160: public void store(final String key, final String value) { For consistency, this could be named "storeInHttpSession". Line 161: processor.store(key, value); Line 162: } Line 163: Line 164: /** Line 165: * Retrieve a stored value from the back-end session. Line 166: * @param key The key. Line 167: * @param callback The callback to call with the value. Line 168: */ Line 169: public void retrieve(final String key, final StorageCallback callback) { For consistency, this could be named "retrieveFromHttpSession". Line 170: processor.retrieve(key, callback); Line 171: } http://gerrit.ovirt.org/#/c/25987/3/frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/gwtservices/GenericApiGWTService.java File frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/gwtservices/GenericApiGWTService.java: Line 46: public VdcReturnValueBase Login(String userName, String password, String profileName, VdcActionType loginType); Line 47: Line 48: public void storeInHttpSession(String key, String value); Line 49: Line 50: public String getFromHttpSession(String key); For consistency, this could be named "retrieveFromHttpSession". http://gerrit.ovirt.org/#/c/25987/3/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 167: //be null. If it is null then reuseSession was called from an automatic login (as restApiSessionId is null Line 168: //can we can utilize the async call to retrieve it from the backend. Line 169: if (getSessionId() != null) { Line 170: RestApiSessionAcquiredEvent.fire(eventBus, getSessionId()); Line 171: scheduleKeepAliveHeartbeat(); Hm, to avoid duplicating code, I suggest to create small methods: void processSessionId(String sessionId) { assert sessionId != null : "Session ID must not be null"; RestApiSessionAcquiredEvent.fire(eventBus, sessionId); scheduleKeepAliveHeartbeat(); } void processSessionIdError(Throwable t) { RestApiSessionManager.logger.log(Level.SEVERE, "Session ID is not available, this might break UI plugins", t); } Line 172: } else { Line 173: getSessionId(new StorageCallback() { Line 174: Line 175: @Override -- 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: 3 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
