lazr.restful

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

extension-interfaces
Merge into trunk

Revision history for this message

Guilherme Salgado (salgado) wrote on 2010-08-03:

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).

>
> 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'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.

> 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.

>
> = 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.

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?

--
Guilherme Salgado <https://launchpad.net/~salgado>

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).

> 
> 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.
>

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.

> 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.

> 
> = 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.

-- 
Guilherme Salgado <https://launchpad.net/~salgado>

« Back to merge proposal