Hi all,
some late feedback for IDM API from me.
1. On various API objects like Group, Role, GroupQuery etc. there are
many dual methods with signatures similar to:
GroupQuery setRelatedUser(User user);
GroupQuery setRelatedUser(String user);
For the former one, it may be bettter to use signature like:
GroupQuery setRelatedUser(String userId);
to emphasize that expected value is Id of user. WDYT?
2. Similar case is for Role. Instead of:
GroupQuery setRole(Role role);
GroupQuery setRole(String role);
we can have for the second method signature like:
GroupQuery setRole(String roleName);
3. On interface Group, I am not sure what's the purpose of methods:
Collection<User> getUsers(User user);
Collection<User> getUsers(String user);
Is it a mistake or am I missing something here? Instead of those, I
think it could make sense to have method for returning all users of
particular group with signature like:
Collection<User> getUsers();
Probably this method is not so usable because of losing information
about role, but I think it makes sense for some usecases.
4. On Group interface, instead of method:
Map<Role, Set<User>> getMembershipsMap();
it may be good to have possibility to obtain reverse map as well. So
having two methods instead like:
Map<Role, Set<User>> getMembershipsMapByRole();
Map<User, Set<Role>> getMembershipsMapByUser();
Same applies for interface User (and there is already TODO on User
interface for this).
5. Method: IdentityStore.getGroup(String name); should be likely
IdentityStore.getGroup(String id) I guess?
6. For Query objects, I am not sure why we have methods like:
List<User> executeQuery(UserQuery query);
and not simply:
List<User> executeQuery();
And usage of the API can simply looks like:
Collection<User> allJohnDoes =
identityManager.createQuery(User.class).setFirstName("John").setLastName("Doe").executeQuery();
7. I like the approach proposed by Jason here
https://github.com/LightGuard/incubator-deltaspike/commit/d5fc02d0493c2cfe6c46cd4ffc6c786968372d52
with the base Query superclass. But I think on the Query object should
be method:
Collection<T> executeQuery();
instead of:
Collection<T> getResults(Query<T> query);
Also not sure if method "getSingleResult" makes sense here for the Query
object.
Also if we use this approach, the method for creating query on
IdentityManager should be like:
+ <T extends IdmObject> Query<T> createQuery(Class<T> objectType);
- <T extends IdmObject> Query<T> createQuery();
I agree with Bolek for the IdmObject part. Adding IdmObject methods to
IdentityManager will reduce total number of needed methods on
IdentityManager, but it adds some unecessary complexity. I think that
adding too much generics and abstractness to the API could make it
harder to understand and use for normal users.
Marek
Dne 9.7.2012 09:32, Boleslaw Dawidowicz napsal(a):
Sorry for late reply - didn't have much time to look last week. Few comments:
1) I like the approach you took with Query and would like to improve it in
similar direction.
2) I'm not convinced that everything should extend IdmObject. The only benefit
is to reduce number of methods however in some cases it can constrain design.
3) Methods like:
<T extends IdmObject> T get(Class<T> objectType, String objectIdentifier);
<T extends IdmObject> void remove(T objectInstance);
<T extends IdmObject> T createBasic(Class<T> idmType, String identifier);
are I think good and suitable for SPI like IdentityStore. However I don't think
they are very user friendly for API that users will consume - like
IdentityManger.
Bolek
On Jul 2, 2012, at 6:53 PM, Jason Porter wrote:
Due to Apache's blanket attachment scrub policy, you can find the same info
at my github fork:
https://github.com/LightGuard/incubator-deltaspike/commit/d5fc02d0493c2cfe6c46cd4ffc6c786968372d52
On Mon, Jul 2, 2012 at 12:20 PM, Jason Porter<[email protected]>wrote:
Attached is my diff from what I had pulled down as of Wednesday or so.
Feel free to discuss, dissect, etc
On Sun, Jul 1, 2012 at 12:49 PM, Jason Porter<[email protected]>wrote:
I have some stuff I did on the plane from Summit. I'm going to go back
over it tomorrow and see if I included everything I wanted. I'll send the
diff to the list.
Sent from my iPhone
On Jul 1, 2012, at 9:43, Boleslaw Dawidowicz<
[email protected]> wrote:
I think it all comes down (again) to use cases [0] which we think this
API should address. Current ones are mainly around typical web application
development and proposed API prototype was focusing on easy usage. My main
fear is to go end up with too abstract and generic design that won't be
really appealing to most of developers. If you look at use cases addressed
by JSR 351 [1] it is all about dealing with attributes and scenarios closer
to healthcare - and this so far results in quite different API design. API
which I believe will be less appealing to typical web developer who doesn't
care about such use cases.
More inline below
[0]
https://cwiki.apache.org/confluence/display/DeltaSpike/Security+Module+Drafts
[1] http://java.net/projects/identity-api-spec/pages/UseCases
On Jun 29, 2012, at 4:56 PM, Jason Porter wrote:
== Security IDM prototype feedback ==
* Fold PermissionQuery into PermissionManager to give the developers a
similar model as JPA.
* Because a Group is an IdentityType, I wonder if it would be better to
just have one set of methods and pass in the IdentityType sub
interface you
would like to receive back
I guess thats a bit matter of taste. We could prepare quick prototype
and compare which one looks more useful to majority of folks here.
* I feel the methods on UserQuery are restricting how the IDM can be
used.
We can't always guarantee that there will be a first name, last name,
email, etc. However, being completely open and free form like what was
done
in the PicketLink IDM is too much. Perhaps we can find some sort of
balance
or meta model similar to JPA?
* Maybe we could have a base no method interface for things and fold
all
the Query and Manager objects into one (similar to above about JPA)
* We probably don't want to go down the route of create a query
language,
but we almost have a DSL with a fluent API here. Maybe we should think
about about JPA the names out a bit more and actually create a fluent
API
for the Query objects
Would you have time to prepare some simple prototype like this to
discuss? I'm not sure how much generic design are you proposing in fact.
Just some bare code snippets would do to back your suggestions with some
example usage.
UserQuery is not something I'm perfectly satisfied with however it is
still an attempt to have something similar to JPA Criteria Query - which is
very intuitive and easy to use IMO. I would love to see more feedback from
others on how to improve this part in fact.
* I believe we should remove some of the exception constructors and
make a
message required (this would probably be good to have in all of
DeltaSpike). Adding a cause is great, but being able to create an
exception
without a message is less helpful for everyone.
Good point. I think this part is simply not really polished yet.
* Is there a point for having addObject and addObjects type methods?
Would
one that's simply a varag be enough? We could check the type that
comes in
if thy send us a collection, or simply have varags and collection
addObjects methods.
Sounds good to me. Again this is probably more matter of taste and
consistency with other APIs in the project. Maybe there should be some
general project wide design guideline for choices like this.
--
Jason Porter
http://lightguard-jp.blogspot.com
http://twitter.com/lightguardjp
Software Engineer
Open Source Advocate
Author of Seam Catch - Next Generation Java Exception Handling
PGP key id: 926CCFF5
PGP key available at: keyserver.net, pgp.mit.edu
--
Jason Porter
http://lightguard-jp.blogspot.com
http://twitter.com/lightguardjp
Software Engineer
Open Source Advocate
Author of Seam Catch - Next Generation Java Exception Handling
PGP key id: 926CCFF5
PGP key available at: keyserver.net, pgp.mit.edu
--
Jason Porter
http://lightguard-jp.blogspot.com
http://twitter.com/lightguardjp
Software Engineer
Open Source Advocate
Author of Seam Catch - Next Generation Java Exception Handling
PGP key id: 926CCFF5
PGP key available at: keyserver.net, pgp.mit.edu
