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

Proposed by James Westby on 2010-03-25
Status: Rejected
Rejected by: Leonard Richardson on 2010-04-05
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) 2010-03-26 Disapprove on 2010-04-05
Gary Poster 2010-03-25 Disapprove on 2010-03-26
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.
127. By James Westby on 2010-03-26

Fix long line in the doctest. Thanks Gary.

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

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

review: Disapprove

Unmerged revisions

127. By James Westby on 2010-03-26

Fix long line in the doctest. Thanks Gary.

126. By James Westby on 2010-03-25

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
1=== modified file 'src/lazr/restful/docs/webservice-marshallers.txt'
2--- src/lazr/restful/docs/webservice-marshallers.txt 2010-02-03 21:22:19 +0000
3+++ src/lazr/restful/docs/webservice-marshallers.txt 2010-03-26 15:42:28 +0000
4@@ -673,6 +673,26 @@
5 >>> print reference_marshaller.marshall_from_json_data(None)
6 None
7
8+If you try and unmarshall an object with an object marshaller, but the
9+associated field is not declared to be an object, say it is a Choice
10+rather than a ReferenceChoice, you get an error. This is to prevent a
11+discrepancy between what the tales take to be a _link field, and the name
12+the marshaller will give to the field.
13+
14+ >>> from lazr.restful.marshallers import ObjectLookupFieldMarshaller
15+ >>> reference_field = Choice(__name__='cuisine', vocabulary=Cuisine)
16+ >>> reference_marshaller = ObjectLookupFieldMarshaller(
17+ ... reference_field, request, vocabulary=Cuisine)
18+ >>> verifyObject(IFieldMarshaller, reference_marshaller)
19+ True
20+
21+ >>> cookbook = COOKBOOKS[0]
22+ >>> reference_marshaller.unmarshall(None, cookbook) # doctest: +NORMALIZE_WHITESPACE
23+ Traceback (most recent call last):
24+ ...
25+ ValueError: cuisine is not defined to be an object field.
26+ Must be one of IObject, IBytes or IReferenceChoice.
27+
28 Collections
29 -----------
30
31
32=== modified file 'src/lazr/restful/fields.py'
33--- src/lazr/restful/fields.py 2009-03-26 18:47:31 +0000
34+++ src/lazr/restful/fields.py 2010-03-26 15:42:28 +0000
35@@ -11,13 +11,18 @@
36
37 from zope.interface import implements
38 from zope.schema import Choice, Field, Object
39-from zope.schema.interfaces import IObject, SchemaNotProvided
40+from zope.schema.interfaces import IBytes, IObject, SchemaNotProvided
41 from zope.schema._field import AbstractCollection
42
43 from lazr.restful.interfaces import (
44 ICollectionField, IReference, IReferenceChoice)
45
46
47+def is_link_field(field):
48+ return (IObject.providedBy(field) or IBytes.providedBy(field)
49+ or IReferenceChoice.providedBy(field))
50+
51+
52 class CollectionField(AbstractCollection):
53 """A collection associated with an entry."""
54 # We subclass AbstractCollection instead of List because List
55
56=== modified file 'src/lazr/restful/marshallers.py'
57--- src/lazr/restful/marshallers.py 2009-11-10 13:58:22 +0000
58+++ src/lazr/restful/marshallers.py 2010-03-26 15:42:28 +0000
59@@ -39,6 +39,7 @@
60
61 from lazr.restful.interfaces import (
62 IFieldMarshaller, IUnmarshallingDoesntNeedValue, IWebServiceConfiguration)
63+from lazr.restful.fields import is_link_field
64 from lazr.restful.utils import safe_hasattr
65
66
67@@ -525,6 +526,10 @@
68
69 Represent an object as the URL to that object
70 """
71+ if not is_link_field(self.field):
72+ raise ValueError("%s is not defined to be an object field. "
73+ "Must be one of IObject, IBytes or IReferenceChoice."
74+ % self.field.__name__)
75 repr_value = None
76 if value is not None:
77 repr_value = absoluteURL(value, self.request)
78
79=== modified file 'src/lazr/restful/tales.py'
80--- src/lazr/restful/tales.py 2010-03-10 20:44:03 +0000
81+++ src/lazr/restful/tales.py 2010-03-26 15:42:28 +0000
82@@ -32,8 +32,9 @@
83 ICollection, ICollectionField, IEntry, IJSONRequestCache,
84 IReferenceChoice, IResourceDELETEOperation, IResourceGETOperation,
85 IResourceOperation, IResourcePOSTOperation, IScopedCollection,
86- ITopLevelEntryLink, IWebServiceClientRequest, IWebServiceConfiguration,
87+ ITopLevelEntryLink, IWebServiceConfiguration,
88 IWebServiceVersion, LAZR_WEBSERVICE_NAME)
89+from lazr.restful.fields import is_link_field
90 from lazr.restful.utils import get_current_web_service_request
91
92
93@@ -455,8 +456,7 @@
94 name = self.field.__name__
95 if ICollectionField.providedBy(self.field):
96 return name + '_collection_link'
97- elif (IObject.providedBy(self.field) or IBytes.providedBy(self.field)
98- or IReferenceChoice.providedBy(self.field)):
99+ elif is_link_field(self.field):
100 return name + '_link'
101 else:
102 return name

Subscribers

People subscribed via source and target branches