Merge lp:~james-w/launchpad/export-code-import into lp:launchpad

Proposed by James Westby
Status: Merged
Approved by: Gary Poster
Approved revision: no longer in the source branch.
Merged at revision: not available
Proposed branch: lp:~james-w/launchpad/export-code-import
Merge into: lp:launchpad
Diff against target: 76 lines (+44/-0)
3 files modified
lib/canonical/launchpad/doc/webservice-marshallers.txt (+16/-0)
lib/canonical/launchpad/webapp/ (+21/-0)
lib/canonical/launchpad/zcml/webservice.zcml (+7/-0)
To merge this branch: bzr merge lp:~james-w/launchpad/export-code-import
Reviewer Review Type Date Requested Status
Gary Poster (community) Approve
Review via email:

Commit message

Exporting an IChoice based on an SQL vocabulary will now error rather than silently doing the wrong thing.

Description of the change


This fixes the root cause of repeated problems with
exporting IChoices for them not to be available over the



To post a comment you must log in.
Revision history for this message
Gary Poster (gary) wrote :
Download full text (3.2 KiB)

Thank you very much for the fix and the changes.

merge-conditional on adding the comment as we discussed.

And now, I present the IRC log.

gary_poster: james_w: Did you consider AssertionError over ValueError? I was gung ho for AssertionError, but then started convincing myself of ValueError. I still lean towards AssertionError...ooh, no I have an idea I like better, maybe. One sec, looking up.
[2:55pm] james_w: I don't really mind what the error type is
[3:05pm] gary_poster: OK, james_w, here is the trivia I'm struggling with. Normally, the right thing to do in this case is to write a function (I'm going to suggest that we switch the class to a function anyway) that returns None. That behaves properly with standard Zope calls: getMultiAdapter will raise a ComponentLookupError, and queryMultiAdapter will return None or the provided default.
[3:05pm] gary_poster: By explicitly raising an exception we're breaking the pattern. Raising ComponentLookupError (what I was considering) is not appropriate here because that's the responsibility of something higher up. And the reason we are doing this is to help the user figure out what they did wrong. In would be nice if the zope machinery had a spelling for that, but it doesn't.
[3:05pm] gary_poster: So, I think an AssertionError is the right thing to do for our intentions: we're doing this to help a poor developer debug and fix their problems, which is the domain of an AssertionError. A ValueError is an attempt to raise a "real" error, and would be the wrong "real" error (ComponentLookupError is the right error, if we get one at all).
[3:05pm] gary_poster: So with all that, I would suggest changing the ChoiceErrorMarshaller class to something like this:
[3:05pm] gary_poster: def choiceMarshallerError(field, request, vocabulary):
[3:05pm] gary_poster: # We don't support marshalling a normal Choice field with a
[3:05pm] gary_poster: # SQLObjectVocabularyBase-based vocabulary.
[3:05pm] gary_poster: # Normally for this kind of use case, one returns None and
[3:05pm] gary_poster: # lets the Zope machinery alert the user that the lookup has gone wrong.
[3:05pm] gary_poster: # However, we want to be more helpful, so we make an assertion,
[3:05pm] gary_poster: # with a comment on how to make things better.
[3:05pm] gary_poster: assert False, (...your message...)
[3:06pm] james_w: ok
[3:06pm] gary_poster: james_w: other than that, r=gary. You cool with that?
[3:07pm] james_w: yeah, let me make the changes so that you can confirm
[3:07pm] gary_poster: ok cool
[3:07pm] lantash3 joined the chat room.
[3:11pm] danilos joined the chat room.
[3:12pm] deryck left the chat room. (Quit: Leaving)
[3:15pm] james_w: gary_poster: please re-check
[3:15pm] gary_poster: ack james_w
[3:16pm] lantash left the chat room. (Quit: Leaving.)
[3:17pm] gary_poster: james_w: +1. I really would like (something like) the comment I gave to be there too. People who know the Zope pattern will appreciate reading what's going on (like me, when I've forgotten all about this!)
[3:17pm] james_w: oh
[3:18pm] gary_poster: Other than that, that's what I had in mind. I need to step out to the bank. Would ...


review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/launchpad/doc/webservice-marshallers.txt'
2--- lib/canonical/launchpad/doc/webservice-marshallers.txt 2010-03-25 20:36:46 +0000
3+++ lib/canonical/launchpad/doc/webservice-marshallers.txt 2010-04-09 14:14:24 +0000
4@@ -31,6 +31,7 @@
6 >>> from canonical.launchpad.interfaces import IPerson
7 >>> from zope.component import getMultiAdapter
8+ >>> from zope.schema import Choice
9 >>> from lazr.restful.fields import ReferenceChoice
10 >>> from lazr.restful.interfaces import IFieldMarshaller
11 >>> from lazr.restful import EntryResource
12@@ -84,6 +85,21 @@
13 >>> marshaller.representation_name
14 'some_person_link'
16+If you export a Choice that uses an SQLObjectVocabularyBase then you
17+get an error, as you should be using a ReferenceChoice instead to
18+ensure that the resulting wadl matches lazr.restful conventions.
20+ >>> field = Choice(
21+ ... __name__='some_person', vocabulary='ValidPersonOrTeam')
22+ >>> field = field.bind(None)
23+ >>> getMultiAdapter((field, request), IFieldMarshaller)
24+ ... # doctest: +NORMALIZE_WHITESPACE
25+ Traceback (most recent call last):
26+ ...
27+ AssertionError: You exported some_person as an IChoice based on an
28+ SQLObjectVocabularyBase, you should use
29+ lazr.restful.fields.ReferenceChoice instead.
31 Cleanup.
33 >>> request.oopsid = None
35=== added file 'lib/canonical/launchpad/webapp/'
36--- lib/canonical/launchpad/webapp/ 1970-01-01 00:00:00 +0000
37+++ lib/canonical/launchpad/webapp/ 2010-04-09 14:14:24 +0000
38@@ -0,0 +1,21 @@
39+# Copyright 2010 Canonical Ltd. This software is licensed under the
40+# GNU Affero General Public License version 3 (see the file LICENSE).
43+__metaclass__ = type
44+__all__ = [
45+ 'choiceMarshallerError'
46+ ]
49+def choiceMarshallerError(field, request, vocabulary=None):
50+ # We don't support marshalling a normal Choice field with a
51+ # SQLObjectVocabularyBase-based vocabulary.
52+ # Normally for this kind of use case, one returns None and
53+ # lets the Zope machinery alert the user that the lookup has gone wrong.
54+ # However, we want to be more helpful, so we make an assertion,
55+ # with a comment on how to make things better.
56+ raise AssertionError("You exported %s as an IChoice based on an "
57+ "SQLObjectVocabularyBase, you should use "
58+ "lazr.restful.fields.ReferenceChoice instead."
59+ % field.__name__)
61=== modified file 'lib/canonical/launchpad/zcml/webservice.zcml'
62--- lib/canonical/launchpad/zcml/webservice.zcml 2010-02-15 13:58:03 +0000
63+++ lib/canonical/launchpad/zcml/webservice.zcml 2010-04-09 14:14:24 +0000
64@@ -63,6 +63,13 @@
65 zope.publisher.interfaces.http.IHTTPRequest
66 canonical.launchpad.webapp.vocabulary.SQLObjectVocabularyBase"
67 provides="lazr.restful.interfaces.IFieldMarshaller"
68+ factory="canonical.launchpad.webapp.marshallers.choiceMarshallerError"
69+ />
70+ <adapter
71+ for="lazr.restful.interfaces.IReferenceChoice
72+ zope.publisher.interfaces.http.IHTTPRequest
73+ canonical.launchpad.webapp.vocabulary.SQLObjectVocabularyBase"
74+ provides="lazr.restful.interfaces.IFieldMarshaller"
75 factory="lazr.restful.marshallers.ObjectLookupFieldMarshaller"
76 />