Closed Bug 1478897 Opened 6 years ago Closed 6 years ago

ensure phabbugs doesn't fail outright when encountering invalid bug ids

Categories

(bugzilla.mozilla.org :: Phabricator Integration, defect, P1)

Production
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: glob, Assigned: dkl)

References

Details

(Keywords: conduit-triaged)

Attachments

(1 file)

45 bytes, text/x-github-pull-request
Details | Review
in https://phabricator-dev.allizom.org/D555 i used arc to submit with bug id set to 1.

evidently this broke phabbugs and the id needed to be manually removed from the revision.

phabbugs needs to handle this situation gracefully.


i suspect the right things to do is for phabbugz to automatically remove invalid bug-ids from revisions (as opposed to failing outright, which may leave it in a weird state requiring user/admin intervention to resolve).
(In reply to Byron Jones ‹:glob› 🎈 from comment #0)
> i suspect the right things to do is for phabbugz to automatically remove
> invalid bug-ids from revisions (as opposed to failing outright, which may
> leave it in a weird state requiring user/admin intervention to resolve).

I was under the impression that the revision creation / update with the invalid bug id would fail, due to the validation. If it's possible to bypass that validation then the custom field's implementation should be re-examined in general as one of the core assumptions has been broken.
(In reply to Steven MacLeod [:smacleod] from comment #1)
> I was under the impression that the revision creation / update with the
> invalid bug id would fail, due to the validation. If it's possible to bypass
> that validation then the custom field's implementation should be re-examined
> in general as one of the core assumptions has been broken.

Ah, okay, that's Bug 1478898
Keywords: conduit-triaged
Priority: -- → P1
See Also: → 1478898
Regardless of protections to prevent that from happening, the code must handle the case.
If phabricator is destroyed and returned from a backup, its database and bmo will be out of sync. Handling that as gracefully as possible is good.
Oh wow, this isn't even the bug I thought it was. I thought y'all were commenting on bug 1478889 which I filed last night.
This is actually a false alarm. globs account is an admin on the bugzilla-dev server and the code is written that we skip bug id validation if the author is an admin. So it just let him use whatever bug id he chose as long as it was numeric.

But i will still fix the other bug where the feed daemon to handle invalid bug ids properly on its end. I am thinking that if an invalid bug is found by feed daemon, we leave the revision private, and add a comment to the revision stating that the bug id is invalid and let the author then change it themselves. It's really the only thing we can do. We cannot clear the bug id field and make it public and we also would not know which security policy to apply to it. We already do something similar when a bug is private to groups not preset in Phabricator projects.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → INVALID
Closed the wrong bug. The other one is about Phabricator.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Assignee: nobody → dkl
Status: REOPENED → ASSIGNED
Also fwiw once we have monitoring in place for phabbugz, we would see this type of error very quickly as the feed daemon would just bail when it saw the invalid bug id. And all processing would just stop til we remedied the issue.
Merged to master.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Attached file pull request
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: