978146 - activity entry when setting flags isn't split across multiple rows

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[Kyle Lahnakoski [:ekyle]]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0 (Beta/Release)
Build ID: 20140212131424

Steps to reproduce:

While debugging the BZ-ETL I noticed some corrupted attachment flags:

eg:

"review?([e-mail]), review?([e-mail]), review?([e-mail]), review?([e-mail]), review?([e-mail]), review?([e-mail]), review?([e-mail]), review?([e-mail]), review?([e-mail]"


Actual results:

There are occasional review requests that request too many reviewers:

https://bugzilla.mozilla.org/show_activity.cgi?id=671185



Expected results:

1) The user should be reprimanded for asking for too many reviewers, or...
2) The submission broken into parts, so the one change looks like many, or...
3) The ADDED and REMOVED fields in BUGS_ACTIVITY table be made longer

Comment 1

[David Lawrence [:dkl]]

Once we refactor the bugs_activity table one day this won't be an issue. Until then we either 1) increase the added/removed fields in size (currently varchar(255)) or 2) do a better job of splitting the added/removed values so that flags changes are not cut off in the middle like the example given.

From looking at the code for 2), we already look for commas as wrapping point so I am not sure why the example did not have the last review flag into a new activity row. Using a test script that calls the same functions and using the same added value, I am not able to get this to fail.

dkl

Comment 2

[Kyle Lahnakoski [:ekyle]]

How does this impact the User Story field?  I have not encountered any diffs that have been impacted, but I find it hard to believe none have hit the 255 limit

Comment 3

[:glob ✱]

when writing to that table, the values are not truncated instead they are split across multiple rows to work around the 255 char limit.

when bugzilla reads that table, values which are split across multiple entries are concatenated together, so we don't hit this issue from within bugzilla.

the user story field most likely hits this limit too, however we don't notice it due to the joining of rows.  for user-story edits, we only write unified diffs rather than a complete before/after in order to minimise this issue.


increasing the size of those fields would be detrimental to performance, as mysql will have to perform an additional lookup to retrieve that information.


to me it looks like the splitting code didn't work when that change was made.
(tests with current codebase).
even with the current code, these rows aren't being split.

Comment 4

[:glob ✱]

Bugzilla/Attachment.pm and editusers.cgi both write to the bugs_activity table directly, instead of using Bugzilla::Bug::LogActivityEntry.  as a result values are not split when they exceed MAX_LINE_LENGTH, resulting in truncated data.

Comment 5

[:glob ✱]

Attachment: 978146_1.patch

Comment 6

[David Lawrence [:dkl]]

Comment on attachment 8385901 [details] [diff] [review]
978146_1.patch

Review of attachment 8385901 [details] [diff] [review]:
-----------------------------------------------------------------

r=dkl

Comment 7

[Frédéric Buclin]

Comment on attachment 8385901 [details] [diff] [review]
978146_1.patch

>=== modified file 'Bugzilla/Attachment.pm'

>+        Bugzilla::Bug::LogActivityEntry($self->bug_id, $field, $change->[0],
>+            $change->[1], $user->id, $timestamp, undef, $self->id);

Are you sure you don't need to call "require Bugzilla::Bug" first? You make the assumption that the caller loaded it already.

Comment 8

[David Lawrence [:dkl]]

My bad. This does not apply cleanly to 4.4 and a separate patch will need to be provided for it. Also comment 7.

dkl

Comment 9

[:glob ✱]

i'll provide an updated patch as well as one for 4.4.

Comment 10

[:glob ✱]

Attachment: 978146_2.patch (trunk)

- adds 'require Bugzilla::Bug'

Comment 11

[:glob ✱]

Attachment: 978146_1.patch (4.4)

- backport for 4.4

Comment 12

[David Lawrence [:dkl]]

Comment on attachment 8429805 [details] [diff] [review]
978146_2.patch (trunk)

Review of attachment 8429805 [details] [diff] [review]:
-----------------------------------------------------------------

r=dkl

Comment 13

[David Lawrence [:dkl]]

Comment on attachment 8429806 [details] [diff] [review]
978146_1.patch (4.4)

Review of attachment 8429806 [details] [diff] [review]:
-----------------------------------------------------------------

r=dkl

Comment 14

[:glob ✱]

To ssh://[e-mail]/bugzilla/bugzilla.git
   4d5e362..02719d2  master -> master

To ssh://[e-mail]/bugzilla/bugzilla.git
   f7f5857..e65e41a  4.4 -> 4.4