On Fri, Feb 20, 2009 at 9:17 AM, Ian Boston <[email protected]> wrote: > Adam, > There was a long(ish) thread on this over 2 weeks. IMHO its not quite as > simple as that. > > The use of the getPeople was specified by the REST API was to > get all the people associated with one ID and then filter that set down to > match a single ID. > > getPerson gets the person specified in the call. How would the filter be > applied ? I think returning null depending on the filter would be almost as > confusing. > > Before suggesting that we didn't change the SPI, I read the javadoc for the > SPI and considered if a set of a single ID with a filter matched the SPI > specification. IMHO it did, hence the suggestion. > > Unfortunately getPeople does not return a structure of the correct form so > it needs to be processed without breaking the Future contract.
> > ------------------------------------ > > > As for the commit: In the past, we have reviewed patches in JIRA, talked > about them and the committed them. > > Have we changed the process to have all patches go into code review before > being comitted ? > I dont remember seeing any conclusions about this on list? No, there isn't any such requirement. Checking in without a review means there's a greater chance of someone griping after the commit, which is a bit more expensive for all concerned. That's what's happening now - which is all fine, and perfectly normal. The fact that people care is a Good Thing for the health of the project. > If this is the way we are supposed to be working, then what are the > thresholds requiring review. I realize that I haven't been able to do > anything for several months, manly because of the pressures of my > employment, so I might be out of touch. > > Ian > > > > On 20 Feb 2009, at 16:50, Adam Winer wrote: > > This breaks the interface in a worse way, and leaves the APIs broken going >> forward - one method that doesn't get called, and one method that *thinks* >> its returning a collection, but really isn't in some undocumented cases. >> I >> know it broke our servers when we upgraded (our implementation of >> getPerson() was subtly different from getPeople(), and intentionally so.) >> Better to break APIs in a clear and simple way than do so cryptically. >> We shouldn't be changing the service layer on the 1.0 branch, but we can't >> commit to API stability on trunk. >> >> On Fri, Feb 20, 2009 at 8:06 AM, Ben Smith <[email protected]> >> wrote: >> >> That's what I suggested (and supplied a patch for) but Ian was keen not >>> to >>> break the interface for the service layer. >>> >>> >>> On 20 Feb 2009, at 16:02, Adam Winer wrote: >>> >>> Speaking of this change: what got committed seems odd. For scalar >>> >>>> calls, PersonHandler now calls PersonService.getPeople(), and drops >>>> all but the first result. PersonService.getPerson() is never called. >>>> >>>> It seems significantly cleaner to add CollectionOptions to >>>> PersonService.getPerson() (and, perhaps, rename CollectionOptions). >>>> Why did we go this route? Sorry that I didn't follow the discussion >>>> the first time through. >>>> >>>> On Fri, Feb 20, 2009 at 1:39 AM, Ben Smith <[email protected]> >>>> wrote: >>>> >>>> Hi Louis, sorry I missed this. >>>>> >>>>> I'll make sure I do this from now on. I thought it was something that >>>>> commiters did, sorry for the omission. >>>>> >>>>> On 17 Feb 2009, at 17:40, Louis Ryan wrote: >>>>> >>>>> Ben >>>>> >>>>>> >>>>>> Can you post this patch to codereview.appspot.com. Its become working >>>>>> practice to post changes that are likely to incur feedback there. >>>>>> >>>>>> -Louis >>>>>> >>>>>> On Sat, Feb 14, 2009 at 8:37 AM, Ben Smith (JIRA) <[email protected]> >>>>>> wrote: >>>>>> >>>>>> >>>>>> [ >>>>>>> >>>>>>> >>>>>>> >>>>>>> https://issues.apache.org/jira/browse/SHINDIG-918?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel >>>>>>> ] >>>>>>> >>>>>>> Ben Smith updated SHINDIG-918: >>>>>>> ------------------------------ >>>>>>> >>>>>>> Attachment: SHINDIG-918-improvement.patch >>>>>>> >>>>>>> Has PersonHandler call getPeople for all types of request. >>>>>>> >>>>>>> Change to PersonHandler to allow filtering of requests - 0.9 >>>>>>> >>>>>>>> improvement >>>>>>>> >>>>>>>> >>>>>>>> ------------------------------------------------------------------------ >>>>>>>> >>>>>>>> Key: SHINDIG-918 >>>>>>>> URL: https://issues.apache.org/jira/browse/SHINDIG-918 >>>>>>>> Project: Shindig >>>>>>>> Issue Type: Improvement >>>>>>>> Components: Java >>>>>>>> Affects Versions: trunk >>>>>>>> Reporter: Ben Smith >>>>>>>> Fix For: trunk >>>>>>>> >>>>>>>> Attachments: SHINDIG-918-improvement.patch >>>>>>>> >>>>>>>> >>>>>>>> Calls for single people (say, /people/@me/@self) should be >>>>>>>> filterable: >>>>>>>> >>>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> http://opensocial-resources.googlecode.com/svn/spec/draft/REST-API.xml#standardQueryParameters >>>>>>> >>>>>>> >>>>>>>> Because PersonHandler calls PersonService.getPerson() for such >>>>>>>> requests, >>>>>>>> >>>>>>>> >>>>>>> which doesn't accept CollectionOptions, the result can't be filtered. >>>>>>> A >>>>>>> patch in SHINDIG-904 solved this by changing the getPerson() method >>>>>>> signature but after much discussion on the mailing list it was >>>>>>> decided >>>>>>> that >>>>>>> a better solution would be to change PersonHandler to only call >>>>>>> getPeople, >>>>>>> and convert the RestfulCollection result to a single Person when >>>>>>> calls >>>>>>> for a >>>>>>> single user are made (like, /people/@me/@self). >>>>>>> >>>>>>> >>>>>>>> Patch to follow. >>>>>>>> >>>>>>>> >>>>>>> -- >>>>>>> This message is automatically generated by JIRA. >>>>>>> - >>>>>>> You can reply to this email to add a comment to the issue online. >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>> >>>>> >>> >
