Roundup Tracker - Issues

Issue 2550722

classification
Templating error from selecting "no selection" on multilink
Type: crash Severity: normal
Components: Web interface Versions: devel
process
Status: fixed fixed
:
: : ber, joseph_myers, schlatterbeck
Priority: : patch

Created on 2011-08-21 23:00 by joseph_myers, last changed 2018-09-27 11:40 by joseph_myers.

Files
File name Uploaded Description Edit Remove
issue2550722-multilink.patch joseph_myers, 2014-09-01 01:22 Patch
issue2550722-multilink.patch-v2 joseph_myers, 2018-08-05 23:11 Revised patch
Messages
msg4386 Author: [hidden] (joseph_myers) Date: 2011-08-21 23:00
If you select "no selection" on a multilink field when editing an item,
and in fact nothing is selected for that multilink in the database item,
and you make no other changes (so submitting a null edit), a templating
error occurs on attempting to render the results of the null edit.  No
redirection occurs for a null edit (unlike for a non-null edit);
instead, the page for that item is rendered directly from the form
submission - with a context containing the values from that form
submission, and in particular a negative value for the multilink, which
is not a valid member of that class.

An example may make this clearer.  I verified the issue with the present
tracker as follows: I found an issue, issue2550709, with a multilink
with no values selected.  I submitted a first, erroneous, edit, that
both set "Components" to None (changing the no-values-selected multilink
to have a value selected) and cleared the title (having JavaScript
disabled so no client-side detection of the missing title could occur).
 This produced the expected error page with "Required issue property
title not supplied", and "Components" correctly showing "None" (i.e.,
that edit was not lost) (and the original title back) - and because of
"None" being selected, the "no selection" option was also present, which
it wasn't on the original issue page.  I then changed "Components" to
"no selection" (selecting "no selection", unselecting "None") and
resubmitted - this being the null edit, since the database already had
no Components value set.  The result was the "An error has occurred" page.
msg4388 Author: [hidden] (ber) Date: 2011-08-22 08:17
Joseph,
thanks for the detailed report.
I guess this was for 1.4.19 and svn trunk?

Next would probably be a patch to fix this or maybe even a 
unit test to check that behaviour even earlier. 

Best,
Bernhard
msg4391 Author: [hidden] (joseph_myers) Date: 2011-08-22 09:14
I observed the issue with 1.4.19 (my own instance, not the standard 
templates and schema) then verified it with Roundup's own tracker (I don't 
know what version that is running) to confirm my conclusion that it was an 
issue with the Roundup core rather than with my own instance and to 
provide an easy way to reproduce it with a more or less standard Roundup 
installation.  I don't believe there have been any relevant changes 
checked in since 1.4.19.
msg5133 Author: [hidden] (joseph_myers) Date: 2014-09-01 01:22
Here is a patch that appears to fix the issue (tested for the problem
cases with 1.5.0, though the diff is generated against current hg sources).

Although the negative multilink value not being a member of the class is
what causes the templating error, it seems the actual problem starts
earlier; the emailed exception information includes another error (a
normal one that would have been reported to the user if the templating
error hadn't arisen and so resulted in an email to the admin).  This is
a "not currently an element" error that there is an attempt to remove a
multilink value despite that value not being an element of the multilink
in the first place (it's not an element in the database; it is an
element in the results of the first erroneous edit).

Because that case can occur legitimately, I think the error is
incorrect, so this patch just replaces it with "pass".  (If the code is
operating correctly, ignoring an attempt to remove something that's not
there to remove in the first place seems completely harmless and in line
with what the user presumably intended by the removal.)

This fixes both cases of this bug that I know of.  (In addition to the
one I described in the original submission, I've since become aware of
the following case of the same bug: the original submission trying to
create a database item, with nonempty value for a multilink, has an
error.  A resubmission - presumably trying to correct the error - then
changes the multilink value to "no selection".  This resulted in the
same templating error before this patch, and works fine after the patch.)
msg5135 Author: [hidden] (ber) Date: 2014-09-02 13:32
Joseph,
thanks for the update and the patch.

Next we need to find someone to evaluate it! :)
When doing a fast read, I can see that it would take me a bit
to understand it.

Bernhard
msg6173 Author: [hidden] (joseph_myers) Date: 2018-08-05 23:11
I've attached a revised and updated version of my patch for this issue.

The hyperdb.py change is the same as before, with the same
justification, except that it's updated so it still applies cleanly
after the Python 3 changes, and test_hyperdbvals.py is also updated so
the testsuite still passes after that change.

In addition, I found another case of this issue where even with the
hyperdb.py patch an internal error occurs: if there is an error (found
by a detector, for example) in the initial attempt to create an object,
with a nonempty setting for a multilink in that attempt, and then "no
selection" is selected by the user and the form resubmitted, but there
is still an error, and an attempt is made by Roundup to render the
object using the submitted form contents with "no selection", then the
templating code produces an internal error trying to produce a menu
entry for the "-<id>" multilink value.  To fix that case, templating.py
is also changed in the latest patch version to remove all such "-<id>"
multilink values.  (If not removed, they cause problems generating a
menu entry: the "make sure we list the current values if they're
retired" logic ends up inserting the "-<id>" value into the options
list, and subsequent code then tries to get the linked-to object.  But
it's not sufficient just to change that logic for adding values to the
options list, because if you do just that then the new menu ends up with
"--<id>" as the value for its "no selection" entry.  So it seems right
to remove such values for all purposes as early as possible in this
templating code.)
msg6246 Author: [hidden] (joseph_myers) Date: 2018-09-16 13:46
Any comments on this patch?
msg6261 Author: [hidden] (joseph_myers) Date: 2018-09-27 11:40
Fix committed.
History
Date User Action Args
2018-09-27 11:40:08joseph_myerssetstatus: new -> fixed
resolution: fixed
messages: + msg6261
2018-09-16 13:46:05joseph_myerssetmessages: + msg6246
2018-08-05 23:11:48joseph_myerssetfiles: + issue2550722-multilink.patch-v2
messages: + msg6173
2016-07-09 21:17:20rouiljsetkeywords: + patch
2016-07-09 21:17:10rouiljsetkeywords: - patch
2014-09-02 13:32:35bersetnosy: + schlatterbeck
messages: + msg5135
2014-09-01 01:22:26joseph_myerssetfiles: + issue2550722-multilink.patch
keywords: + patch
messages: + msg5133
2011-08-22 09:14:54joseph_myerssetmessages: + msg4391
2011-08-22 08:17:24bersetnosy: + ber
messages: + msg4388
2011-08-21 23:00:20joseph_myerscreate