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

[James Westby]

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

Hi,

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

Thanks,

James

[Gary Poster (gary)]

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

=== modified file 'lib/canonical/launchpad/doc/webservice-marshallers.txt'
--- lib/canonical/launchpad/doc/webservice-marshallers.txt	2010-03-25 20:36:46 +0000
+++ lib/canonical/launchpad/doc/webservice-marshallers.txt	2010-04-09 14:14:24 +0000
@@ -31,6 +31,7 @@
 
     >>> from canonical.launchpad.interfaces import IPerson
     >>> from zope.component import getMultiAdapter
+    >>> from zope.schema import Choice
     >>> from lazr.restful.fields import ReferenceChoice
     >>> from lazr.restful.interfaces import IFieldMarshaller
     >>> from lazr.restful import EntryResource
@@ -84,6 +85,21 @@
     >>> marshaller.representation_name
     'some_person_link'
 
+If you export a Choice that uses an SQLObjectVocabularyBase then you
+get an error, as you should be using a ReferenceChoice instead to
+ensure that the resulting wadl matches lazr.restful conventions.
+
+    >>> field = Choice(
+    ...    __name__='some_person', vocabulary='ValidPersonOrTeam')
+    >>> field = field.bind(None)
+    >>> getMultiAdapter((field, request), IFieldMarshaller)
+    ... # doctest: +NORMALIZE_WHITESPACE
+    Traceback (most recent call last):
+    ...
+    AssertionError: You exported some_person as an IChoice based on an
+    SQLObjectVocabularyBase, you should use
+    lazr.restful.fields.ReferenceChoice instead.
+
 Cleanup.
 
     >>> request.oopsid = None

=== added file 'lib/canonical/launchpad/webapp/marshallers.py'
--- lib/canonical/launchpad/webapp/marshallers.py	1970-01-01 00:00:00 +0000
+++ lib/canonical/launchpad/webapp/marshallers.py	2010-04-09 14:14:24 +0000
@@ -0,0 +1,21 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+
+__metaclass__ = type
+__all__ = [
+    'choiceMarshallerError'
+    ]
+
+
+def choiceMarshallerError(field, request, vocabulary=None):
+    # We don't support marshalling a normal Choice field with a
+    # SQLObjectVocabularyBase-based vocabulary.
+    # Normally for this kind of use case, one returns None and
+    # lets the Zope machinery alert the user that the lookup has gone wrong.
+    # However, we want to be more helpful, so we make an assertion,
+    # with a comment on how to make things better.
+    raise AssertionError("You exported %s as an IChoice based on an "
+                         "SQLObjectVocabularyBase, you should use "
+                         "lazr.restful.fields.ReferenceChoice instead."
+                         % field.__name__)

=== modified file 'lib/canonical/launchpad/zcml/webservice.zcml'
--- lib/canonical/launchpad/zcml/webservice.zcml	2010-02-15 13:58:03 +0000
+++ lib/canonical/launchpad/zcml/webservice.zcml	2010-04-09 14:14:24 +0000
@@ -63,6 +63,13 @@
             zope.publisher.interfaces.http.IHTTPRequest
             canonical.launchpad.webapp.vocabulary.SQLObjectVocabularyBase"
        provides="lazr.restful.interfaces.IFieldMarshaller"
+       factory="canonical.launchpad.webapp.marshallers.choiceMarshallerError"
+       />
+   <adapter
+       for="lazr.restful.interfaces.IReferenceChoice
+            zope.publisher.interfaces.http.IHTTPRequest
+            canonical.launchpad.webapp.vocabulary.SQLObjectVocabularyBase"
+       provides="lazr.restful.interfaces.IFieldMarshaller"
        factory="lazr.restful.marshallers.ObjectLookupFieldMarshaller"
        />