Alon Bar-Lev has posted comments on this change.
Change subject: [WIP] Introduce new directory interface
......................................................................
Patch Set 1: (6 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/directory/DirectoryManager.java
Line 55: /**
Line 56: * Loads the directory implementations that are available in the
class path
Line 57: * using the service loader mechanism.
Line 58: */
Line 59: private void loadImplementations() {
Can we have these in separate directory and not within classpath? it is easier
to maintain that externally.
Line 60: ServiceLoader<DirectorySpi> loader =
Line 61: ServiceLoader.load(DirectorySpi.class);
Line 62: for (DirectorySpi implementation : loader) {
Line 63: implementations.add(implementation);
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/users/DirectoryEntry.java
Line 21: this.id = id;
Line 22: this.name = name;
Line 23: }
Line 24:
Line 25: public Guid getId() {
Please don't assume Guid
Line 26: return id;
Line 27: }
Line 28:
Line 29: public void setId(Guid id) {
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/users/Directory.java
Line 26: * @param name the name of the user
Line 27: * @return the user corresponding to the given name or
Line 28: * <code>null</code> if no such user can be found
Line 29: */
Line 30: DirectoryUser findUserByName(String name);
How can name be unique? we should always be prepared mutli-value result out of
queries... or you internally will raise exception if we have multiple results?
I would have put findUserByAttribute(attribute, value) instead the above, to
allow multiple simple queries without modifying the interface.
Line 31:
Line 32: /**
Line 33: * Retrieves an user from the directory given its identifier. Note
that this
Line 34: * identifier is the one assigned by the engine to the user, not
the one
Line 50: * with no particular order, note that the returned set may
contain less
Line 51: * elements than the given set of identifiers and that it may be
in a
Line 52: * different order
Line 53: */
Line 54: Set<DirectoryUser> findUsersById(Set<Guid> ids);
I like the separation.
As you may have findUserById which accept String as well.
What I don't think is correct is the assumption of Guid here... this is true
for active directory but not to other implementation.
I think id should be generic string of type:content, so it can be uuid:<uuid>
for active directory, and str:<string> for others.
Line 55:
Line 56: /**
Line 57: * Retrieves a group from the directory given its identifier. Note
that this
Line 58: * identifier is the one assigned by the engine to the group, not
the one
Line 80: *
Line 81: * @param query the query
Line 82: * @return a list containing the groups that match the given query
Line 83: */
Line 84: Set<DirectoryGroup> findGroupsByQuery(String query);
what is the query format?
Usually I expect:
1. Locate user by name -> among others returns unique id attribute.
2. Fetch user groups by user unique id
3. Fetch group groups by group id
For present:
4. Fetch groups.
Important to note that we do not want to / forced to replicate the entire
directory into our database.
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/users/DirectoryUser.java
Line 15: private String firstName;
Line 16: private String lastName;
Line 17: private String title;
Line 18: private String email;
Line 19: private String department;
I would have put plain getAttribute(attribute) and hold these in map, so that
when adding new attributes we won't change the interface, and UI can enum the
attributes and present the object.
Line 20:
Line 21: // Flag indicating if this user has the administrator role:
Line 22: private boolean isAdmin = false;
Line 23:
--
To view, visit http://gerrit.ovirt.org/15596
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If84a0c9d6553d81cdbbe224972696f169cca90d4
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Juan Hernandez <juan.hernan...@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com>
Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com>
Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
