Roundup Tracker - Issues

Issue 2550830

classification
Title: An empty LinkHTMLProperty cannot be compared successfully
Type: behavior Severity: normal
Components: Versions:
process
Status: fixed Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: ber, r.david.murray
Priority: Keywords: patch

Created on 2014-03-05 20:21 by r.david.murray, last changed 2014-03-12 16:49 by ber.

Files
File name Uploaded Description Edit Remove
linkhtmlproperty.patch r.david.murray, 2014-03-06 20:06
Messages
msg4994 Author: [hidden] (r.david.murray) Date: 2014-03-05 20:21
I ran across this fixing the classic query.edit.html template.  There is
a snippet that does:

  query.private_for == None

This is never true, even if the _value of private_for is None.  The
reason is that because the HTMLProperty classes are old style classes,
python looks for an __eq__ method and, when it doesn't find one (because
neither LinkHTMLProperty nor any of its superclasses define one)
LinkHTMLProperty's __getattr__ is called.  That method does:

    if not self._value:
        <construct MissingValue object and look up attr on it)

This is obviously never going to equal None.

I can fix the template by doing 'not query.private_for', since
__nonzero__ is defined by a subclass, but it seems to me that this is a
bug in LinkHTMLProperty.

This could be fixed by making LinkHTMLProperty a new-style class, but I
don't know if that would have any unexpected consequences.
msg4995 Author: [hidden] (ber) Date: 2014-03-06 08:15
David, 
thanks for reporting the issue!

Can you say more about the consequences this defect has?
As for the consequences of changing the LinkHTMLProperty
class: If it is easy, you could try doing the chance and 
ran the included testsuite.

Best,
Bernhard
msg4996 Author: [hidden] (r.david.murray) Date: 2014-03-06 13:49
Well, in the specific case of the query.edit page, the consequence was
that all queries were shown as 'private', whether they were or not, and
then when the form was submitted for any reason, all queries were set to
private, whether they were before or not, unless you had changed them
back before submitting.

In general the problem is that if a LinkHTMLProperty is None, and you
try to *test* that it is None using == (you can't use 'is', obviously,
which is a different but unavoidable problem), things don't work as you
expect, and (the big problem) the failure is silent.

I will try the test suite when I have time (later today, with any luck).
msg4998 Author: [hidden] (r.david.murray) Date: 2014-03-06 20:06
With the attached patch applied, all unit tests pass.  I looked at the
unit tests a bit, but it isn't at all obvious how I'd write a test for
this using the current infrastructure.  I mean, it looks like it is
quite possible, but I don't know enough to do it.
msg4999 Author: [hidden] (ber) Date: 2014-03-07 08:04
David,
thanks for the patch!
I admin I probably have to look up a bit before I can check that
it is helpful, but maybe we'll find somebody else to review it
on the list.

As for the unit tests: Usually I copy one of the existing test files
and adapt them. 

Best,
Bernhard
msg5006 Author: [hidden] (ber) Date: 2014-03-10 11:32
To switch over from Issue2550831:
David,
Yes, it would be cool to now that this change also got some production
exposure.
msg5015 Author: [hidden] (ber) Date: 2014-03-11 14:20
I'm peeking a look in the code trying to understand the impliciation
and if this is the best fix.

Am I right that an alternative solution would be to implement __eq__
and keep the old-style class?

Because this probably would be the only new style class in that area
and I wonder about consistency. If we wanted to go to new style classes,
why not make HTMLInputMixin a newstyle class? What about the other
objects. in cgi/templating.py
msg5016 Author: [hidden] (r.david.murray) Date: 2014-03-11 14:42
Yeah, I changed that class only in my test just to have a minimal change.

I think you would have to implement all the rich comparison methods to
fix it fully without changing the class type.

I'll try changing HTMLInputMixin to new-style in my tracker instance and
see if it breaks anything.  I don't think it will.
msg5017 Author: [hidden] (ber) Date: 2014-03-12 11:19
Reproduced an unwanted behaviour, instructions:
a) start a demo instance, e.g.
   ppython demo.py -b postgresql nuke
b) login as admin, create an additional user A
c) login as demo and in a different webbrowser as A
d) create a named query with demo
e) verify that A cannot see this query
f) as demo: switch the "private" on this query to now and save
Observations:
  private is still shown as "yes"
  A cannot see the search entry.
Wanted behaviour: private shown as "no" and A can see the query.

Adding the following small change makes the above test pass.
-class HTMLInputMixin:
+class HTMLInputMixin(object):

Interesting enough, in an old installation, setting a query to non-private
worked, so it got broken somewhere.
msg5018 Author: [hidden] (ber) Date: 2014-03-12 16:49
this change removes raising an exception if an attribute is not found:
hg log -r 3164
Ă„nderung:        3164:24476db8872f
Nutzer:          Richard Jones <richard@users.sourceforge.net>
Datum:           Mon Feb 14 04:53:41 2005 +0000
Zusammenfassung: nicer error looking up values of None (response to 
[SF#1108697]
@@ -1565,7 +1568,8 @@
         ''' return a new HTMLItem '''
        #print 'Link.getattr', (self, attr, self._value)
         if not self._value:
-            raise AttributeError, "Can't access missing value"
+            msg = self._('Attempt to look up %(attr)s on a missing value')
+            return MissingValue(msg%locals())
         i = HTMLItem(self._client, self._prop.classname, self._value)
         return getattr(i, attr)

I'm observing the bad behaviour the first time after
Ă„nderung:        3913:00896a2acaa5
Nutzer:          Justus Pendleton <jpend@users.sourceforge.net>
Datum:           Thu Sep 20 23:44:58 2007 +0000
Zusammenfassung: clean up query display of "Private to you" items

No obvious problem found, it probably did not get tested well enough 
after the rev3913 change. 

I've decided for the smaller fix, because it limits possible undetected side 
effect, changing  LinkHTMLProperty to a new-style class.
Committed with rev4864:f630eb0adcee

Thanks again David!
History
Date User Action Args
2014-03-12 16:49:40bersetstatus: new -> fixed
messages: + msg5018
2014-03-12 11:19:38bersetmessages: + msg5017
2014-03-11 14:42:37r.david.murraysetmessages: + msg5016
2014-03-11 14:20:33bersetmessages: + msg5015
2014-03-10 11:32:00bersetmessages: + msg5006
2014-03-07 08:04:20bersetmessages: + msg4999
2014-03-06 20:06:21r.david.murraysetfiles: + linkhtmlproperty.patch
keywords: + patch
messages: + msg4998
2014-03-06 19:13:19r.david.murraysettitle: A LinkHTMLProperty cannot be compared successfully -> An empty LinkHTMLProperty cannot be compared successfully
2014-03-06 13:49:54r.david.murraysetmessages: + msg4996
2014-03-06 08:15:53bersetnosy: + ber
messages: + msg4995
2014-03-05 20:21:15r.david.murraycreate