On a somewhat related note, we should probably fix the constructor for SimpleGroup also, which currently looks like this:

public SimpleGroup(String id, String name, Group parentGroup)

Having both id and parentGroup parameters is redundant, so I suggest removing the id parameter (and removing the id field altogether) and instead have the getId() method return a calculated id.


On 11/08/2012 06:08 AM, Shane Bryzak wrote:
They're actually a fundamental part of the identity model (see [1]).  I have no real problem with the principle of removing the String versions of createGroup() (we would also have to do the same to createRole() for consistency) and in fact it would provide some additional advantages.  For example, being able to set a Group to being disabled at creation time, setting attribute values, etc.  My only concern is from a coding "correctness" point of view, and I guess is centered around the creation date being automatically set (or potentially overridden) on the Group instance that's passed to createGroup().  It's probably not an important concern though, and I'm happy to concede on this one which would mean we end up with the following methods (replacing all existing createGroup() and createRole() methods):

void createGroup(Group group);

void createRole(Role role);


[1] https://github.com/picketlink/picketlink/blob/master/idm/api/src/main/java/org/picketlink/idm/model/IdentityType.java

On 11/08/2012 05:37 AM, Jason Porter wrote:
Aren't those implementation details though?