shindig-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Dumont" <ddum...@us.ibm.com>
Subject Re: Review Request: A patch that enables API for updating person data
Date Mon, 23 Jan 2012 14:32:35 GMT


> On 2012-01-20 16:35:55, Stanton Sievers wrote:
> > Small nits on whitespace.
> > 
> > I do have some bigger questions regarding the implementation and maybe these are
more for the spec group.  What's to stop one person from updating another person?  The rest
API says that the User-Id defaults to @me, but doesn't say you can't list another user.  Shouldn't
we only allow @me to be updated, which would be equivalent to the viewer id in the security
token?
> > 
> > You mentioned implementing osapi.people.update() as a gadget API, but I neither
see it in this review (is it there by virtue of the REST API being implemented?) nor do I
see any tests for it.  I  also don't see osapi.people.update() in the social gadget spec and
if the intent is for it to be there, you should try to get the spec updated as well.
> 
> Evgeny Bogdanov wrote:
>     Thanks Stanton for your points.
>     > 1. What's to stop one person from updating another person?
>     I also had this question, but then I thought that maybe it depends on container,
>     because anyways it's done in PersonServiceDb which is container specific normally.
>     I'll start a discussion on it in the spec mailing list.
>     
>     > 2. osapi.people.update()
>     it's true, it is not in the spec, only REST api, should be added I believe.
>     Once one implements the rest api in shindig, javascript api is automatically available.
>     I m not sure whether it should be tested or not, since this code was not written
by me :)
> 
> Stanton Sievers wrote:
>     I had some offline conversations about #1 and I think you are right that it depends
on the container.  For instance, to enable administration oriented use cases, where you have
an admin with elevated privileges, you would need to allow one person to update another in
the server code.  The PersonServiceDb would have to handle the rights management.
>     
>     Re #2.  If you don't mind writing up the tests I think they would be useful.  It's
reassuring (at least to me :) to have explicit tests for code like that as a place where we
can point and say "yes, osapi.people.update()" works, and say it with a greater level of confidence.
 This doesn't seem to be the case with the osapi.people.get, et al, however... I can't find
explicit feature code tests for it.  Maybe this is a larger question for the whole dev list.
> 
> Evgeny Bogdanov wrote:
>     well, it seems as osapi.people.get, etc. are tested.
>     1) in peopleTest.xml file
>     2) in peoplesuite.js (suite.xml)
>     
>     I don't know if they run automatically with mvn, but at least they can be open as
a gadget that will run tests
>     I will add those tests

I think I'd like to see the default shindig impl only allow users to edit their own data with
this api.  I'd also like to see a place where container implementations can override this
behavior easily.   Where would they do that today, inside the entiyManager?


- Dan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3544/#review4491
-----------------------------------------------------------


On 2012-01-19 09:30:25, Evgeny Bogdanov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3544/
> -----------------------------------------------------------
> 
> (Updated 2012-01-19 09:30:25)
> 
> 
> Review request for shindig.
> 
> 
> Summary
> -------
> 
> A patch that enables API for updating person data
> PUT people/12/@self
> osapi.people.update()
> 
> 
> for this JIRA reports
> https://issues.apache.org/jira/browse/SHINDIG-1674
> 
> 
> Diffs
> -----
> 
>   http://svn.apache.org/repos/asf/shindig/trunk/java/samples/src/main/java/org/apache/shindig/social/opensocial/jpa/spi/PersonServiceDb.java
1231912 
>   http://svn.apache.org/repos/asf/shindig/trunk/java/samples/src/test/java/org/apache/shindig/social/opensocial/jpa/spi/PersonServiceDbTest.java
1231912 
>   http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/opensocial/service/PersonHandler.java
1231912 
>   http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/opensocial/spi/PersonService.java
1231912 
>   http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/sample/spi/JsonDbOpensocialService.java
1231912 
>   http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/test/java/org/apache/shindig/social/opensocial/service/PersonHandlerTest.java
1231912 
>   http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/test/java/org/apache/shindig/social/sample/spi/JsonDbOpensocialServiceTest.java
1231912 
> 
> Diff: https://reviews.apache.org/r/3544/diff
> 
> 
> Testing
> -------
> 
> tests are in the patch
> 
> 
> Thanks,
> 
> Evgeny
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message