Alon Bar-Lev has posted comments on this change. Change subject: aaa : Add engine sso ......................................................................
Patch Set 49: (5 comments) https://gerrit.ovirt.org/#/c/36119/49/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/SSOQueryServlet.java File backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/SSOQueryServlet.java: Line 49: if (!SSOUtils.versionSupported(version)) { Line 50: throw new Exception("Unsupported version found in request."); Line 51: } Line 52: } Line 53: switch (command) { can't we have some kind of map between command an reference to method? no need for reflection... if you put all methods in Runnable anonymous class you can access the request and response without passing as parameter if you hold them as final, right? so it ends to be as: Map<String, Runnable> map = new HashMap<>(); map.add("authn-credentials", new Runnable() { @Override void run() { authnCredentials() } }); Runnable r = map.get(command); if (r == null) { // handle error; } else { r.run(); } Line 54: case AUTHN_CREDENTIALS: Line 55: authCredentials(request, response); Line 56: break; Line 57: case GET_TOKEN_PAYLOAD: Line 51: } Line 52: } Line 53: switch (command) { Line 54: case AUTHN_CREDENTIALS: Line 55: authCredentials(request, response); please align method name to constant. Line 56: break; Line 57: case GET_TOKEN_PAYLOAD: Line 58: getTokenPayload(request, response); Line 59: break; Line 57: case GET_TOKEN_PAYLOAD: Line 58: getTokenPayload(request, response); Line 59: break; Line 60: case INVALIDATE_SESSION: Line 61: HttpSession existingSession = request.getSession(false); not sure how invalidate does not take sso token. Line 62: if (existingSession != null) { Line 63: existingSession.invalidate(); Line 64: } Line 65: break; Line 60: case INVALIDATE_SESSION: Line 61: HttpSession existingSession = request.getSession(false); Line 62: if (existingSession != null) { Line 63: existingSession.invalidate(); Line 64: } please have a method as well. Line 65: break; Line 66: case TOKEN_QUERY: Line 67: getTicketUser(request, response); Line 68: break; Line 63: existingSession.invalidate(); Line 64: } Line 65: break; Line 66: case TOKEN_QUERY: Line 67: getTicketUser(request, response); please align method names with constants. Line 68: break; Line 69: case VALIDATE_SESSION: Line 70: validateSession(request, response); Line 71: break; -- To view, visit https://gerrit.ovirt.org/36119 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4894fc12653027271b6abd4dd5313b10593703fa Gerrit-PatchSet: 49 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ravi Nori <rn...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: Ravi Nori <rn...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
