lazr.restful

Merge lp:~james-w/lazr.restful/object-unmarshal into lp:lazr.restful

  1. object-unmarshal
  2. Merge into trunk
Proposed by James Westby
Status: Rejected
Rejected by: Leonard Richardson
Proposed branch: lp:~james-w/lazr.restful/object-unmarshal
Merge into: lp:lazr.restful
Diff against target: 102 lines (+34/-4)
4 files modified
src/lazr/restful/docs/webservice-marshallers.txt (+20/-0)
src/lazr/restful/fields.py (+6/-1)
src/lazr/restful/marshallers.py (+5/-0)
src/lazr/restful/tales.py (+3/-3)
To merge this branch: bzr merge lp:~james-w/lazr.restful/object-unmarshal
Reviewer Review Type Date Requested Status
Leonard Richardson (community) Disapprove
Gary Poster Disapprove
Review via email: mp+22182@code.launchpad.net

Description of the change

Hi,

This is a fix to stop bugs like bug 546558 from happening again.

Launchpad defines an adapter that makes lazr.restful use
the Object marshaller for every Choice with an SQL-based vocabulary.
However, it doesn't do anything to turn those Choices in to
ReferenceChoices automatically.

tales.py in lazr.restful uses the Interface of the field to determine
whether to add _link to the name, but the Object marshaller does
it unconditionally.

Throwing the error at unmarshall time isn't pretty, but there is a
comment in tales.py that we can't do it at that point. It could be
done at marshaller creation time instead.

An alternative would be for the Object marshaller to choose whether
to append _link depending on the type as tales.py does, but that
wouldn't enforce the _link convention.

Thanks,

James

To post a comment you must log in.
lp:~james-w/lazr.restful/object-unmarshal updated
127. By James Westby

Fix long line in the doctest. Thanks Gary.

Revision history for this message
Gary Poster (gary) wrote :
Download full text (8.8 KiB)

Summary of IRC conversation:

- I think that Launchpad is abusing the Object field marshaller by using it to marshall a Choice field and that LP should simply have better registrations (details below).
- I don't think lazr.restful should protect from this abuse, because things can be abused in too many ways, and trying to protect this one in particular makes no sense to me in that context.
- Despite my disapproval, I'm happy to go along with Leonard's review, if James convinces him to make a change like this one.

These are the "better registrations" I suggest. In lib/canonical/launchpad/zcml/webservice.zcml ...

- we make our own adapter for
          "zope.schema.interfaces.IChoice
           zope.publisher.interfaces.http.IHTTPRequest
           canonical.launchpad.webapp.vocabulary.SQLObjectVocabularyBase"
 that either returns None (causing Zope to say that there is no adaptation available) or that raises an error (that says something like "Use IReferenceChoice, not IChoice, silly person!"); and
- we change the current registration to
          "lazr.restful.interfaces.IReferenceChoice
           zope.publisher.interfaces.http.IHTTPRequest
           canonical.launchpad.webapp.vocabulary.SQLObjectVocabularyBase"

That should accomplish the same thing but within the Launchpad codebase, where it belongs, I argue.

James made two related good points that seem valid to me:

- "the root cause is that there are two different things, using two different criteria, deciding what the field name should be in the API. When they disagree you get problems."
- "A secondary concern is that we want exported objects to be serialised to links, and when that is done for the field name to have _link appended."

Here's the IRC log, for the die-hard reader.

[11:27am] james_w: gary_poster: here's what I was talking about yesterday: https://code.edge.launchpad.net/~james-w/lazr.restful/object-unmarshal/+merge/22182
[11:28am] • gary_poster looks
[11:29am] gary_poster: james_w: do you want a review?
[11:29am] gary_poster: rephrase:
[11:29am] gary_poster: do you want me to review
[11:29am] james_w: yes please
[11:30am] gary_poster: ok
[11:30am] james_w: I'd like leonard to look as well
[11:30am] james_w: I'm not entirely convinced by the approach as it isn't a great example of elegance, and prevents you doing some odd but legal things.
[11:30am] james_w: but I can't find a better way
[11:32am] gary_poster: james_w: leonard will not be able to before this release (not around today). I agree that he should review. Because he is not around today, this won't make it into the cycle unless we push it as release-critical. My gut feeling so far is that this is more important for developing (that is, future work) that for production (that is, this release). Do you agree?
[11:33am] gary_poster: (IOW, the fact that this will not make the release does not seem like a release-critical problem; but we should get it into the devel branch asap)
[11:33am] james_w: gary_poster: yes, sinzui landed the branch that fixes the problems yesterday, this is to stop any more from creeping in.
[11:34am] gary_poster: james_w: cool. I'll look now to see if I have anything to add, and ma...

Read more...

review: Disapprove
Revision history for this message
Leonard Richardson (leonardr) wrote :

I agree with Gary. It's problematic that we have two different criteria for deciding whether a field name should end in "_link" or "_collection_link" or nothing at all, and a branch that fixed that would go into lazr.restful. But this problem should be fixed in Launchpad, and though I don't totally understand Gary's proposed solution it seems like the kind of thing Launchpad needs.

Revision history for this message
James Westby (james-w) wrote :

On Thu, 01 Apr 2010 15:40:02 -0000, Leonard Richardson <email address hidden> wrote:
> I agree with Gary. It's problematic that we have two different criteria for deciding whether a field name should end in "_link" or "_collection_link" or nothing at all, and a branch that fixed that would go into lazr.restful. But this problem should be fixed in Launchpad, and though I don't totally understand Gary's proposed solution it seems like the kind of thing Launchpad needs.

That change is already approved for landing in to LP, just waiting for
PQM to open.

This merge proposal still stands as just that, a proposal. Please reject
it if you don't want to take this approach.

Thanks,

James

Revision history for this message
Leonard Richardson (leonardr) :
review: Disapprove

Unmerged revisions

127. By James Westby

Fix long line in the doctest. Thanks Gary.

126. By James Westby

Test that an ObjectLookupFieldMarshaller without an object field fails to unmarshal.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'src/lazr/restful/docs/webservice-marshallers.txt'
--- src/lazr/restful/docs/webservice-marshallers.txt 2010-02-03 21:22:19 +0000
+++ src/lazr/restful/docs/webservice-marshallers.txt 2010-03-26 15:42:28 +0000
@@ -673,6 +673,26 @@
673 >>> print reference_marshaller.marshall_from_json_data(None)673 >>> print reference_marshaller.marshall_from_json_data(None)
674 None674 None
675675
676If you try and unmarshall an object with an object marshaller, but the
677associated field is not declared to be an object, say it is a Choice
678rather than a ReferenceChoice, you get an error. This is to prevent a
679discrepancy between what the tales take to be a _link field, and the name
680the marshaller will give to the field.
681
682 >>> from lazr.restful.marshallers import ObjectLookupFieldMarshaller
683 >>> reference_field = Choice(__name__='cuisine', vocabulary=Cuisine)
684 >>> reference_marshaller = ObjectLookupFieldMarshaller(
685 ... reference_field, request, vocabulary=Cuisine)
686 >>> verifyObject(IFieldMarshaller, reference_marshaller)
687 True
688
689 >>> cookbook = COOKBOOKS[0]
690 >>> reference_marshaller.unmarshall(None, cookbook) # doctest: +NORMALIZE_WHITESPACE
691 Traceback (most recent call last):
692 ...
693 ValueError: cuisine is not defined to be an object field.
694 Must be one of IObject, IBytes or IReferenceChoice.
695
676Collections696Collections
677-----------697-----------
678698
679699
=== modified file 'src/lazr/restful/fields.py'
--- src/lazr/restful/fields.py 2009-03-26 18:47:31 +0000
+++ src/lazr/restful/fields.py 2010-03-26 15:42:28 +0000
@@ -11,13 +11,18 @@
1111
12from zope.interface import implements12from zope.interface import implements
13from zope.schema import Choice, Field, Object13from zope.schema import Choice, Field, Object
14from zope.schema.interfaces import IObject, SchemaNotProvided14from zope.schema.interfaces import IBytes, IObject, SchemaNotProvided
15from zope.schema._field import AbstractCollection15from zope.schema._field import AbstractCollection
1616
17from lazr.restful.interfaces import (17from lazr.restful.interfaces import (
18 ICollectionField, IReference, IReferenceChoice)18 ICollectionField, IReference, IReferenceChoice)
1919
2020
21def is_link_field(field):
22 return (IObject.providedBy(field) or IBytes.providedBy(field)
23 or IReferenceChoice.providedBy(field))
24
25
21class CollectionField(AbstractCollection):26class CollectionField(AbstractCollection):
22 """A collection associated with an entry."""27 """A collection associated with an entry."""
23 # We subclass AbstractCollection instead of List because List28 # We subclass AbstractCollection instead of List because List
2429
=== modified file 'src/lazr/restful/marshallers.py'
--- src/lazr/restful/marshallers.py 2009-11-10 13:58:22 +0000
+++ src/lazr/restful/marshallers.py 2010-03-26 15:42:28 +0000
@@ -39,6 +39,7 @@
3939
40from lazr.restful.interfaces import (40from lazr.restful.interfaces import (
41 IFieldMarshaller, IUnmarshallingDoesntNeedValue, IWebServiceConfiguration)41 IFieldMarshaller, IUnmarshallingDoesntNeedValue, IWebServiceConfiguration)
42from lazr.restful.fields import is_link_field
42from lazr.restful.utils import safe_hasattr43from lazr.restful.utils import safe_hasattr
4344
4445
@@ -525,6 +526,10 @@
525526
526 Represent an object as the URL to that object527 Represent an object as the URL to that object
527 """528 """
529 if not is_link_field(self.field):
530 raise ValueError("%s is not defined to be an object field. "
531 "Must be one of IObject, IBytes or IReferenceChoice."
532 % self.field.__name__)
528 repr_value = None533 repr_value = None
529 if value is not None:534 if value is not None:
530 repr_value = absoluteURL(value, self.request)535 repr_value = absoluteURL(value, self.request)
531536
=== modified file 'src/lazr/restful/tales.py'
--- src/lazr/restful/tales.py 2010-03-10 20:44:03 +0000
+++ src/lazr/restful/tales.py 2010-03-26 15:42:28 +0000
@@ -32,8 +32,9 @@
32 ICollection, ICollectionField, IEntry, IJSONRequestCache,32 ICollection, ICollectionField, IEntry, IJSONRequestCache,
33 IReferenceChoice, IResourceDELETEOperation, IResourceGETOperation,33 IReferenceChoice, IResourceDELETEOperation, IResourceGETOperation,
34 IResourceOperation, IResourcePOSTOperation, IScopedCollection,34 IResourceOperation, IResourcePOSTOperation, IScopedCollection,
35 ITopLevelEntryLink, IWebServiceClientRequest, IWebServiceConfiguration,35 ITopLevelEntryLink, IWebServiceConfiguration,
36 IWebServiceVersion, LAZR_WEBSERVICE_NAME)36 IWebServiceVersion, LAZR_WEBSERVICE_NAME)
37from lazr.restful.fields import is_link_field
37from lazr.restful.utils import get_current_web_service_request38from lazr.restful.utils import get_current_web_service_request
3839
3940
@@ -455,8 +456,7 @@
455 name = self.field.__name__456 name = self.field.__name__
456 if ICollectionField.providedBy(self.field):457 if ICollectionField.providedBy(self.field):
457 return name + '_collection_link'458 return name + '_collection_link'
458 elif (IObject.providedBy(self.field) or IBytes.providedBy(self.field)459 elif is_link_field(self.field):
459 or IReferenceChoice.providedBy(self.field)):
460 return name + '_link'460 return name + '_link'
461 else:461 else:
462 return name462 return name

Subscribers

People subscribed via source and target branches