92266 - Generate software error if truncation occurs on bugs activity table.

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect, P1)

Description

[Matthew Tuck [:CodeMachine]]

Supposedly there won't be any more truncation "in practice" on the activity
table now bug #55161 is fixed, but let's not risk it - we could add heaps of
dependencies at once.

We should add an assertion to check it fits, and generate a software error (or a
nicer error if you can) if the assertion fails.

Comment 1

[Dave Miller [:justdave]]

other options for how to fix this:

1) create a separate activity table entry for each item added or removed instead
of putting a list of them

2) automatically split at the first comma before 255 characters and create a
second (and third, etc, if necessary) entry for the additional data.

Comment 2

[Dave Miller [:justdave]]

Attachment: Patch v1

Comment 3

[Dave Miller [:justdave]]

The attached patch creates a new routine called LogActivityEntry() in an attempt
to consolidate the activity table stuff so I didn't have to add the same code in
4 different places (since it was a sizeable chunk of code).

It takes the approach of truncating the data at the last comma before 255
characters, and then creating a second entry with the remaining data (and a
third, and so on, if necessary).  If for some strange reason there are no commas
in the first 255 characters, it sends the whole thing through (which will end up
getting truncated) but prepends it with a "? " which is used by the display code
to indicate dataloss in that entry.  I can't think of any situation where this
would ever get triggered, but it's there just in case.

I have NOT tested this, it's late, and I'm tired, and I wanted to get it up
before I went to bed. ;)  We can play with it tomorrow, but I thought I'd get it
here in case anyone wanted to play before I got here again.

Comment 4

[Jacob Steenhagen]

I was going to put this on landfill for testing, but I got compile errors:

[Thu Aug 16 05:56:35 2001] process_bug.cgi: Number found where operator expected
at process_bug.cgi line 773, near ""," 254"

Also, |my $ccid = GetFieldID("cc");| isn't needed anymore (it's in the last
chunk, but doesn't have a '-' in front of it.

Comment 5

[Dave Miller [:justdave]]

Attachment: Patch v2

Comment 6

[Dave Miller [:justdave]]

OK, looks like the $whochange thing wasn't necessary either.  (and it wasn't
before, either, so I'm not sure why it was there to begin with :)

Comment 7

[Jacob Steenhagen]

In the limited testing I've done, this looks good.  I don't have 256 character's
of data to throw at it to give it a proper testing, but if somebody else does
it's posted at http://landfill.tequilarista.org/bz92593 (and more importantly,
the time to do it as I've just run short on that for the afternoon :)

I did confirm that I can do a couple of basic changes and they still properly
appear in the log.

Comment 8

[Dave Miller [:justdave]]

OK, I've tested this on landfill, see
http://landfill.tequilarista.org/bz92593/show_activity.cgi?id=267

It properly separated the lines on commas.  I was disturbed however, to discover
that the summary and status_whiteboard fields on bugs are both mediumints in the
database (which is 32K), and no validation is performed within Bugzilla to make
sure they fit anything.  This patch appears to work as is (but since I wrote the
patch, it needs independent verification).  The summary/status_whiteboard issue
is probably good for another bug.  It's infrequent enough (22 bugs affected out
of 96000 on b.m.o according to Myk) that it's probably not a rush issue.

Comment 9

[Dave Miller [:justdave]]

Attachment: Patch v3

Comment 10

[Dave Miller [:justdave]]

v3 completely changes the way the split point is determined, so we unfortunately
probably need to do all the same tests all over again (poor people getting all
that spam...)

It now attempts first to split on a comma, if it doesn't find one it'll look for
a space, and failing that will look for a hyphen.  If it *still* hasn't found
something to split on at that point it'll just break it at 254 characters
whether it's in the middle of a word or not.

No more dataloss at all.

Comment 11

[Jacob Steenhagen]

Darn, I found dataloss.  If you make a change w/out an space/comma/dash in it 
and it's forced to split on character 254, you loose a single character.

http://landfill.tequilarista.org/214shakedown/show_activity.cgi?id=267


Bug 96431 has been filed to deal with the field size descrepencies.

Comment 12

[Dave Miller [:justdave]]

Attachment: Patch v4

Comment 13

[Dave Miller [:justdave]]

This one shouldn't have any dataloss.  Instead of removing the character that it
splits on, it now leaves it on the left side if it's a hyphen, and on the right
side if it's anything else, and it removes commas and spaces from the beginning
of the right side before using it now.  It's live on 214shakedown on landfill.

Comment 14

[Dave Miller [:justdave]]

Attachment: Patch v5

Comment 15

[Dave Miller [:justdave]]

Ignore v4.  I didn't notice that 'patch' dropped the sub from that existing
patch in the middle of another sub when I applied it to the cvs tip to work on
it some more.  v5 has the code actually in the right place :-)

Comment 16

[Dave Miller [:justdave]]

r= Jake in IRC

checked in.

Comment 17

[Dave Miller [:justdave]]

correcting version field lost in product move