Closed Bug 978146 Opened 7 years ago Closed 7 years ago

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

Categories

(Bugzilla :: Creating/Changing Bugs, defect)

4.2.7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 4.4

People

(Reporter: ekyle, Assigned: glob)

Details

Attachments

(2 files, 1 obsolete file)

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?(mak77@bonardo.net), review?(ehsan@mozilla.com), review?(tglek@mozilla.com), review?(cbiesinger@gmail.com), review?(l10n@mozilla.com), review?(khuey@kylehuey.com), review?(dholbert@mozilla.com), review?(joshmoz@gmail.com), review?(bjacob@mozilla.co"


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
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
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
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.
Summary: Is added and removed fields in bugs_activity table big enough? → activity entry when setting flags isn't split across multiple rows
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.
Assignee: nobody → create-and-change
Component: General → Creating/Changing Bugs
Product: bugzilla.mozilla.org → Bugzilla
QA Contact: default-qa
Version: Production → 4.2.7
Flags: approval?
Flags: approval4.4?
Target Milestone: --- → Bugzilla 4.4
Flags: approval?
Flags: approval4.4?
Target Milestone: Bugzilla 4.4 → ---
Attached patch 978146_1.patch (obsolete) β€” β€” Splinter Review
Assignee: create-and-change → glob
Status: NEW → ASSIGNED
Attachment #8385901 - Flags: review?(dkl)
Comment on attachment 8385901 [details] [diff] [review]
978146_1.patch

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

r=dkl
Attachment #8385901 - Flags: review?(dkl) → review+
Flags: approval?
Flags: approval4.4?
Target Milestone: --- → Bugzilla 4.4
Flags: approval?
Flags: approval4.4?
Flags: approval4.4+
Flags: approval+
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.
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
Flags: approval4.4+ → needinfo?(glob)
i'll provide an updated patch as well as one for 4.4.
Flags: needinfo?(glob)
Flags: approval+
Attached patch 978146_2.patch (trunk) β€” β€” Splinter Review
- adds 'require Bugzilla::Bug'
Attachment #8385901 - Attachment is obsolete: true
Attachment #8429805 - Flags: review?(dkl)
Attached patch 978146_1.patch (4.4) β€” β€” Splinter Review
- backport for 4.4
Attachment #8429806 - Flags: review?(dkl)
Comment on attachment 8429805 [details] [diff] [review]
978146_2.patch (trunk)

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

r=dkl
Attachment #8429805 - Flags: review?(dkl) → review+
Flags: approval4.4?
Comment on attachment 8429806 [details] [diff] [review]
978146_1.patch (4.4)

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

r=dkl
Attachment #8429806 - Flags: review?(dkl) → review+
Flags: approval?
Flags: approval?
Flags: approval4.4?
Flags: approval4.4+
Flags: approval+
To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   4d5e362..02719d2  master -> master

To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   f7f5857..e65e41a  4.4 -> 4.4
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.