© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
67d34a1
(Get the code!)
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/
- we make our own adapter for
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
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:/
[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_
[11:37am] gary_poster: james_w: >>> reference_
[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_
[11:38am] gary_poster: ... # doctest: +NORMALIZE_
[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/
[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/
[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.
[12:10pm] gary_poster: zope.publisher.
[12:10pm] gary_poster: canonical.
[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.
[12:12pm] gary_poster: zope.publisher.
[12:12pm] gary_poster: canonical.
[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