[Engine-patches] Change in ovirt-engine[master]: aaa: using the new extensions API in InternalDirectory

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: aaa: using the new extensions API in InternalDirectory
......................................................................


Patch Set 4:

(6 comments)

http://gerrit.ovirt.org/#/c/26477/4/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/DirectoryProxy.java
File 
backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/DirectoryProxy.java:

Line 28:     @Override
Line 29:     public DirectoryUser findUser(String name) {
Line 30:         ExtMap inputMap = new ExtMap().mput(
Line 31:                 Base.InvokeKeys.COMMAND, 
Authz.InvokeCommands.FETCH_PRINCIPAL_RECORD
Line 32:                 ).mput(Authn.InvokeKeys.AUTH_RECORD, new 
ExtMap().mput(Authn.AuthRecord.PRINCIPAL, name));
can we try to indent our code in decent way?

I expect one indent per block.

in this case:

 ExtMap inputMap = new ExtMap().mput(
     Base.InvokeKeys.COMMAND, 
     Authz.InvokeCommands.FETCH_PRINCIPAL_RECORD
 ).mput(
     Authn.InvokeKeys.AUTH_RECORD,
     new ExtMap().mput(
         Authn.AuthRecord.PRINCIPAL,
         name
    )
 );

it makes the eye find each block/parameter with ease, as each line contain 
single expression and indent is nesting level.

please find a way to do similar in your auto formatting ide
Line 33:         ExtMap outputMap = extension.invoke(inputMap);
Line 34:         ExtMap principalRecord = 
outputMap.<ExtMap>get(Authz.InvokeKeys.PRINCIPAL_RECORD);
Line 35:         if (principalRecord == null) {
Line 36:             return null;


Line 59:             return populateUsers(inputMap);
Line 60:         } finally {
Line 61:             extension.invoke(new ExtMap().mput(
Line 62:                     Base.InvokeKeys.COMMAND, 
Authz.InvokeCommands.QUERY_CLOSE
Line 63:                     ).mput(Authz.InvokeKeys.QUERY_OPAQUE, 
queryOpaque));
if you do not invoke the open here, you should not invoke the close.

please try to be symmetric... close should be only when opened.
Line 64:         }
Line 65:     }
Line 66: 
Line 67:     @Deprecated


Line 87:             return populateUsers(outputMap);
Line 88:         } finally {
Line 89:             extension.invoke(new ExtMap().mput(
Line 90:                     Base.InvokeKeys.COMMAND, 
Authz.InvokeCommands.QUERY_CLOSE
Line 91:                     ).mput(Authz.InvokeKeys.QUERY_OPAQUE, 
queryOpaque));
this leads to multiple close...
Line 92:         }
Line 93:     }
Line 94: 
Line 95:     private List<DirectoryUser> populateUsers(ExtMap inputMap) {


Line 98:         queryOpaque = outputMap.get(Authz.InvokeKeys.QUERY_OPAQUE);
Line 99:         while (true) {
Line 100:             inputMap = new ExtMap().mput(
Line 101:                     Base.InvokeKeys.COMMAND, 
Authz.InvokeCommands.QUERY_EXECUTE
Line 102:                     ).mput(Authz.InvokeKeys.QUERY_OPAQUE, 
queryOpaque);
no need for inputMap temp variable.... please notice that you change input 
parameter which is bad.
Line 103:             outputMap = extension.invoke(inputMap);
Line 104:             ExtMap result = 
outputMap.get(Authz.InvokeKeys.QUERY_RESULT);
Line 105:             if (result == null) {
Line 106:                 break;


Line 109:         }
Line 110:         inputMap = new ExtMap().mput(
Line 111:                 Base.InvokeKeys.COMMAND, 
Authz.InvokeCommands.QUERY_CLOSE
Line 112:                 ).mput(Authz.InvokeKeys.QUERY_OPAQUE, queryOpaque);
Line 113:         outputMap = extension.invoke(inputMap);
no need for inputMap temp variable.
Line 114:         return directoryUsers;
Line 115:     }
Line 116: 
Line 117:     private List<DirectoryGroup> populateGroups(ExtMap inputMap) {


Line 135:                 Base.InvokeKeys.COMMAND, 
Authz.InvokeCommands.QUERY_CLOSE
Line 136:                 ).mput(Authz.InvokeKeys.QUERY_OPAQUE, queryOpaque);
Line 137:         outputMap = extension.invoke(inputMap);
Line 138:         return directoryGroups;
Line 139:     }
why not have query with callback? something like: but in the java way... of 
useless interface.

 query(input, callback) {
    invoke()
    get opaque
    while(true) {
        output = invoke()
        if (!callback(output)) {
           break;
        }
 }

 final List<DirectoryGroup> directoryGroups = new ArrayList<>();
 query(
         new ExtMap().mput(
             Base.InvokeKeys.COMMAND,
             Authz.InvokeCommands.QUERY_PRINCIPALS_OPEN
         ).mput(
             Authz.InvokeKeys.QUERY,
             query
         )
     ),
     new QueryResults() {
         public boolean process(ExtMap output) {
             if (output.result == null) {
                 return false;
             }
             for (r : output.results) {
                 directoryGroups.add(r);
             }
             return true;
         }
     }
 );
Line 140: 
Line 141:     @Deprecated
Line 142:     @Override
Line 143:     public List<DirectoryGroup> queryGroups(String query) {


-- 
To view, visit http://gerrit.ovirt.org/26477
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I958443292da0455e0a12039fac98eebb9b17dee2
Gerrit-PatchSet: 4
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Yair Zaslavsky <yzasl...@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com>
Gerrit-Reviewer: Yair Zaslavsky <yzasl...@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