RE: [DISCUSS] User Service
Hi syncoper,
I do not even see methods in RoleService that should be moved into a different
ServiceInterface! Workflow controller is used in RoleController but just for
CRUD operations, no direct access, so this is mostly transparent to user.
But I completely agree with Christian, that direct workflow operations
identified by TaskID should be moved out of UserService. They are used in
context of users (currently) but can be used also in other contexts. This is
why I would move the following method to WorkflowService:
@POST
@Path(workflow/tasks/{taskId}/claim)
WorkflowFormTO claimForm(@PathParam(taskId) String taskId);
@POST
@Path(workflow/tasks/{taskId}/execute)
UserTO executeWorkflow(@PathParam(taskId) String taskId, UserTO userTO);
@GET
@Path(workflow/form)
ListWorkflowFormTO getForms();
@POST
@Path(workflow/form)
UserTO submitForm(WorkflowFormTO form);
Since this next method contains a userId but is related to workflow activity,
I'm not sure what to do with it. I guess is should remain in UserService, to
not have an dependency from WorkflowService to UserService.
@GET
@Path({userId}/workflow/form)
WorkflowFormTO getFormForUser(@PathParam(userId) Long userId);
Best regards.
Jan
-Original Message-
From: Francesco Chicchiriccò [mailto:ilgro...@apache.org]
Sent: Dienstag, 5. Februar 2013 11:03
To: dev@syncope.apache.org
Subject: Re: [DISCUSS] User Service
On 05/02/2013 10:01, Christian Schneider wrote:
On 04.02.2013 18:10, Francesco Chicchiriccò wrote:
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?
UserService and RoleService contain the full set of crud operations.
So why would they be readonly?
I am not absolutely sure if separating the interfaces makes sense but
I think it could. One indicator is that the UserService always works
with the userId or userName while the workflow methods normally use
taskId as a key. So I think this indicates that the resource we are
handling is not the user but rather a workflow identified by a task.
So naming it UserWorkflowService sounds intuitive to me.
Ok, could you please detail which methods from the current UserService and
RoleService you would like to move into new UserWorkflowService /
RoleWorkflowService?
Why make this distinction for SyncopeUser and not for any other
entity? We expose internal ids for everything.
That is true. It would not make too much sense to make an exemption
for User.
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 will experiment with introducing methods that take a userId or
userName String. So at least we can cut half of the methods for status
changes.
Is it ok to remove the reactivate and use activate with a null token
instead or is this too different in meaning?
They have different meaning and are mapped to different methods on the
underlying UserWorkflowAdapter, so we should keed them distinct.
Regards.
--
Francesco Chicchiriccò
ASF Member, Apache Syncope PMC chair, Apache Cocoon PMC Member
http://people.apache.org/~ilgrosso/
Re: [DISCUSS] User Service
On 08/02/2013 09:05, Jan Bernhardt wrote:
Hi syncoper,
I do not even see methods in RoleService that should be moved into a different
ServiceInterface! Workflow controller is used in RoleController but just for
CRUD operations, no direct access, so this is mostly transparent to user.
But I completely agree with Christian, that direct workflow operations
identified by TaskID should be moved out of UserService. They are used in
context of users (currently) but can be used also in other contexts. This is
why I would move the following method to WorkflowService:
@POST
@Path(workflow/tasks/{taskId}/claim)
WorkflowFormTO claimForm(@PathParam(taskId) String taskId);
@POST
@Path(workflow/tasks/{taskId}/execute)
UserTO executeWorkflow(@PathParam(taskId) String taskId, UserTO userTO);
@GET
@Path(workflow/form)
ListWorkflowFormTO getForms();
@POST
@Path(workflow/form)
UserTO submitForm(WorkflowFormTO form);
Since this next method contains a userId but is related to workflow activity,
I'm not sure what to do with it. I guess is should remain in UserService, to
not have an dependency from WorkflowService to UserService.
@GET
@Path({userId}/workflow/form)
WorkflowFormTO getFormForUser(@PathParam(userId) Long userId);
Hi Jan,
I think I've finally got your point - it took me some time, though..
getFormForUser() is not the only workflow-related method explicitly
referencing users (userId in that case); there is also executeWorkflow()
with UserTO for example, but if you take a look at method implementation
there is even more.
Hence my proposal: let's call it UserWorkflowService and let's move all
the methods above (including getFormForUser()) to this new service.
WDYT?
Regards.
--
Francesco Chicchiriccò
ASF Member, Apache Syncope PMC chair, Apache Cocoon PMC Member
http://people.apache.org/~ilgrosso/
Re: [DISCUSS] User Service
On 04.02.2013 18:10, Francesco Chicchiriccò wrote:
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?
UserService and RoleService contain the full set of crud operations. So
why would they be readonly?
I am not absolutely sure if separating the interfaces makes sense but I
think it could. One indicator is that the UserService always works with
the userId or userName while the workflow methods normally use taskId as
a key. So I think this indicates that the resource we are handling is
not the user but rather a workflow identified by a task. So naming it
UserWorkflowService sounds intuitive to me.
Why make this distinction for SyncopeUser and not for any other
entity? We expose internal ids for everything.
That is true. It would not make too much sense to make an exemption for
User.
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 will experiment with introducing methods that take a userId or
userName String. So at least we can cut half of the methods for status
changes.
Is it ok to remove the reactivate and use activate with a null token
instead or is this too different in meaning?
Best regards
Christian
--
Christian Schneider
http://www.liquid-reality.de
Open Source Architect
http://www.talend.com
RE: [DISCUSS] User Service
-Original Message- From: Francesco Chicchiriccò [mailto:ilgro...@apache.org] Sent: Freitag, 1. Februar 2013 17:45 To: dev@syncope.apache.org Subject: Re: [DISCUSS] User Service On 01/02/2013 17:32, Jan Bernhardt wrote: Hi Syncoper, I'm almost done with new REST API Service Interface documentation [1]. Last peace missing is only User Service. Please take time to review changes to new Service Interfaces and let's have a discussion on this mailing-list. I think we already discussed all general changes, but if you find something missing, just continue this discussion. I have a couple of proposals for changes in User Service Interface, which I would like to get your feedback for. After we all agree with User Interface I will update Interface and Documentation accordingly. 1. User Controller contains 5 methods focusing on workflow activities. My proposal would be to move these methods to Workflow Service interface. We could leave business code in UserController class for 1.1.0 release and only change User and Workflow Service Interfaces for now, so that this change will only be visible to new CXF REST API. The WorkflowController is a general interface for managing and querying the workflow definitions (that can vary depending on the standard or custom workflow adapters you have on your own project). 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. 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? 3. There are two methods (and status types) to activate an user account. ACTIVATE and REACTIVATE. From my point of view REACTIVATE is redundant, since it shouldn't matter if user was active previously or not. All that matters is that user should be active afterwards. Therefore my proposal would be to remove REACTIVATE status type and also reactivate operation method. Activate and reactivate are differentiated at UserWorkflowAdapter level because they are in fact two distinct operations. This difference is then naturally reflected into the REST interface. One strength of a REST is its STATELESS design [1], which brings many advantages IMHO. So I would still recommend to refactor our current REST implementation. Of course we don't need to do this for 1.1.0 release. But do you agree that this would be a valuable improvement and that I should create a JIRA issue for that? Best regards. Jan [1] http://en.wikipedia.org/wiki/Representational_state_transfer#Constraints Regards. [1] https://cwiki.apache.org/confluence/display/SYNCOPE/REST+API+upgrade -- Francesco Chicchiriccò ASF Member, Apache Syncope PMC chair, Apache Cocoon PMC Member http://people.apache.org/~ilgrosso/
Re: [DISCUSS] User Service
On 04/02/2013 12:12, Jan Bernhardt wrote: -Original Message- From: Francesco Chicchiriccò [mailto:ilgro...@apache.org] Sent: Freitag, 1. Februar 2013 17:45 To: dev@syncope.apache.org Subject: Re: [DISCUSS] User Service On 01/02/2013 17:32, Jan Bernhardt wrote: Hi Syncoper, I'm almost done with new REST API Service Interface documentation [1]. Last peace missing is only User Service. Please take time to review changes to new Service Interfaces and let's have a discussion on this mailing-list. I think we already discussed all general changes, but if you find something missing, just continue this discussion. I have a couple of proposals for changes in User Service Interface, which I would like to get your feedback for. After we all agree with User Interface I will update Interface and Documentation accordingly. 1. User Controller contains 5 methods focusing on workflow activities. My proposal would be to move these methods to Workflow Service interface. We could leave business code in UserController class for 1.1.0 release and only change User and Workflow Service Interfaces for now, so that this change will only be visible to new CXF REST API. The WorkflowController is a general interface for managing and querying the workflow definitions (that can vary depending on the standard or custom workflow adapters you have on your own project). 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. 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. 3. There are two methods (and status types) to activate an user account. ACTIVATE and REACTIVATE. From my point of view REACTIVATE is redundant, since it shouldn't matter if user was active previously or not. All that matters is that user should be active afterwards. Therefore my proposal would be to remove REACTIVATE status type and also reactivate operation method. Activate and reactivate are differentiated at UserWorkflowAdapter level because they are in fact two distinct operations. This difference is then naturally reflected into the REST interface. One strength of a REST is its STATELESS design [1], which brings many advantages IMHO. So I would still recommend to refactor our current REST implementation. Of course we don't need to do this for 1.1.0 release. But do you agree that this would be a valuable improvement and that I should create a JIRA issue for that? Ok, so we must match a non-functional REST need (providing stateless APIs) with a functional need (user workflow is stateful by definition, activate() and reactivate() maps to two completely different statuses). Under these conditions, I am +1 to open an issue for that. Regards. [1] http://en.wikipedia.org/wiki/Representational_state_transfer#Constraints Regards. [1] https://cwiki.apache.org/confluence/display/SYNCOPE/REST+API+upgrade -- Francesco Chicchiriccò ASF Member, Apache Syncope PMC chair, Apache Cocoon PMC Member http://people.apache.org
Re: [DISCUSS] User Service
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.
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.
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.
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.
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.
Best regards
Christian
--
Christian Schneider
http://www.liquid-reality.de
Open Source Architect
http://www.talend.com
Re: [DISCUSS] User Service
One other idea for the username / userId based methods.
Why not simply use:
/users/{userIdorName}
So basically you could do
get /users/100 or get /users/chris@mydomain
We could handle both using
@GET
@Path({userId})
UserTO read(@PathParam(userId) String userId);
This is convenient for any kind of client and quite efficient at the
same time.
Christian
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.
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.
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.
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.
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.
Best regards
Christian
--
Christian Schneider
http://www.liquid-reality.de
Open Source Architect
http://www.talend.com
Re: [DISCUSS] User Service
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/
Re: [DISCUSS] User Service
On 01/02/2013 17:32, Jan Bernhardt wrote: Hi Syncoper, I'm almost done with new REST API Service Interface documentation [1]. Last peace missing is only User Service. Please take time to review changes to new Service Interfaces and let's have a discussion on this mailing-list. I think we already discussed all general changes, but if you find something missing, just continue this discussion. I have a couple of proposals for changes in User Service Interface, which I would like to get your feedback for. After we all agree with User Interface I will update Interface and Documentation accordingly. 1. User Controller contains 5 methods focusing on workflow activities. My proposal would be to move these methods to Workflow Service interface. We could leave business code in UserController class for 1.1.0 release and only change User and Workflow Service Interfaces for now, so that this change will only be visible to new CXF REST API. The WorkflowController is a general interface for managing and querying the workflow definitions (that can vary depending on the standard or custom workflow adapters you have on your own project). 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. 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. 3. There are two methods (and status types) to activate an user account. ACTIVATE and REACTIVATE. From my point of view REACTIVATE is redundant, since it shouldn't matter if user was active previously or not. All that matters is that user should be active afterwards. Therefore my proposal would be to remove REACTIVATE status type and also reactivate operation method. Activate and reactivate are differentiated at UserWorkflowAdapter level because they are in fact two distinct operations. This difference is then naturally reflected into the REST interface. Regards. [1] https://cwiki.apache.org/confluence/display/SYNCOPE/REST+API+upgrade -- Francesco Chicchiriccò ASF Member, Apache Syncope PMC chair, Apache Cocoon PMC Member http://people.apache.org/~ilgrosso/
