247936 - Creating a product/component which already existed crashes when adding series

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[Jeff Dubrule]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6) Gecko/20040404 Firefox/0.8
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6) Gecko/20040404 Firefox/0.8

Deleting a product does not remove its entries from the 'Series' table.

Reproducible: Always
Steps to Reproduce:
1. Add a product
2. Delete it
3. Try to add it again

Actual Results:  
Software error:

DBD::Oracle::db do failed: ORA-00001: unique constraint (BUGZILLA.SERIES_1_UDX)
violated (DBD ERROR: OCIStmtExecute) [for statement ``INSERT INTO series
(series_id, creator, category, subcategory, name, frequency, query) VALUES
(series_series_id_seq.nextval, ?, ?, ?, ?, ?, ?)'']) at Bugzilla/Series.pm line 185
	Bugzilla::Series::writeToDatabase('Bugzilla::Series=HASH(0x88e531c)') called at
/srv/www/cgi-bin/bugzilla/editproducts.cgi line 420



Expected Results:  
Adding new product
	  	
OK, done.

Comment 1

[Albert Ting]

I'm seeing this as well for 2.18RC1.  This becomes a problem if you delete
a product and want to recreate the product.  

Comment 2

[Dave Miller [:justdave]]

*** Bug 263161 has been marked as a duplicate of this bug. ***

Comment 3

[Dave Miller [:justdave]]

Happens with components, too.

Comment 4

[Frédéric Buclin]

Attachment: untested patch, v1

this patch checks if the series already exists and updates its series_id
accordingly. Works with editproducts.cgi but may break something else. This
patch needs to be tested...

Comment 5

[Frédéric Buclin]

Attachment: untested patch, v1.0.1

same patch but with trailing whitespaces removed.

Comment 6

[Gervase Markham [:gerv]]

I'm not sure this is the right fix - or at least, not a complete fix. For
example, if we delete a product, we should be deleting all the series data too.

Gerv

Comment 7

[Frédéric Buclin]

gerv, justdave said we should keep series data:

(18:15:41) LpSolit: justdave, when deleting a product, do we want to delete its
informations from series too or do we want to keep them for historical purpose?
(18:16:18) justdave: that's a really good question.
(18:16:33) justdave: in theory we should probably keep it around
(18:16:43) LpSolit: ok
(18:16:55) wicked: what about in practice? :)
(18:16:57) justdave: we probably just need to not bail if there's already
categories or subcategories existing with that product/component name
(18:17:14) justdave: i.e. check if they already exist and not do anything if
they're already there
(18:17:28) justdave: only create them if they don't already exist

Or I misunderstood what he said ;)

Comment 8

[Jeff Jensen]

May I suggest running "Sanity check" too.  For example, after I deleted a group,
it reports the orphaned records as errors:
Checking references to groups.id
  [snip]
Bad value 10 found in group_group_map.grantor_id
  [snip]
Bad value 10 found in user_group_map.group_id 

As a final test of the fixes to cleanup orphaned records, should not "Sanity
check" run without reporting these errors?

I also experienced the error with the components table (which is already
reported), and note the groups table has the same type of problem.

Comment 9

[Frédéric Buclin]

gerv, could you please review this patch?

Comment 10

[Gervase Markham [:gerv]]

Hmm. But what happens if the newly-created product is in fact a different thing
from the old one which happened to have the same name? No series will get
created when perhaps they were wanted... I need to think about this some more.

I also can't be sure about the possible side-effects of this patch without
checking carefully. I will endeavour to do so over the Easter weekend.

Gerv

Comment 11

[Marc Schumann [:Wurblzap]]

Just for the record, I disagree with the idea of comment 7 to retain the series
data when deleting a product (or component). I think it should be deleted along
with the product. If you want to keep the data, keep the product.

Comment 12

[Max Kanat-Alexander]

(In reply to comment #11)
> Just for the record, I disagree with the idea of comment 7 to retain the series
> data when deleting a product (or component). I think it should be deleted along
> with the product. If you want to keep the data, keep the product.

  I agree also. You can always close a product to bug entry and put it behind a
mandatory group if you just want to make it "invisible."

  Keeping around data in the DB that breaks referential integrity is a bad idea.

Comment 13

[Max Kanat-Alexander]

I talked to Gerv on IRC, and he thinks that this doesn't really block 2.18.1,
but we should push it off to 2.18.2.

Comment 14

[Frédéric Buclin]

gerv, any progress?

Comment 15

[Gervase Markham [:gerv]]

Nope, sorry - still snowed under :-( I know I suck.

Gerv

Comment 16

[A. Karl Kornel]

I talked with LpSolit & Wurblzap about this on IRC not too long ago, and it
seems that this relies on two things:

1) A standard interface for deleting a data set, both manually through chart.cgi
and automatically when deleting a product.
2) A way to modify the query executed by a data set, accessible through
chart.cgi (and linked to when deleting a product).

Why is it so important for someone to be able to modify a query?  Wurblzap noted
that you might have a data set tracking (for example) the number of bugs over
more than one customer product.  Deleting the entire set simply because you're
deleting a single customer product would delete still-valuable data.  Even so,
it may be enough for now to simply take care of (1) and leave (2) for another time.

I've created bug 302542 to track (1).  (2) already exists as bug 230254.  By the
way, the IRC conversation can be read at <http://tinyurl.com/8l7r3>, from time
23:16 to 23:37.

Comment 17

[Max Kanat-Alexander]

justdave - I recommend that this bug no longer block the final 2.20 release, due
to the possible complexity that it will create, and also due to the fact that
it's extremely unlikely that we will have a r+ patch on this bug any time soon.

Comment 18

[Dave Miller [:justdave]]

Agreed.  We shall live with charting continuing to suck for another release.

Comment 19

[A. Karl Kornel]

Attachment: Sample "Confirm Product Deletion" Page

Here is what the Patch v1 output would look like.

This is an example of how the delete product confirmation page will look if
there are data sets explicitly mentioning the product, and the logged-in user
is a non-admin who has chartgroup and editcomponents access.

The first table shows the data sets that were automagically created.  The
second table shows the data sets created by the logged-in user, which will also
be deleted.  The paragraph directly below the second table states that there
are 1+ data sets that were created by others and can't be touched, so the
product can't be deleted.  If the other 1+ data sets were visible to all, then
they would also be displayed.

Note the CSV & query links.  Also note how the logged-in user is able to edit
his data set to keep it from deletion.

Comment 20

[A. Karl Kornel]

Attachment: Patch v1

This patch, when combined with the patches to bug 230254 (edit data set query)
and bug 302542 (delete data set), should do all that's required.

This patch only comes into play if there are one or more series that explicitly
refer to the product being deleted.  If one or more series are found that
explicitly refer to the product, the users group memberships determine the
outcome.

* If the user is an administrator, the deletion can go ahead.  The admin is
also given the option to edit the data set queries, and can also download the
data set data (in CSV form) for archiving.

* If the user is not an administrator but is in the chargroup, the user can
delete the product IF the only series that exist were automagically-created
data sets (data sets created by Bugzilla when the product was created) or
custom data sets created by the user.  The user is given the option to edit his
own custom data sets instead of deleting them.	If any of the data sets were
created by other users, then the product can not be deleted.  The user will be
able to archive data set data for all data sets, except for private data sets
created by other users.  In fact, if there are any private data sets, the user
will be told that they exist but won't be given any names.

* If the user is not an admin AND is not in the chartgroup, then the product
can only be deleted IF the only relevant data sets were automagically created
(in other words, no user-created data sets).  The user won't be able to
archive, view, or edit anything except for the data set names.

Most of the work takes place in editproducts.cgi.  Changes were also made to
the templates admin/product/confirm.delete.html and global/user-error.html.

Requesting review from Gerv.  I'll also be requesting review from
documentation@.  I'll be looking out for review responses regularly, so any
delays probably won't be coming from me!

I should note that the dependencies could probably be removed, but doing so
requires a sacrifice.  If you're willing to lose the ability to edit a data set
query, then the dependency on bug 230254 can be removed.  The dependency on bug
302542 can also be removed, but some of the code from bug 302542 would have to
be moved into this bug, and it would also mean that the only way to delete a
data set would be to delete the product associated with it (at least, until bug
302542 lands).

Comment 21

[Frédéric Buclin]

gerv, could you have a look at karl's patches? Charts really need work and we
should be glad that somebody cares to do some work on them. These patches are
awaiting your reviews for almost one month. FYI, karl is often in #mozwebtools
and maybe you could discuss together.

Comment 22

[Gervase Markham [:gerv]]

I've reviewed several of Karl's patches today, and sent him an email about the rest.

Gerv

Comment 23

[Frédéric Buclin]

Now that 2.20 has been released, 2.18 should now only accept crash and security
fixes. Retargetting to 2.20

Comment 24

[Dave Miller [:justdave]]

I'll leave this targetted to 2.22 for now, but it's not going to block release.  This has been broken for as long as we've had charts, so it won't break anything new by continuing to be broken.

Comment 25

[Vlad Dascalu]

*** Bug 336861 has been marked as a duplicate of this bug. ***

Comment 26

[Frédéric Buclin]

*** Bug 336861 has been marked as a duplicate of this bug. ***

Comment 27

[Frédéric Buclin]

*** Bug 347809 has been marked as a duplicate of this bug. ***

Comment 29

[Loïc Minier]

I also get a sub-bug of this bug and I find that quite disturbing: Bugzilla requires removing people from groups before removing groups, but it will happily keep components of removed products and the components will magically re-appear if you create the Product again...

Comment 31

[Frédéric Buclin]

gerv, I would like to take my patch 1.0.1 as a workaround till this bug is fixed to stop these crashes when you create twice the same component or product. This kind of problem happens all the time when doing QA. I can file a separate bug to include this workaround so that we keep this bug open for a real fix. Is that OK for you?

Comment 32

[Gervase Markham [:gerv]]

You should do whatever you think is right :-)

Gerv

Comment 33

[Max Kanat-Alexander]

Comment on attachment 177855 [details] [diff] [review]
untested patch, v1.0.1

If this works, then it's fine with me. Let's just do it--it's such an edge case anyway.

Comment 34

[Max Kanat-Alexander]

The patch is so old we should make sure it still works on all the modern code and branches before checkin, of course.

Comment 35

[Frédéric Buclin]

(In reply to comment #34)
> The patch is so old we should make sure it still works on all the modern code
> and branches before checkin, of course.

No worry, all our QA installations, from 2.22 to 3.2, have this patch applied already. So I know it works as all our Selenium scripts are written assuming this patch is applied (else we would be unable to delete and recreate products and components with the same names).

I morphed the bug summary to reflect what we did here. I will clone the bug to decide what to do when deleting products.

tip:

Checking in Bugzilla/Series.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Series.pm,v  <--  Series.pm
new revision: 1.18; previous revision: 1.17
done

3.2rc1:

Checking in Bugzilla/Series.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Series.pm,v  <--  Series.pm
new revision: 1.16.2.1; previous revision: 1.16
done

3.0.5:

Checking in Bugzilla/Series.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Series.pm,v  <--  Series.pm
new revision: 1.14.2.1; previous revision: 1.14
done