diff options
Diffstat (limited to 'extensions/PhabBugz')
| -rw-r--r-- | extensions/PhabBugz/lib/Constants.pm | 2 | ||||
| -rw-r--r-- | extensions/PhabBugz/lib/Feed.pm | 182 | ||||
| -rw-r--r-- | extensions/PhabBugz/lib/Util.pm | 77 | ||||
| -rw-r--r-- | extensions/PhabBugz/lib/WebService.pm | 105 | ||||
| -rw-r--r-- | extensions/PhabBugz/t/basic.t | 27 | ||||
| -rw-r--r-- | extensions/PhabBugz/t/feed-daemon-guts.t | 12 |
6 files changed, 80 insertions, 325 deletions
diff --git a/extensions/PhabBugz/lib/Constants.pm b/extensions/PhabBugz/lib/Constants.pm index 1692f8fb9..19987de25 100644 --- a/extensions/PhabBugz/lib/Constants.pm +++ b/extensions/PhabBugz/lib/Constants.pm @@ -19,6 +19,7 @@ our @EXPORT = qw( PHAB_FEED_POLL_SECONDS PHAB_USER_POLL_SECONDS PHAB_GROUP_POLL_SECONDS + PHAB_TIMEOUT ); use constant PHAB_ATTACHMENT_PATTERN => qr/^phabricator-D(\d+)/; @@ -27,5 +28,6 @@ use constant PHAB_CONTENT_TYPE => 'text/x-phabricator-request'; use constant PHAB_FEED_POLL_SECONDS => $ENV{PHAB_FEED_POLL} // 5; use constant PHAB_USER_POLL_SECONDS => $ENV{PHAB_USER_POLL} // 60; use constant PHAB_GROUP_POLL_SECONDS => $ENV{PHAB_GROUP_POLL} // 300; +use constant PHAB_TIMEOUT => $ENV{PHAB_TIMEOUT} // 60; 1; diff --git a/extensions/PhabBugz/lib/Feed.pm b/extensions/PhabBugz/lib/Feed.pm index f2a440bb1..71e6aa827 100644 --- a/extensions/PhabBugz/lib/Feed.pm +++ b/extensions/PhabBugz/lib/Feed.pm @@ -11,10 +11,10 @@ use 5.10.1; use IO::Async::Timer::Periodic; use IO::Async::Loop; +use IO::Async::Signal; use List::Util qw(first); use List::MoreUtils qw(any uniq); use Moo; -use Scalar::Util qw(blessed); use Try::Tiny; use Type::Params qw( compile ); use Type::Utils; @@ -48,6 +48,13 @@ my $Invocant = class_type { class => __PACKAGE__ }; sub start { my ($self) = @_; + my $sig_alarm = IO::Async::Signal->new( + name => 'ALRM', + on_receipt => sub { + FATAL("Timeout reached"); + exit; + }, + ); # Query for new revisions or changes my $feed_timer = IO::Async::Timer::Periodic->new( first_interval => 0, @@ -56,13 +63,17 @@ sub start { on_tick => sub { try { with_writable_database { + alarm(PHAB_TIMEOUT); $self->feed_query(); }; } catch { FATAL($_); + } + finally { + alarm(0); + Bugzilla->_cleanup(); }; - Bugzilla->_cleanup(); }, ); @@ -74,13 +85,17 @@ sub start { on_tick => sub { try { with_writable_database { + alarm(PHAB_TIMEOUT); $self->user_query(); }; } catch { FATAL($_); + } + finally { + alarm(0); + Bugzilla->_cleanup(); }; - Bugzilla->_cleanup(); }, ); @@ -92,13 +107,18 @@ sub start { on_tick => sub { try { with_writable_database { + alarm(PHAB_TIMEOUT); $self->group_query(); }; } catch { FATAL($_); + } + finally { + alarm(0); + Bugzilla->_cleanup(); }; - Bugzilla->_cleanup(); + }, ); @@ -106,6 +126,7 @@ sub start { $loop->add($feed_timer); $loop->add($user_timer); $loop->add($group_timer); + $loop->add($sig_alarm); $feed_timer->start; $user_timer->start; $group_timer->start; @@ -221,6 +242,11 @@ sub feed_query { $delete_build_target->execute($target->{name}, $target->{value}); } + + if (Bugzilla->datadog) { + my $dd = Bugzilla->datadog(); + $dd->increment('bugzilla.phabbugz.feed_query_count'); + } } sub user_query { @@ -261,6 +287,11 @@ sub user_query { }; $self->save_last_id($user_id, 'user'); } + + if (Bugzilla->datadog) { + my $dd = Bugzilla->datadog(); + $dd->increment('bugzilla.phabbugz.user_query_count'); + } } sub group_query { @@ -359,6 +390,11 @@ sub group_query { INFO( "Project " . $project->name . " updated" ); } } + + if (Bugzilla->datadog) { + my $dd = Bugzilla->datadog(); + $dd->increment('bugzilla.phabbugz.group_query_count'); + } } sub process_revision_change { @@ -501,105 +537,6 @@ sub process_revision_change { $attachment->update($timestamp); } - # REVIEWER STATUSES - - my (@accepted, @denied); - foreach my $review (@{ $revision->reviews }) { - push @accepted, $review->{user} if $review->{status} eq 'accepted'; - push @denied, $review->{user} if $review->{status} eq 'rejected'; - } - - my @accepted_user_ids = map { $_->bugzilla_user->id } grep { defined $_->bugzilla_user } @accepted; - my @denied_user_ids = map { $_->bugzilla_user->id } grep { defined $_->bugzilla_user } @denied; - my %reviewers_hash = map { $_->{user}->name => 1 } @{ $revision->reviews }; - - foreach my $attachment (@attachments) { - my ($attach_revision_id) = ($attachment->filename =~ PHAB_ATTACHMENT_PATTERN); - next if $revision->id != $attach_revision_id; - - # Clear old accepted review flags if no longer accepted - my (@denied_flags, @new_flags, @removed_flags, %accepted_done, $flag_type); - foreach my $flag (@{ $attachment->flags }) { - next if $flag->type->name ne 'review'; - $flag_type = $flag->type if $flag->type->is_active; - next if $flag->status ne '+'; - if (any { $flag->setter->id == $_ } @denied_user_ids) { - INFO('Denying review flag set by ' . $flag->setter->name); - push(@denied_flags, { id => $flag->id, setter => $flag->setter, status => 'X' }); - } - if (any { $flag->setter->id == $_ } @accepted_user_ids) { - INFO('Skipping as review+ already set by ' . $flag->setter->name); - $accepted_done{$flag->setter->id}++; - } - if (!any { $flag->setter->id == $_ } (@accepted_user_ids, @denied_user_ids)) { - INFO('Clearing review+ flag set by ' . $flag->setter->name); - push(@removed_flags, { id => $flag->id, setter => $flag->setter, status => 'X' }); - } - } - - $flag_type ||= first { $_->name eq 'review' && $_->is_active } @{ $attachment->flag_types }; - - die "Unable to find review flag!" unless $flag_type; - - # Create new flags - foreach my $user_id (@accepted_user_ids) { - next if $accepted_done{$user_id}; - my $user = Bugzilla::User->check({ id => $user_id, cache => 1 }); - INFO('Setting new review+ flag for ' . $user->name); - push(@new_flags, { type_id => $flag_type->id, setter => $user, status => '+' }); - } - - # Process each flag change by updating the flag and adding a comment - foreach my $flag_data (@new_flags) { - my $comment = $flag_data->{setter}->name . " has approved the revision."; - $self->add_flag_comment( - { - bug => $bug, - attachment => $attachment, - comment => $comment, - user => $flag_data->{setter}, - old_flags => [], - new_flags => [$flag_data], - timestamp => $timestamp - } - ); - } - foreach my $flag_data (@denied_flags) { - my $comment = $flag_data->{setter}->name . " has requested changes to the revision.\n"; - $self->add_flag_comment( - { - bug => $bug, - attachment => $attachment, - comment => $comment, - user => $flag_data->{setter}, - old_flags => [$flag_data], - new_flags => [], - timestamp => $timestamp - } - ); - } - foreach my $flag_data (@removed_flags) { - my $comment; - if ( exists $reviewers_hash{ $flag_data->{setter}->name } ) { - $comment = "Flag set by " . $flag_data->{setter}->name . " is no longer active.\n"; - } - else { - $comment = $flag_data->{setter}->name . " has been removed from the revision.\n"; - } - $self->add_flag_comment( - { - bug => $bug, - attachment => $attachment, - comment => $comment, - user => $flag_data->{setter}, - old_flags => [$flag_data], - new_flags => [], - timestamp => $timestamp - } - ); - } - } - # FINISH UP $bug->update($timestamp); @@ -736,6 +673,11 @@ sub process_new_user { foreach my $attachment (@attachments) { my ($revision_id) = ($attachment->filename =~ PHAB_ATTACHMENT_PATTERN); + if (!$revision_id) { + WARN("Skipping " . $attachment->filename . " on bug $bug_id. Filename should be fixed."); + next; + } + INFO("Processing revision D$revision_id"); my $revision = Bugzilla::Extension::PhabBugz::Revision->new_from_query( @@ -839,38 +781,4 @@ sub get_group_members { ); } -sub add_flag_comment { - state $check = compile( - $Invocant, - Dict [ - bug => Bug, - attachment => Attachment, - comment => Str, - user => User, - old_flags => ArrayRef, - new_flags => ArrayRef, - timestamp => Str, - ], - ); - my ( $self, $params ) = $check->(@_); - my ( $bug, $attachment, $comment, $user, $old_flags, $new_flags, $timestamp ) - = @$params{qw(bug attachment comment user old_flags new_flags timestamp)}; - - # when this function returns, Bugzilla->user will return to its previous value. - my $restore_prev_user = Bugzilla->set_user($user, scope_guard => 1); - - INFO("Flag comment: $comment"); - $bug->add_comment( - $comment, - { - isprivate => $attachment->isprivate, - type => CMT_ATTACHMENT_UPDATED, - extra_data => $attachment->id - } - ); - - $attachment->set_flags( $old_flags, $new_flags ); - $attachment->update($timestamp); -} - 1; diff --git a/extensions/PhabBugz/lib/Util.pm b/extensions/PhabBugz/lib/Util.pm index a93533e75..32f860413 100644 --- a/extensions/PhabBugz/lib/Util.pm +++ b/extensions/PhabBugz/lib/Util.pm @@ -20,14 +20,14 @@ use Bugzilla::Util qw(trim); use Bugzilla::Extension::PhabBugz::Constants; use Bugzilla::Extension::PhabBugz::Types qw(:types); -use JSON::XS qw(encode_json decode_json); use List::Util qw(first); -use LWP::UserAgent; use Taint::Util qw(untaint); use Try::Tiny; use Type::Params qw( compile ); use Type::Utils; use Types::Standard qw( :types ); +use Mojo::UserAgent; +use Mojo::JSON qw(encode_json); use base qw(Exporter); @@ -35,7 +35,6 @@ our @EXPORT = qw( create_revision_attachment get_attachment_revisions get_bug_role_phids - get_needs_review intersect is_attachment_phab_revision request @@ -160,11 +159,10 @@ sub request { my $ua = $request_cache->{phabricator_ua}; unless ($ua) { - $ua = $request_cache->{phabricator_ua} = LWP::UserAgent->new(timeout => 10); + $ua = $request_cache->{phabricator_ua} = Mojo::UserAgent->new; if ($params->{proxy_url}) { - $ua->proxy('https', $params->{proxy_url}); + $ua->proxy($params->{proxy_url}); } - $ua->default_header('Content-Type' => 'application/x-www-form-urlencoded'); } my $phab_api_key = $params->{phabricator_api_key}; @@ -176,25 +174,16 @@ sub request { $data->{__conduit__} = { token => $phab_api_key }; - my $response = $ua->post($full_uri, { params => encode_json($data) }); - + my $response = $ua->post($full_uri => form => { params => encode_json($data) })->result; ThrowCodeError('phabricator_api_error', { reason => $response->message }) if $response->is_error; - my $result; - my $result_ok = eval { - my $content = $response->content; - untaint($content); - $result = decode_json( $content ); - 1; - }; - if (!$result_ok || $result->{error_code}) { - ThrowCodeError('phabricator_api_error', - { reason => 'JSON decode failure' }) if !$result_ok; - ThrowCodeError('phabricator_api_error', - { code => $result->{error_code}, - reason => $result->{error_info} }) if $result->{error_code}; - } + my $result = $response->json; + ThrowCodeError('phabricator_api_error', + { reason => 'JSON decode failure' }) if !defined($result); + ThrowCodeError('phabricator_api_error', + { code => $result->{error_code}, + reason => $result->{error_info} }) if $result->{error_code}; return $result; } @@ -206,48 +195,4 @@ sub set_phab_user { return Bugzilla->set_user($user, scope_guard => 1); } -sub get_needs_review { - my ($user) = @_; - $user //= Bugzilla->user; - return unless $user->id; - - my $phab_user = Bugzilla::Extension::PhabBugz::User->new_from_query( - { - ids => [ $user->id ] - } - ); - - return [] unless $phab_user; - - my $diffs = request( - 'differential.revision.search', - { - attachments => { - reviewers => 1, - }, - constraints => { - reviewerPHIDs => [$phab_user->phid], - statuses => ["open()"], - }, - order => 'newest', - } - ); - ThrowCodeError('phabricator_api_error', { reason => 'Malformed Response' }) - unless exists $diffs->{result}{data}; - - my @revisions; - foreach my $revision ( @{ $diffs->{result}{data} } ) { - foreach my $reviewer ( @{ $revision->{attachments}->{reviewers}->{reviewers} } ) { - if ( $reviewer->{reviewerPHID} eq $phab_user->phid - && $reviewer->{status} =~ /^(?:added|blocking)$/ ) - { - push @revisions, $revision; - last; - } - } - } - - return \@revisions; -} - 1; diff --git a/extensions/PhabBugz/lib/WebService.pm b/extensions/PhabBugz/lib/WebService.pm index fa9306667..19a758a70 100644 --- a/extensions/PhabBugz/lib/WebService.pm +++ b/extensions/PhabBugz/lib/WebService.pm @@ -16,29 +16,23 @@ use base qw(Bugzilla::WebService); use Bugzilla::Constants; use Bugzilla::Error; use Bugzilla::User; -use Bugzilla::Util qw(detaint_natural datetime_from time_ago trick_taint); +use Bugzilla::Util qw(detaint_natural trick_taint); use Bugzilla::WebService::Constants; use Bugzilla::Extension::PhabBugz::Constants; -use Bugzilla::Extension::PhabBugz::Util qw( - get_needs_review -); -use DateTime (); -use List::Util qw(first uniq); +use List::Util qw(first); use List::MoreUtils qw(any); use MIME::Base64 qw(decode_base64); use constant READ_ONLY => qw( check_user_enter_bug_permission check_user_permission_for_bug - needs_review ); use constant PUBLIC_METHODS => qw( check_user_enter_bug_permission check_user_permission_for_bug - needs_review set_build_target ); @@ -99,95 +93,6 @@ sub check_user_enter_bug_permission { }; } -sub needs_review { - my ($self, $params) = @_; - - $self->_check_phabricator(); - - my $user = Bugzilla->login(LOGIN_REQUIRED); - my $dbh = Bugzilla->dbh; - - my $reviews = get_needs_review(); - - my $authors = Bugzilla::Extension::PhabBugz::User->match({ - phids => [ - uniq - grep { defined } - map { $_->{fields}{authorPHID} } - @$reviews - ] - }); - - my %author_phab_to_id = map { $_->phid => $_->bugzilla_user->id } @$authors; - my %author_id_to_user = map { $_->bugzilla_user->id => $_->bugzilla_user } @$authors; - - # bug data - my $visible_bugs = $user->visible_bugs([ - uniq - grep { $_ } - map { $_->{fields}{'bugzilla.bug-id'} } - @$reviews - ]); - - # get all bug statuses and summaries in a single query to avoid creation of - # many bug objects - my %bugs; - if (@$visible_bugs) { - #<<< - my $bug_rows =$dbh->selectall_arrayref( - 'SELECT bug_id, bug_status, short_desc ' . - ' FROM bugs ' . - ' WHERE bug_id IN (' . join(',', ('?') x @$visible_bugs) . ')', - { Slice => {} }, - @$visible_bugs - ); - #>>> - %bugs = map { $_->{bug_id} => $_ } @$bug_rows; - } - - # build result - my $datetime_now = DateTime->now(time_zone => $user->timezone); - my @result; - foreach my $review (@$reviews) { - my $review_flat = { - id => $review->{id}, - title => $review->{fields}{title}, - url => Bugzilla->params->{phabricator_base_uri} . 'D' . $review->{id}, - }; - - # show date in user's timezone - my $datetime = DateTime->from_epoch( - epoch => $review->{fields}{dateModified}, - time_zone => 'UTC' - ); - $datetime->set_time_zone($user->timezone); - $review_flat->{updated} = $datetime->strftime('%Y-%m-%d %T %Z'); - $review_flat->{updated_fancy} = time_ago($datetime, $datetime_now); - - # review requester - if (my $author = $author_id_to_user{$author_phab_to_id{ $review->{fields}{authorPHID} }}) { - $review_flat->{author_name} = $author->name; - $review_flat->{author_email} = $author->email; - } - else { - $review_flat->{author_name} = 'anonymous'; - $review_flat->{author_email} = 'anonymous'; - } - - # referenced bug - if (my $bug_id = $review->{fields}{'bugzilla.bug-id'}) { - my $bug = $bugs{$bug_id}; - $review_flat->{bug_id} = $bug_id; - $review_flat->{bug_status} = $bug->{bug_status}; - $review_flat->{bug_summary} = $bug->{short_desc}; - } - - push @result, $review_flat; - } - - return { result => \@result }; -} - sub set_build_target { my ( $self, $params ) = @_; @@ -257,12 +162,6 @@ sub rest_resources { }, }, }, - # Review requests - qw{^/phabbugz/needs_review$}, { - GET => { - method => 'needs_review', - }, - } ]; } diff --git a/extensions/PhabBugz/t/basic.t b/extensions/PhabBugz/t/basic.t index 9a6723ccb..af92dc64f 100644 --- a/extensions/PhabBugz/t/basic.t +++ b/extensions/PhabBugz/t/basic.t @@ -17,6 +17,7 @@ use Test::More; use Test2::Tools::Mock; use Data::Dumper; use JSON::MaybeXS; +use Bugzilla::Test::Util qw(mock_useragent_tx); use Carp; use Try::Tiny; @@ -98,13 +99,13 @@ my $feed = Bugzilla::Extension::PhabBugz::Feed->new; # Same members in both do { - my $UserAgent = mock 'LWP::UserAgent' => ( + my $UserAgent = mock 'Mojo::UserAgent' => ( override => [ 'post' => sub { - my ($self, $url, $params) = @_; + my ($self, $url, undef, $params) = @_; my $data = decode_json($params->{params}); is_deeply($data->{transactions}, [], 'no-op'); - return mock({is_error => 0, content => '{}'}); + return mock_useragent_tx('{}'); }, ], ); @@ -119,14 +120,14 @@ do { # Project has members not in group do { - my $UserAgent = mock 'LWP::UserAgent' => ( + my $UserAgent = mock 'Mojo::UserAgent' => ( override => [ 'post' => sub { - my ($self, $url, $params) = @_; + my ($self, $url, undef, $params) = @_; my $data = decode_json($params->{params}); my $expected = [ { type => 'members.remove', value => ['foo'] } ]; is_deeply($data->{transactions}, $expected, 'remove foo'); - return mock({is_error => 0, content => '{}'}); + return mock_useragent_tx('{}'); }, ] ); @@ -139,14 +140,14 @@ do { # Group has members not in project do { - my $UserAgent = mock 'LWP::UserAgent' => ( + my $UserAgent = mock 'Mojo::UserAgent' => ( override => [ 'post' => sub { - my ($self, $url, $params) = @_; + my ($self, $url, undef, $params) = @_; my $data = decode_json($params->{params}); my $expected = [ { type => 'members.add', value => ['foo'] } ]; is_deeply($data->{transactions}, $expected, 'add foo'); - return mock({is_error => 0, content => '{}'}); + return mock_useragent_tx('{}'); }, ] ); @@ -164,10 +165,10 @@ do { 'update' => sub { 1 }, ], ); - my $UserAgent = mock 'LWP::UserAgent' => ( + my $UserAgent = mock 'Mojo::UserAgent' => ( override => [ 'post' => sub { - my ($self, $url, $params) = @_; + my ($self, $url, undef, $params) = @_; if ($url =~ /differential\.revision\.search/) { my $content = <<JSON; { @@ -215,10 +216,10 @@ do { } } JSON - return mock { is_error => 0, content => $content }; + return mock_useragent_tx($content); } else { - return mock { is_error => 1, message => "bad request" }; + return mock_useragent_tx("bad request"); } }, ], diff --git a/extensions/PhabBugz/t/feed-daemon-guts.t b/extensions/PhabBugz/t/feed-daemon-guts.t index 376af18e4..0c508be98 100644 --- a/extensions/PhabBugz/t/feed-daemon-guts.t +++ b/extensions/PhabBugz/t/feed-daemon-guts.t @@ -12,7 +12,7 @@ use lib qw( . lib local/lib/perl5 ); BEGIN { $ENV{LOG4PERL_CONFIG_FILE} = 'log4perl-t.conf' } use Bugzilla::Test::MockDB; use Bugzilla::Test::MockParams; -use Bugzilla::Test::Util qw(create_user); +use Bugzilla::Test::Util qw(create_user mock_useragent_tx); use Test::More; use Test2::Tools::Mock; use Try::Tiny; @@ -31,7 +31,7 @@ Bugzilla->error_mode(ERROR_MODE_TEST); my $phab_bot = create_user(PHAB_AUTOMATION_USER, '*'); -my $UserAgent = mock 'LWP::UserAgent' => (); +my $UserAgent = mock 'Mojo::UserAgent' => (); { SetParam('phabricator_enabled', 0); @@ -54,9 +54,9 @@ my $UserAgent = mock 'LWP::UserAgent' => (); } my @bad_response = ( - ['http error', mock({ is_error => 1, message => 'some http error' }) ], - ['invalid json', mock({ is_error => 0, content => '<xml>foo</xml>' })], - ['json containing error code', mock({ is_error => 0, content => encode_json({error_code => 1234 }) })], + ['http error', mock_useragent_tx("doesn't matter", sub { $_->code(500) }) ], + ['invalid json', mock_useragent_tx('<xml>foo</xml>') ], + ['json containing error code', mock_useragent_tx(encode_json({error_code => 1234 }))], ); SetParam(phabricator_enabled => 1); @@ -67,7 +67,7 @@ foreach my $bad_response (@bad_response) { my $feed = Bugzilla::Extension::PhabBugz::Feed->new; $UserAgent->override( post => sub { - my ( $self, $url, $params ) = @_; + my ( $self, $url, undef, $params ) = @_; return $bad_response->[1]; } ); |