lazr.restful

Code review comment for lp:~salgado/lazr.restful/extension-interfaces

extension-interfaces
Merge into trunk

Revision history for this message

Gary Poster (gary) wrote on 2010-08-04:

On Aug 3, 2010, at 5:46 PM, Guilherme Salgado wrote:

> On Tue, 2010-08-03 at 18:11 +0000, Gary Poster wrote:
>> This is really a very nice branch. Thank you.
>>
>> I hate to not give it a final go-ahead, but I do have one last request.
>>
>> = Requests =
>>
>> You said:
>>> Leonard said:
>>>> I would like to see this feature used in an example web service (ie.
>>>> added to examples/base or in a totally new web service of your
>>>> design). This is less for purposes of test coverage and more to show
>>>> how the feature would work in a real situation.
>>>
>>> I moved some things in examples/base to make use of the new feature
>>> (http://paste.ubuntu.com/471195/), but I see no reason why people would
>>> use it like that in a simple interface. In fact, since the need for
>>> this feature came from us wanting to split the implementation of some
>>> very complex classes into smaller ones, I find it hard to think of a
>>> good yet short example to use here. Maybe you have an idea?
>>
>> I'd argue that the central story is reuse. That reuse might be to
>> factor out a simpler webservice that can be reused elsewhere (which is
>> actually what is driving your work, AIUI) or it might be to extend a
>> simpler webservice (which you might end up doing as well). I think
>> you could easily explain a change in this way.
>
> Well, what we really want is to be able to move attributes/methods out
> of existing interfaces and into adapters while still publicizing a
> single entry comprising what's exported by the changed interface and all
> of its adapters. For the reuse story one can use mix-ins as long as
> it's ok to have the exported interface inherit from a bunch of others
> (as we currently do).

Right, but the mix-in approach means a lot more work for small changes. I could be wrong, but with the mix-in class approach, you have to make your own separate registration of *everything* because you don't want one interface, you want another one. There are examples of this kind of annoyance in the webservice package. With your feature, you can reuse the entirety of a webservice, including its registrations, and then register your own things in addition. That's much easier.

Maybe I'm missing something, but that's how I see it right now.

>
>>
>> If you didn't already have an example, I might try to think up some
>> scenario, like another branch of a company that wanted to add, say,
>> the idea of a recipe contributor, or wanted to add the idea of
>> favorite recipes or recipe ratings. That particular idea might be
>> undesirable for the imaginary people using the base website.
>>
>> What you have already might work though. While it's hard to imagine
>> someone not wanting search, perhaps other teams have a more
>> complicated search method. You are sharing/reusing the base
>> webservice, and you just want to implement a very simple search. The
>> other team works from the same base code, but extends the cookbook
>> with a much more complex search method that also incorporates ideas
>> that you don't need and don't want, like contributors, favorites,
>> and/or ratings.
>>
>
> After letting this marinate on my head for a while longer, I thought of
> creating an IHasComments interface that contributes a 'comments'
> attribute to IRecipe, as if there was a separate web service which wants
> to allow its users to comment on recipes
> (http://paste.ubuntu.com/472821/). How does that sound to you?

I like it.

You *could* show what I was just talking about above, if you wanted to, by setting up a separate webservice that added comments to the original webservice. It ought to be super easy--include the configuration of the original webservice plus your extra bits. If it's not easy then that might even be construed as a problem.

But that's not a requirement for this to land.

>
> I've also modified one of the tests for the base example to show the
> comments, but that uncovered a bug in the current implementation:
> EntryResource.redacted_fields will look for the 'comments' attribute on
> the IRecipe object directly instead of through its IHasComments adapter,
> and that causes a ForbiddenAttribute. I think I could change
> redacted_fields to adapt the context into the field's original interface
> before doing the checker.check(), but we currently don't have the
> original interface easily accessible at that point, so I'll have to
> think of something. I'll do that tomorrow.

Cool. Will comment separately

>
>> Can you use your patch and then add an explanation with an imaginary
>> scenario like the one I gave above?
>>
>> = Small suggestions =
>>
>> In generate_entry_adapters (src/lazr/restful/declarations.py) maybe
>> add a comment explaining that the update
>> ("fields.update(getFields(iface).items())") is not destructive because
>> we have already verified that there are no overlaps in
>> find_interfaces_and_contributors.
>
> In find_interfaces_and_contributors() we check for overlap in the fields
> exported by contributing interfaces, but here we're iterating over the
> interfaces for every different version of the webservice, and each of
> them already contains all the fields exported by the main interface and
> all of its contributors. That means there will be some overlapping
> names, but the field objects themselves will differ. Given that the
> field objects differ, we shouldn't need to use a set() there, so I've
> turned it into a list.

Great

>
>>
>> = Thoughts =
>>
>> AFAICT, saying that an exported interface contributes to another
>> interface effectively exports that other interface even if it were not
>> initially marked to be exported. I don't love that, but I don't think
>> it is a big deal, and I'm not going to worry about it.
>
> Not really -- it fails with a less than optimal error, so I've fixed it
> to raise a meaningful exception when that happens. This change has been
> pushed already.

Thank you

>
> Another thing to note is that I've bumped the version to 0.9.30 but I
> saw a branch from Benji changing it to 0.10-dev or something, so I was
> wondering if I should be using something else as the version?

trunk should have a "some-next-release-dev" version number. I think it's fine for you to do 0.9.30...or 0.10. I'd actually lean towards 0.10. Not sure why we've stuck in 0.9 for so long.

Gary

On Aug 3, 2010, at 5:46 PM, Guilherme Salgado wrote:

> On Tue, 2010-08-03 at 18:11 +0000, Gary Poster wrote:
>> This is really a very nice branch.  Thank you.
>> 
>> I hate to not give it a final go-ahead, but I do have one last request.
>> 
>> = Requests =
>> 
>> You said:
>>> Leonard said:
>>>> I would like to see this feature used in an example web service (ie.
>>>> added to examples/base or in a totally new web service of your
>>>> design). This is less for purposes of test coverage and more to show
>>>> how the feature would work in a real situation.
>>> 
>>> I moved some things in examples/base to make use of the new feature
>>> (http://paste.ubuntu.com/471195/), but I see no reason why people would
>>> use it like that in a simple interface.  In fact, since the need for
>>> this feature came from us wanting to split the implementation of some
>>> very complex classes into smaller ones, I find it hard to think of a
>>> good yet short example to use here.  Maybe you have an idea?
>> 
>> I'd argue that the central story is reuse.  That reuse might be to
>> factor out a simpler webservice that can be reused elsewhere (which is
>> actually what is driving your work, AIUI) or it might be to extend a
>> simpler webservice (which you might end up doing as well).  I think
>> you could easily explain a change in this way.
> 
> Well, what we really want is to be able to move attributes/methods out
> of existing interfaces and into adapters while still publicizing a
> single entry comprising what's exported by the changed interface and all
> of its adapters.  For the reuse story one can use mix-ins as long as
> it's ok to have the exported interface inherit from a bunch of others
> (as we currently do).

Right, but the mix-in approach means a lot more work for small changes.  I could be wrong, but with the mix-in class approach, you have to make your own separate registration of *everything* because you don't want one interface, you want another one.  There are examples of this kind of annoyance in the webservice package.  With your feature, you can reuse the entirety of a webservice, including its registrations, and then register your own things in addition.  That's much easier.

Maybe I'm missing something, but that's how I see it right now.

> 
>> 
>> If you didn't already have an example, I might try to think up some
>> scenario, like another branch of a company that wanted to add, say,
>> the idea of a recipe contributor, or wanted to add the idea of
>> favorite recipes or recipe ratings.  That particular idea might be
>> undesirable for the imaginary people using the base website.
>> 
>> What you have already might work though.  While it's hard to imagine
>> someone not wanting search, perhaps other teams have a more
>> complicated search method.  You are sharing/reusing the base
>> webservice, and you just want to implement a very simple search.  The
>> other team works from the same base code, but extends the cookbook
>> with a much more complex search method that also incorporates ideas
>> that you don't need and don't want, like contributors, favorites,
>> and/or ratings.
>> 
> 
> After letting this marinate on my head for a while longer, I thought of
> creating an IHasComments interface that contributes a 'comments'
> attribute to IRecipe, as if there was a separate web service which wants
> to allow its users to comment on recipes
> (http://paste.ubuntu.com/472821/).  How does that sound to you?

I like it.

You *could* show what I was just talking about above, if you wanted to, by setting up a separate webservice that added comments to the original webservice.  It ought to be super easy--include the configuration of the original webservice plus your extra bits.  If it's not easy then that might even be construed as a problem.

But that's not a requirement for this to land.

> 
> I've also modified one of the tests for the base example to show the
> comments, but that uncovered a bug in the current implementation:
> EntryResource.redacted_fields will look for the 'comments' attribute on
> the IRecipe object directly instead of through its IHasComments adapter,
> and that causes a ForbiddenAttribute.  I think I could change
> redacted_fields to adapt the context into the field's original interface
> before doing the checker.check(), but we currently don't have the
> original interface easily accessible at that point, so I'll have to
> think of something. I'll do that tomorrow.

Cool.  Will comment separately

> 
>> Can you use your patch and then add an explanation with an imaginary
>> scenario like the one I gave above?
>> 
>> = Small suggestions =
>> 
>> In generate_entry_adapters (src/lazr/restful/declarations.py) maybe
>> add a comment explaining that the update
>> ("fields.update(getFields(iface).items())") is not destructive because
>> we have already verified that there are no overlaps in
>> find_interfaces_and_contributors.
> 
> In find_interfaces_and_contributors() we check for overlap in the fields
> exported by contributing interfaces, but here we're iterating over the
> interfaces for every different version of the webservice, and each of
> them already contains all the fields exported by the main interface and
> all of its contributors.  That means there will be some overlapping
> names, but the field objects themselves will differ.  Given that the
> field objects differ, we shouldn't need to use a set() there, so I've
> turned it into a list.

Great

> 
>> 
>> = Thoughts =
>> 
>> AFAICT, saying that an exported interface contributes to another
>> interface effectively exports that other interface even if it were not
>> initially marked to be exported.  I don't love that, but I don't think
>> it is a big deal, and I'm not going to worry about it.
> 
> Not really -- it fails with a less than optimal error, so I've fixed it
> to raise a meaningful exception when that happens. This change has been
> pushed already.

Thank you

> 
> Another thing to note is that I've bumped the version to 0.9.30 but I
> saw a branch from Benji changing it to 0.10-dev or something, so I was
> wondering if I should be using something else as the version?

trunk should have a "some-next-release-dev" version number.  I think it's fine for you to do 0.9.30...or 0.10.  I'd actually lean towards 0.10.  Not sure why we've stuck in 0.9 for so long.

Gary

« Back to merge proposal