You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@syncope.apache.org by Jan Bernhardt <jb...@talend.com> on 2013/02/01 17:32:26 UTC
[DISCUSS] User Service
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.
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.
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.
WDYT?
Best regards.
Jan
[1] https://cwiki.apache.org/confluence/display/SYNCOPE/REST+API+upgrade
Re: [DISCUSS] User Service
Posted by Christian Schneider <ch...@die-schneider.net>.
+1
Christian
On 08.02.2013 09:12, Francesco Chicchiriccò wrote:
> 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")
>> List<WorkflowFormTO> 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.
>
--
Christian Schneider
http://www.liquid-reality.de
Open Source Architect
http://www.talend.com
RE: [DISCUSS] User Service
Posted by Jan Bernhardt <jb...@talend.com>.
I created a jira ticket for this: https://issues.apache.org/jira/browse/SYNCOPE-312
Best regards.
Jan
> -----Original Message-----
> From: Jan Bernhardt [mailto:jbernhardt@talend.com]
> Sent: Freitag, 8. Februar 2013 14:06
> To: dev@syncope.apache.org
> Subject: RE: [DISCUSS] User Service
>
> +1
> Sounds good to me!
>
> Let us create a UserWorkflowService, but let's not create a
> RoleWorkflowService (for now) thus leaving RoleService unchanged.
>
> Best regards.
> Jan
>
>
> > -----Original Message-----
> > From: Francesco Chicchiriccò [mailto:ilgrosso@apache.org]
> > Sent: Freitag, 8. Februar 2013 09:13
> > To: dev@syncope.apache.org
> > Subject: 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")
> > > List<WorkflowFormTO> 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
Posted by Jan Bernhardt <jb...@talend.com>.
+1
Sounds good to me!
Let us create a UserWorkflowService, but let's not create a RoleWorkflowService (for now) thus leaving RoleService unchanged.
Best regards.
Jan
> -----Original Message-----
> From: Francesco Chicchiriccò [mailto:ilgrosso@apache.org]
> Sent: Freitag, 8. Februar 2013 09:13
> To: dev@syncope.apache.org
> Subject: 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")
> > List<WorkflowFormTO> 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
Posted by Francesco Chicchiriccò <il...@apache.org>.
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")
> List<WorkflowFormTO> 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
Posted by Jan Bernhardt <jb...@talend.com>.
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")
List<WorkflowFormTO> 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:ilgrosso@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
Posted by Francesco Chicchiriccò <il...@apache.org>.
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
Posted by Christian Schneider <ch...@die-schneider.net>.
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
Posted by Francesco Chicchiriccò <il...@apache.org>.
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
Posted by Francesco Chicchiriccò <il...@apache.org>.
On 04/02/2013 14:06, Christian Schneider wrote:
> 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.
+1
This seems to be the egg of Columbus.
Regards.
--
Francesco Chicchiriccò
ASF Member, Apache Syncope PMC chair, Apache Cocoon PMC Member
http://people.apache.org/~ilgrosso/
Re: [DISCUSS] User Service
Posted by Christian Schneider <ch...@die-schneider.net>.
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
Posted by Christian Schneider <ch...@die-schneider.net>.
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
Posted by Francesco Chicchiriccò <il...@apache.org>.
On 04/02/2013 12:12, Jan Bernhardt wrote:
>> -----Original Message-----
>> From: Francesco Chicchiriccò [mailto:ilgrosso@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/~ilgrosso/
RE: [DISCUSS] User Service
Posted by Jan Bernhardt <jb...@talend.com>.
> -----Original Message-----
> From: Francesco Chicchiriccò [mailto:ilgrosso@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
Posted by Francesco Chicchiriccò <il...@apache.org>.
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/