You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@shindig.apache.org by Louis Ryan <lr...@google.com> on 2009/02/17 18:40:54 UTC
Re: [jira] Updated: (SHINDIG-918) Change to PersonHandler to allow
filtering of requests - 0.9 improvement
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) <ji...@apache.org> 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.
>
>
Re: [jira] Updated: (SHINDIG-918) Change to PersonHandler to allow
filtering of requests - 0.9 improvement
Posted by Adam Winer <aw...@google.com>.
On Fri, Feb 20, 2009 at 9:27 AM, Paul Lindner <pl...@hi5.com> wrote:
> Well, one thing we could do is this:
>
> * If a filter for a single user is supplied, then call getPeople. This
> means that getPerson remains filter/sort free
> * If no filter, then continue to call getPerson.
Seems like a reasonable compromise that keeps the SPI intact.
>
>
> On Feb 20, 2009, at 9:20 AM, Ian Boston wrote:
>
> Sorry, I didn't know.... it was talked about at length on list, but no one
>> looked like there were interested. I will bow out and let those with more
>> skin in the game decide.
>>
>> On 20 Feb 2009, at 17:12, Paul Lindner wrote:
>>
>> +1 on adding collectionOptions to getPerson()
>>>
>>> We too have a heavily optimized version of getPerson() that's now not
>>> being called.
>>>
>>>
>>> On Feb 20, 2009, at 9:03 AM, Ben Smith wrote:
>>>
>>> All sounds pretty reasonable. I'm going to have to bow out and let the
>>>> committers come to the right decision. There is a patch for either option
>>>> (that will probably need updating).
>>>>
>>>> Let me know what you decide.
>>>> Ben
>>>>
>>>> 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 <be...@gmail.com>
>>>>> 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 <be...@gmail.com>
>>>>>>> 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) <jira@apache.org
>>>>>>>>> >
>>>>>>>>> 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.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>>
>>
>
Re: [jira] Updated: (SHINDIG-918) Change to PersonHandler to allow filtering of requests - 0.9 improvement
Posted by Paul Lindner <pl...@hi5.com>.
Well, one thing we could do is this:
* If a filter for a single user is supplied, then call getPeople.
This means that getPerson remains filter/sort free
* If no filter, then continue to call getPerson.
On Feb 20, 2009, at 9:20 AM, Ian Boston wrote:
> Sorry, I didn't know.... it was talked about at length on list, but
> no one looked like there were interested. I will bow out and let
> those with more skin in the game decide.
>
> On 20 Feb 2009, at 17:12, Paul Lindner wrote:
>
>> +1 on adding collectionOptions to getPerson()
>>
>> We too have a heavily optimized version of getPerson() that's now
>> not being called.
>>
>>
>> On Feb 20, 2009, at 9:03 AM, Ben Smith wrote:
>>
>>> All sounds pretty reasonable. I'm going to have to bow out and let
>>> the committers come to the right decision. There is a patch for
>>> either option (that will probably need updating).
>>>
>>> Let me know what you decide.
>>> Ben
>>>
>>> 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
>>>> <be...@gmail.com> 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 <ben.thesmith@gmail.com
>>>>>> >
>>>>>> 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) <jira@apache.org
>>>>>>>> >
>>>>>>>> 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.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>
>>>
>>
>
Re: [jira] Updated: (SHINDIG-918) Change to PersonHandler to allow filtering of requests - 0.9 improvement
Posted by Ian Boston <ie...@tfd.co.uk>.
Sorry, I didn't know.... it was talked about at length on list, but no
one looked like there were interested. I will bow out and let those
with more skin in the game decide.
On 20 Feb 2009, at 17:12, Paul Lindner wrote:
> +1 on adding collectionOptions to getPerson()
>
> We too have a heavily optimized version of getPerson() that's now
> not being called.
>
>
> On Feb 20, 2009, at 9:03 AM, Ben Smith wrote:
>
>> All sounds pretty reasonable. I'm going to have to bow out and let
>> the committers come to the right decision. There is a patch for
>> either option (that will probably need updating).
>>
>> Let me know what you decide.
>> Ben
>>
>> 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
>>> <be...@gmail.com> 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
>>>>> <be...@gmail.com>
>>>>> 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) <jira@apache.org
>>>>>>> >
>>>>>>> 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.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>>
>>>>
>>
>
Re: [jira] Updated: (SHINDIG-918) Change to PersonHandler to allow filtering of requests - 0.9 improvement
Posted by Paul Lindner <pl...@hi5.com>.
+1 on adding collectionOptions to getPerson()
We too have a heavily optimized version of getPerson() that's now not
being called.
On Feb 20, 2009, at 9:03 AM, Ben Smith wrote:
> All sounds pretty reasonable. I'm going to have to bow out and let
> the committers come to the right decision. There is a patch for
> either option (that will probably need updating).
>
> Let me know what you decide.
> Ben
>
> 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 <be...@gmail.com>
>> 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 <be...@gmail.com>
>>>> 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) <jira@apache.org
>>>>>> >
>>>>>> 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.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>
>>>>>
>>>
>
Re: [jira] Updated: (SHINDIG-918) Change to PersonHandler to allow filtering of requests - 0.9 improvement
Posted by Ben Smith <be...@gmail.com>.
All sounds pretty reasonable. I'm going to have to bow out and let the
committers come to the right decision. There is a patch for either
option (that will probably need updating).
Let me know what you decide.
Ben
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 <be...@gmail.com>
> 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 <be...@gmail.com>
>>> 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)
>>>>> <ji...@apache.org>
>>>>> 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.
>>>>>>
>>>>>>
>>>>>>
>>>>
>>>>
>>
Re: Commit process [was Re: [jira] Updated: (SHINDIG-918) Change to PersonHandler to allow filtering of requests - 0.9 improvement]
Posted by Paul Lindner <pl...@hi5.com>.
I think what happened with this change is completely acceptable.
Given my limited bandwidth I can't review every change as it comes
in. I'm sure there are plenty of people in my situation as well.
Given that issues are in JIRA and they're referenced in checkins I'm
happy.
For largish changes I'm happy to see code review, since that has
proven successful in improving quality.
On Feb 21, 2009, at 3:23 AM, Ian Boston wrote:
> (I am asking these questions to get a common understanding of how we
> work as a community, not because I feel I am right or wrong)
>
> I agree, people caring, and reviewing the code before/after and long
> after commit is 100% what this community should be doing. No
> argument there, what I want to know is how we achieve that...
> because at the moment we have 2 processes for code review and its
> confusing.
>
> So, neither I nor Ben pushed the patch to the appspot codereview.
> (not least because my google id is not subscribed to shindig-dev and
> for some reason I cant make aliases work properly with google id's)
>
> but the patch was pushed into JIRA and then discussed at some length
> on the list, ('Extension to getPerson for
> 0.9',SHINDIG-904,SHINDIG-918,'PersonService precludes filtering'),
> starting on 5 Feb.
>
> IMHO, that was review?
>
> If patch + jira is not going to considered sufficient for code
> review then IMHO we need to make that clear so decisions like the
> one I made don't go un-noticed by those with production
> implementations until they are found in production.
>
> BTW, I was thinking of those who had already implemented the SPI in
> making the decision, but obviously I was not privy to their source
> code.
>
>
> I have no preference to how we work, either way is good with me, but
> I don't want to be making decisions totally on my own, just because
> one half of the community is working one way, and the other are
> working another.
>
> Ian
>
> (also IMHO, just because something was reviewed doesn't mean that
> its perfect and cant be made better in the future and always the
> case with anything I write :))
>
>
>
> On 20 Feb 2009, at 18:22, Adam Winer wrote:
>
>>>
>>> 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.
>
Commit process [was Re: [jira] Updated: (SHINDIG-918) Change to PersonHandler to allow filtering of requests - 0.9 improvement]
Posted by Ian Boston <ie...@tfd.co.uk>.
(I am asking these questions to get a common understanding of how we
work as a community, not because I feel I am right or wrong)
I agree, people caring, and reviewing the code before/after and long
after commit is 100% what this community should be doing. No argument
there, what I want to know is how we achieve that... because at the
moment we have 2 processes for code review and its confusing.
So, neither I nor Ben pushed the patch to the appspot codereview. (not
least because my google id is not subscribed to shindig-dev and for
some reason I cant make aliases work properly with google id's)
but the patch was pushed into JIRA and then discussed at some length
on the list, ('Extension to getPerson for
0.9',SHINDIG-904,SHINDIG-918,'PersonService precludes filtering'),
starting on 5 Feb.
IMHO, that was review?
If patch + jira is not going to considered sufficient for code review
then IMHO we need to make that clear so decisions like the one I made
don't go un-noticed by those with production implementations until
they are found in production.
BTW, I was thinking of those who had already implemented the SPI in
making the decision, but obviously I was not privy to their source code.
I have no preference to how we work, either way is good with me, but I
don't want to be making decisions totally on my own, just because one
half of the community is working one way, and the other are working
another.
Ian
(also IMHO, just because something was reviewed doesn't mean that its
perfect and cant be made better in the future and always the case with
anything I write :))
On 20 Feb 2009, at 18:22, Adam Winer wrote:
>>
>> 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.
Re: [jira] Updated: (SHINDIG-918) Change to PersonHandler to allow
filtering of requests - 0.9 improvement
Posted by Adam Winer <aw...@google.com>.
On Fri, Feb 20, 2009 at 9:17 AM, Ian Boston <ie...@tfd.co.uk> 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 <be...@gmail.com>
>> 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 <be...@gmail.com>
>>>> 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) <ji...@apache.org>
>>>>>> 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.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>
>>>>>
>>>
>
Re: [jira] Updated: (SHINDIG-918) Change to PersonHandler to allow filtering of requests - 0.9 improvement
Posted by Ian Boston <ie...@tfd.co.uk>.
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?
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 <be...@gmail.com>
> 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 <be...@gmail.com>
>>> 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)
>>>>> <ji...@apache.org>
>>>>> 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.
>>>>>>
>>>>>>
>>>>>>
>>>>
>>>>
>>
Re: [jira] Updated: (SHINDIG-918) Change to PersonHandler to allow
filtering of requests - 0.9 improvement
Posted by Adam Winer <aw...@google.com>.
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 <be...@gmail.com> 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 <be...@gmail.com>
>> 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) <ji...@apache.org>
>>>> 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.
>>>>>
>>>>>
>>>>>
>>>
>>>
>
Re: [jira] Updated: (SHINDIG-918) Change to PersonHandler to allow filtering of requests - 0.9 improvement
Posted by Ben Smith <be...@gmail.com>.
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 <be...@gmail.com>
> 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)
>>> <ji...@apache.org> 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.
>>>>
>>>>
>>
>>
Re: [jira] Updated: (SHINDIG-918) Change to PersonHandler to allow
filtering of requests - 0.9 improvement
Posted by Adam Winer <aw...@gmail.com>.
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 <be...@gmail.com> 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) <ji...@apache.org> 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.
>>>
>>>
>
>
Re: [jira] Updated: (SHINDIG-918) Change to PersonHandler to allow filtering of requests - 0.9 improvement
Posted by Ben Smith <be...@gmail.com>.
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) <ji...@apache.org>
> 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.
>>
>>