Merge into trunk : object-unmarshal : Code : lazr.restful

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
Related bugs:	Link a bug report

Reviewer	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

lp:~james-w/lazr.restful/object-unmarshal updated on 2010-03-26

127. By James Westby on 2010-03-26: Fix long line in the doctest. Thanks Gary.

Revision history for this message

Gary Poster (gary) wrote on 2010-03-26:

#

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

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 make sure that reviewing this branch is in leonardr's to-do list.
[11:34am] james_w: thanks
[11:35am] james_w: there's a long line in the doctest that I don't know how to wrap
[11:37am] gary_poster: james_w: you can use #doctest: +ELLIPSIS and elide the text with an ellipsis. #doctest: +NORMALIZE_WHITESPACE might let you wrap the line, but IME sometimes tracebacks are "exceptions" to the rule.  A-ha-ha.  But you could try that first.
[11:37am] gary_poster: james_w: >>> reference_marshaller.unmarshall(None, cookbook) # doctest: +NORMALIZE_WHITESPACE
[11:37am] james_w: that goes on the test line, or on the response line?
[11:38am] james_w: ah, thanks
[11:38am] gary_poster: or
[11:38am] gary_poster: >>> reference_marshaller.unmarshall(None, cookbook)
[11:38am] gary_poster: ... # doctest: +NORMALIZE_WHITESPACE
[11:40am] james_w: fixed, thanks
[11:40am] gary_poster: cool, np
[11:57am] gary_poster: james_w: yeah, that
[11:57am] gary_poster: 's not fun.  Can you tell me where the adapter is that you mention in the cover notes?
[11:58am] gary_poster: That uses an Object marshaller for a Choice?
[11:58am] gary_poster: (with the certain kind of vocab)
[11:58am] james_w: some webservice zcml
[11:58am] james_w: lib/canonical/launchpad/zcml/webservice.zcml
[11:58am] gary_poster: ack thanks
[11:59am] james_w: I thought the most elegant way to solve it would be for tales.py to just add _link if it would just an ObjectMarshaller for that field, but there's already a comment there saying it can't do that
[12:00pm] gary_poster: james_w: My thought would have been that using an ObjectMarshaller is an abuse.  We should write our own marshaller for this case that does the right thing.
[12:01pm] gary_poster: Does the tales comment (which I have not yet looked up) contradict that?  Going to hunt down tales comment...
[12:01pm] james_w: search for Marshaller in src/lazr/restful/tales.py
[12:02pm] james_w: I'm not sure what "the right thing" for the new marshaller to do would be
[12:04pm] james_w: so, 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.
[12:05pm] gary_poster: james_w: a minimal "right thing" would be to puke, as you are doing here.  Puking where you have it now (in lazr.restful) doesn't make sense because, looking at the code, why would an ObjectMarshaller ever be called with anything other than an IObject?  It shouldn't.
[12:05pm] james_w: 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.
[12:05pm] gary_poster: root cause: that sounds valid, but it's been too long since I looked at the code for me to have an informed opinion without staring for awhile more.
[12:06pm] james_w: gary_poster: right, it never is. The issue is when that Marshaller isn't bound to an "object" field type, the name is already wrong in the wadl.
[12:06pm] james_w: so I'm fixing the wrong end in effect, as the problem is already past, but this barfs at you and tells you something is wrong at least, rather than silently doing the wrong thing.
[12:07pm] james_w: I couldn't see how to fix it at the "other end", which would be the field declaration and tales.py, as we need the developer to indicate whether it is a field that references another exported object or not.
[12:07pm] james_w: which they do by using ReferenceChoice instead of Choice
[12:09pm] james_w: perhaps the right thing to do is change the adapter declaration so that it only affects IReferenceChoices?
[12:09pm] gary_poster: james_w: right, which is why I called it an abuse.  We can't protect ourselves from all abuses in lazr.restful.  Here's my proposal for your current motivations.  Since I have this alternative, I would not approve your approach.  Leonard may have an even better answer.
[12:10pm] gary_poster: - we make our own adapter for
[12:10pm] gary_poster:       for="zope.schema.interfaces.IChoice
[12:10pm] gary_poster:             zope.publisher.interfaces.http.IHTTPRequest
[12:10pm] gary_poster:             canonical.launchpad.webapp.vocabulary.SQLObjectVocabularyBase"
[12:11pm] gary_poster: that says "this is broken!" or some other pukage
[12:12pm] gary_poster: - we change the current registration to
[12:12pm] gary_poster:       for="lazr.restful.interfaces.IReferenceChoice
[12:12pm] gary_poster:             zope.publisher.interfaces.http.IHTTPRequest
[12:12pm] gary_poster:             canonical.launchpad.webapp.vocabulary.SQLObjectVocabularyBase"
[12:12pm] gary_poster: Done.
[12:12pm] gary_poster: Actually, the first step could be to return None
[12:12pm] james_w: and zope knows to pick the more specific adapter?
[12:13pm] gary_poster: but then we could provide a helpful error ("Use IReferenceChoice, silly person!")
[12:13pm] gary_poster: I mean, couldn't
[12:13pm] gary_poster: yes
[12:13pm] james_w: right, that makes sense
[12:14pm] gary_poster: james_w: I'll copy some edited version of this over to the MP for leonardr, and move on.  s'ok?
[12:14pm] james_w: yep
[12:14pm] gary_poster: cool.  thank you for doing this.
[12:14pm] james_w: I'll work on that after lunch
[12:14pm] gary_poster: great
[12:15pm] james_w: I think that Leonard may still want to consider a change to lazr.restful, as the discrepancy could still bite people using it.
[12:15pm] james_w: but this is the right fix for LP as the bad registration is what is causing the problem.
[12:16pm] gary_poster: lazr.restful change: OK.  I argue against it because of the "can't stop every abuse" argument, but if you convince me I'll go placidly along. 
[12:16pm] gary_poster: I mean, convince him
[12:17pm] james_w: sure, it's not so much about abuse. I wasn't the one that put the comment identifying the issue in tales.py after all

review: Disapprove

Revision history for this message

Leonard Richardson (leonardr) wrote on 2010-04-01:

#

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 2010-04-01:

#

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) on 2010-04-05:

#

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.

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

lazr.restful

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

Commit message

Description of the change

Unmerged revisions

Preview Diff

Subscribers