On 04/02/2013 14:00, Christian Schneider wrote:
On 04.02.2013 12:41, Francesco Chicchiriccò wrote:
On 04/02/2013 12:12, Jan Bernhardt wrote:
The workflow-related methods in UserController (and RoleController BTW)
are there because they are involved in user (and role) lifecycle
management.
Given these things, I wouldn't make any movement.
OK, it still doesn't feel right to me, but of course we still can
make refactoring's at a later time, when we all feel a greater need
to do so.
It is not matter of feeling, it is just incorrect: UserController and
RoleController make usage of workflow adapters to perform user and
role operations while the WorkflowController manipulates the workflow
definition, used to initialize and configure the workflow adapters.
I agree with you that WorkflowController and the workflow methods in
UserController do not belong together. On the other hand I think it may
be a good idea to separate them from the UserService. So would it make
sense to have a UserWorkflowService? This is supported by the fact that
we already have a separate ApprovalRestClient in the client module.
I am still not completely convinced of the rationale for creating such
new UserWorkflowService (and RoleWorkflowService, please don't forget
that also roles are workflow-enabled).
We will end up with UserService / RoleService for read-only operations
and UserWorkflowService / RoleWorkflowService for write-only operations:
does it make sense?
2. There are quite some many methods capable of handling username and
userId likewise (suspendByUsername, deleteByUsername). As far as I can
see console does not use any of them (*byUsername). From my point of
view, these methods are redundant (except one), since you can always
get
user id by readUser(String username) operation. My proposal would be to
not support these methods in new interface any longer.
The username-based methods were introduced for making more user-
friendly the invocation of user methods, even from command line
using tools
like curl; the admin console was built before that, and hence it's
using the
former id-based methods.
I agree that username-based are redundant, but they were introduced to
increase the ease of use, not the efficiency.
IMHO I would not add so many convenience functions to core interface,
just for tools like curl. Any adaptor of syncope should rather use
IDs than (temporary) names. I think these kind of convenience
functions would belong to a rich syncope client [SYNCOPE-150], rather
than core interface.
One problem with username is, that it makes sub-resource access more
difficult or increases potential path collisions with other resources.
Of course we can keep these methods as they currently are, but I
would still like to remove them.
Who else has an opinion on this matter?
Adding some bits to this: SYNCOPE-42 and SYNCOPE-91 introduced in
1.0.0-incubating such methods that were absent before.
Anyway, I am +-0 to remove such methods in 1.2.0.
Perhaps these convenience methods show that it might be better to have
the user name as a key in the rest service instead of the id.
I am not really sure about this though. When talking with Jan about it
he argued that a user name can change over time.
Correct: if you take a look at SyncopeUser's source code, username is
just a value with unique constraint.
On the other hand I think the user id we currently have is an internal
thing of our database and may better be hidden to the outside.
Why make this distinction for SyncopeUser and not for any other entity?
We "expose" internal ids for everything.
One other thing we could consider is using a redirect from the user
based rest resource to the id based resource. So when you do a get on
users/readByUsername/{username} we could redirect to users/{username}.
For the status changing methods like users/activate/{userid} we could
instead use: users/{userId}/status.
You could send put with body active to users/{userId}/status to make a
user active. Or you could get users/{userId}/status to retrieve the
status of a user.
As explained in another e-mail thread, it does not make sense to set a
"generic" status for users: "activate" an user is not the same as
setting its status to "active" (and so on): the difference is given by
the workflow definition that any single project could implement to suit
its own needs.
For these reasons, we should try to find a way to express the operations:
* activate()
* suspend()
* reactivate()
in RESTful terms.
But "putting user status" is not an option, IMO.
I think it makes sense to think about these things to make the API as
good as possible. On the other hand I think these considerations should
not block the 1.1.0 release.
Agree.
Regards.
--
Francesco Chicchiriccò
ASF Member, Apache Syncope PMC chair, Apache Cocoon PMC Member
http://people.apache.org/~ilgrosso/
