From 8c33e82948c33b1b945d3afe40e4ca663a38533b Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Sat, 24 Oct 2026 23:16:17 -0400 Subject: [PATCH 1/5] Bug 2078938 - Message/Banner for people creating tickets under mozilla.org :: GitHub: Administration --- .../default/bug/create/github-admin.html.tmpl | 23 +++++++++++++++++++ .../hook/bug/create/create-end.html.tmpl | 1 + .../bug_modal/create-before_modules.html.tmpl | 2 +- 3 files changed, 25 insertions(+), 1 deletion(-) create mode 100644 extensions/BMO/template/en/default/bug/create/github-admin.html.tmpl diff --git a/extensions/BMO/template/en/default/bug/create/github-admin.html.tmpl b/extensions/BMO/template/en/default/bug/create/github-admin.html.tmpl new file mode 100644 index 0000000000..8c796194d6 --- /dev/null +++ b/extensions/BMO/template/en/default/bug/create/github-admin.html.tmpl @@ -0,0 +1,23 @@ +[%# This Source Code Form is subject to the terms of the Mozilla Public + # License, v. 2.0. If a copy of the MPL was not distributed with this + # file, You can obtain one at http://mozilla.org/MPL/2.0/. + # + # This Source Code Form is "Incompatible With Secondary Licenses", as + # defined by the Mozilla Public License, v. 2.0. + #%] + +[% RETURN UNLESS product.name == 'mozilla.org' %] + +
+
+ +
+
+

+ This request path for GHE support will be deprecated on October 30. Starting in November 2026, please + submit your GitHub Enterprise requests through Jira instead. + The new documentation hub can also be found in + Confluence. +

+
+
diff --git a/extensions/BMO/template/en/default/hook/bug/create/create-end.html.tmpl b/extensions/BMO/template/en/default/hook/bug/create/create-end.html.tmpl index 3719d0f597..c81c9df38e 100644 --- a/extensions/BMO/template/en/default/hook/bug/create/create-end.html.tmpl +++ b/extensions/BMO/template/en/default/hook/bug/create/create-end.html.tmpl @@ -7,3 +7,4 @@ #%] [% INCLUDE "bug/create/bugzilla-admin.html.tmpl" %] +[% INCLUDE "bug/create/github-admin.html.tmpl" %] diff --git a/extensions/BMO/template/en/default/hook/bug_modal/create-before_modules.html.tmpl b/extensions/BMO/template/en/default/hook/bug_modal/create-before_modules.html.tmpl index ed940541d7..6d69d31e23 100644 --- a/extensions/BMO/template/en/default/hook/bug_modal/create-before_modules.html.tmpl +++ b/extensions/BMO/template/en/default/hook/bug_modal/create-before_modules.html.tmpl @@ -7,5 +7,5 @@ #%] [% INCLUDE "bug/create/bugzilla-admin.html.tmpl" %] - +[% INCLUDE "bug/create/github-admin.html.tmpl" %] [% INCLUDE "bug/create/custom-forms-message.html.tmpl" %] From ef54f845e730c45297961fa1223d0626ee3fefdc Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Wed, 21 Oct 2026 18:05:21 -0400 Subject: [PATCH 2/5] Bumped version to 20261021.1 --- Bugzilla.pm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Bugzilla.pm b/Bugzilla.pm index eb8e437175..afbd732702 100644 --- a/Bugzilla.pm +++ b/Bugzilla.pm @@ -13,7 +13,7 @@ use warnings; use Bugzilla::Logging; -our $VERSION = '20261013.2'; +our $VERSION = '20261021.1'; use Bugzilla::Auth; use Bugzilla::Auth::Persist::Cookie; From 0727355ff3623fc17c53729cd6bb7dd0b6c4ee34 Mon Sep 17 00:00:00 2001 From: Xavier L'Hour Date: Thu, 22 Oct 2026 17:55:59 +0200 Subject: [PATCH 3/5] Bug 2081856 - Migrate Bug REST resource to native Mojo API: attachments (attachments, add_attachment, update_attachment) --- Bugzilla/API/V1/BugAttachment.pm | 488 +++++++ Bugzilla/WebService/Bug.pm | 1121 ++--------------- .../WebService/Server/REST/Resources/Bug.pm | 31 - qa/t/rest_bug_update_attachment.t | 113 ++ t/app-cgi-request-limit.t | 2 +- 5 files changed, 701 insertions(+), 1054 deletions(-) create mode 100644 Bugzilla/API/V1/BugAttachment.pm create mode 100644 qa/t/rest_bug_update_attachment.t diff --git a/Bugzilla/API/V1/BugAttachment.pm b/Bugzilla/API/V1/BugAttachment.pm new file mode 100644 index 0000000000..89ff9f9733 --- /dev/null +++ b/Bugzilla/API/V1/BugAttachment.pm @@ -0,0 +1,488 @@ +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this +# file, You can obtain one at http://mozilla.org/MPL/2.0/. +# +# This Source Code Form is "Incompatible With Secondary Licenses", as +# defined by the Mozilla Public License, v. 2.0. + +package Bugzilla::API::V1::BugAttachment; + +use 5.10.1; +use Mojo::Base qw( Mojolicious::Controller ); + +use List::MoreUtils qw(uniq); +use MIME::Base64 qw(decode_base64); + +use Bugzilla::API::V1::Util; +use Bugzilla::Attachment; +use Bugzilla::Bug; +use Bugzilla::Constants; +use Bugzilla::Error; +use Bugzilla::Flag; +use Bugzilla::Util qw(detaint_natural remote_ip trim); +use Bugzilla::WebService::Constants; +use Bugzilla::WebService::Util qw(extract_flags filter filter_wants + merge_request_params strip_request_params translate validate); + +use constant ATTACHMENT_MAPPED_SETTERS => + {file_name => 'filename', summary => 'description',}; + +use constant ATTACHMENT_MAPPED_RETURNS => { + description => 'summary', + ispatch => 'is_patch', + isprivate => 'is_private', + isobsolete => 'is_obsolete', + filename => 'file_name', + mimetype => 'content_type', +}; + +sub setup_routes { + my ($class, $r) = @_; + my $routes + = $r->under('/' => sub { Bugzilla->usage_mode(USAGE_MODE_MOJO_REST); }); + + $routes->get('/bug/#id/attachment')->to('V1::BugAttachment#attachments'); + $routes->post('/bug/#id/attachment')->to('V1::BugAttachment#add_attachment'); + $routes->get('/bug/attachment/#attachment_id') + ->to('V1::BugAttachment#attachments'); + $routes->put('/bug/attachment/#attachment_id') + ->to('V1::BugAttachment#update_attachment'); + + $routes->options('/bug/#id/attachment') + ->to('V1::BugAttachment#options', allow => 'GET, POST'); + $routes->options('/bug/attachment/#attachment_id') + ->to('V1::BugAttachment#options', allow => 'GET, PUT'); +} + +sub options { + my ($self) = @_; + + my $allow = $self->stash('allow'); + $self->res->headers->header('Allow' => $allow); + $self->res->headers->header('Access-Control-Allow-Methods' => $allow); + + return $self->rendered(200); +} + +# Same output as type() on the legacy JSON-RPC/REST server. +sub type { + my ($self, $type, $value) = @_; + return Bugzilla::API::V1::Util->type($type, $value); +} + +# GET /bug//attachment and /bug/attachment/ +sub attachments { + my ($self) = @_; + + my $user = $self->bugzilla->login; + + my $params = {}; + for my $name (qw(ids attachment_ids include_fields exclude_fields)) { + my $values = $self->every_param($name); + $params->{$name} = $values if @$values; + } + _split_include_exclude($params); + + # As on the legacy REST server, an id in the path is added to the ones from + # the query string. + for my $pair (['id', 'ids'], ['attachment_id', 'attachment_ids']) { + my ($stash, $key) = @$pair; + next if !defined(my $id = $self->stash($stash)); + $params->{$key} = [uniq($id, @{$params->{$key} || []})]; + } + + Bugzilla->switch_to_shadow_db() unless $user->id; + + my $ids = $params->{ids} || []; + my $attach_ids = $params->{attachment_ids} || []; + + unless ($user->id) { + Bugzilla->check_rate_limit("get_attachments", remote_ip()); + } + + my %bugs; + foreach my $bug_id (@$ids) { + my $bug = Bugzilla::Bug->check($bug_id); + $bugs{$bug->id} = []; + foreach my $attach (@{$bug->attachments}) { + push @{$bugs{$bug->id}}, $self->_attachment_to_hash($attach, $params); + } + } + + my %attachments; + my @log_attachments; + foreach my $attach (@{Bugzilla::Attachment->new_from_list($attach_ids)}) { + Bugzilla::Bug->check($attach->bug_id); + if ($attach->isprivate && !$user->is_insider) { + return $self->user_error('auth_failure', + {action => 'access', object => 'attachment', attach_id => $attach->id}); + } + push @log_attachments, $attach; + + $attachments{$attach->id} = $self->_attachment_to_hash($attach, $params); + } + + if ($user->id) { + foreach my $attachment (@log_attachments) { + Bugzilla->log_user_request($attachment->bug_id, $attachment->id, + "attachment-get"); + } + } + + return $self->render(json => {bugs => \%bugs, attachments => \%attachments}); +} + +# POST /bug//attachment +sub add_attachment { + my ($self) = @_; + + # BMO: Don't allow updating of bugs if disabled. The legacy REST server + # answered this with the HTML "updates disabled" page, as ThrowErrorPage() + # only returns an error object in JSON-RPC mode. Return the JSON error an API + # client can handle, with the generic code user_error() uses for an error + # that has no code of its own. + if (Bugzilla->params->{disable_bug_updates}) { + return $self->render( + json => { + error => 1, + code => ERROR_UNKNOWN_TRANSIENT, + message => 'Bug updates are currently disabled.', + documentation => 'https://bmo.readthedocs.io/en/latest/api/', + }, + status => REST_STATUS_CODE_MAP()->{_default} + ); + } + + my $user = $self->bugzilla->login; + $user->id || return $self->user_error('login_required'); + + my ($params, $error) = $self->_request_params; + return $self->user_error($error) if $error; + + defined $params->{data} + || return $self->code_error('param_required', {param => 'data'}); + $params->{data} = decode_base64($params->{data}); + + my $dbh = Bugzilla->dbh; + + my @bugs = map { Bugzilla::Bug->check($_) } @{$params->{ids}}; + foreach my $bug (@bugs) { + $user->can_edit_product($bug->product_id) + || return $self->user_error("product_edit_denied", + {product => $bug->product}); + } + + my @created; + $dbh->bz_start_transaction(); + my $timestamp = $dbh->selectrow_array('SELECT LOCALTIMESTAMP(0)'); + + my $flags = delete $params->{flags}; + my $comment = delete $params->{comment}; + my $is_markdown = delete $params->{is_markdown}; + my $bug_flags = delete $params->{bug_flags}; + + $comment = $comment ? trim($comment) : ''; + + # Default to the system use_markdown setting, matching attachment.cgi. + if (!defined $is_markdown) { + $is_markdown = Bugzilla->params->{use_markdown} ? 1 : 0; + } + + foreach my $bug (@bugs) { + my $attachment = Bugzilla::Attachment->create({ + bug => $bug, + creation_ts => $timestamp, + data => $params->{data}, + description => $params->{summary}, + filename => $params->{file_name}, + mimetype => $params->{content_type}, + ispatch => $params->{is_patch}, + isprivate => $params->{is_private}, + }); + + if ($flags) { + my ($old_flags, $new_flags) = extract_flags($flags, $bug, $attachment); + $attachment->set_flags($old_flags, $new_flags); + } + + $attachment->update($timestamp); + + # The comment has to be added even if it's empty + $bug->add_comment( + $comment, + { + isprivate => $attachment->isprivate, + type => CMT_ATTACHMENT_CREATED, + extra_data => $attachment->id, + is_markdown => ($is_markdown ? 1 : 0), + } + ); + + if ($bug_flags) { + my ($old_flags, $new_flags) = extract_flags($bug_flags, $bug); + $bug->set_flags($old_flags, $new_flags); + } + + push(@created, $attachment); + } + $_->bug->update($timestamp) foreach @created; + $dbh->bz_commit_transaction(); + + $_->send_changes() foreach @bugs; + + my %attachments + = map { $_->id => $self->_attachment_to_hash($_, $params) } @created; + + return $self->render(json => {attachments => \%attachments}, status => 201); +} + +# PUT /bug/attachment/ +sub update_attachment { + my ($self) = @_; + + my $user = $self->bugzilla->login; + $user->id || return $self->user_error('login_required'); + + my ($params, $error) = $self->_request_params; + return $self->user_error($error) if $error; + + my $dbh = Bugzilla->dbh; + my $ids = delete $params->{ids}; + + # Drop the request-level keys, which are not attachment fields, so that + # set_all() still raises unknown_method on an unrecognized field. + $params = translate(strip_request_params($params), ATTACHMENT_MAPPED_SETTERS); + + # Get all the attachments, after verifying that they exist and are editable + my @attachments = (); + my %bugs = (); + foreach my $id (@$ids) { + detaint_natural($id) + || return $self->code_error('param_must_be_numeric', + {function => 'Bug.update_attachment', param => 'ids'}); + my $attachment = Bugzilla::Attachment->new($id) + || return $self->user_error("invalid_attach_id", {attach_id => $id}); + my $bug = $attachment->bug; + $attachment->_check_bug; + + push @attachments, $attachment; + $bugs{$bug->id} = $bug; + } + + my $flags = delete $params->{flags}; + my $comment = delete $params->{comment}; + my $is_markdown = delete $params->{is_markdown}; + my $bug_flags = delete $params->{bug_flags}; + + $comment = $comment ? trim($comment) : ''; + + # Default to the system use_markdown setting, matching attachment.cgi. + if (!defined $is_markdown) { + $is_markdown = Bugzilla->params->{use_markdown} ? 1 : 0; + } + + # Update the values + foreach my $attachment (@attachments) { + my ($update_flags, $new_flags) + = $flags ? extract_flags($flags, $attachment->bug, $attachment) : ([], []); + if ($attachment->validate_can_edit) { + $attachment->set_all($params); + $attachment->set_flags($update_flags, $new_flags) if $flags; + } + elsif (scalar @$update_flags && !scalar(@$new_flags) && !scalar keys %$params) { + + # Requestees can set flags targeted to them, even if they cannot + # edit the attachment. Flag setters can edit their own flags too. + my %flag_list = map { $_->{id} => $_ } @$update_flags; + my $flag_objs = Bugzilla::Flag->new_from_list([keys %flag_list]); + my @editable_flags; + foreach my $flag_obj (@$flag_objs) { + if ($flag_obj->setter_id == $user->id + || ($flag_obj->requestee_id && $flag_obj->requestee_id == $user->id)) + { + push(@editable_flags, $flag_list{$flag_obj->id}); + } + } + if (!scalar @editable_flags) { + return $self->user_error("illegal_attachment_edit", + {attach_id => $attachment->id}); + } + $attachment->set_flags(\@editable_flags, []); + } + else { + return $self->user_error("illegal_attachment_edit", + {attach_id => $attachment->id}); + } + } + + $dbh->bz_start_transaction(); + + # Do the actual update and get information to return to user + my @result; + foreach my $attachment (@attachments) { + my $changes = $attachment->update(); + my $bug = $attachment->bug; + + if ($comment) { + $bug->add_comment( + $comment, + { + isprivate => $attachment->isprivate, + type => CMT_ATTACHMENT_UPDATED, + extra_data => $attachment->id, + is_markdown => ($is_markdown ? 1 : 0), + } + ); + } + + if ($bug_flags) { + my ($old_flags, $new_flags) = extract_flags($bug_flags, $bug); + $bug->set_flags($old_flags, $new_flags); + } + + $changes = translate($changes, ATTACHMENT_MAPPED_RETURNS); + + my %hash = ( + id => $self->type('int', $attachment->id), + last_change_time => $self->type('dateTime', $attachment->modification_time), + changes => {}, + ); + + foreach my $field (keys %$changes) { + my $change = $changes->{$field}; + + # We normalize undef to an empty string, so that the API + # stays consistent for things like Deadline that can become + # empty. + $hash{changes}->{$field} = { + removed => $self->type('string', $change->[0] // ''), + added => $self->type('string', $change->[1] // '') + }; + } + + push(@result, \%hash); + } + + $dbh->bz_commit_transaction(); + + # Email users about the change + foreach my $bug (values %bugs) { + $bug->update(); + $bug->send_changes(); + } + + # Return the information to the user + return $self->render(json => {attachments => \@result}); +} + +# Parameters of a POST or PUT: the query string and the request body, as on +# the legacy REST server. The id in the path is only used when neither of +# them has "ids". +sub _request_params { + my ($self) = @_; + + my ($params, $error) + = merge_request_params($self, [qw(ids include_fields exclude_fields)]); + return (undef, $error) if $error; + + # As legacy validate(@_, 'ids'): a single id becomes a list, and anything + # but a list of plain values (e.g. a hash of conditions) is invalid_params. + (undef, $params) = validate($self, $params, 'ids'); + $params->{ids} //= [$self->stash('id') // $self->stash('attachment_id')]; + _split_include_exclude($params); + + return ($params, undef); +} + +sub _split_include_exclude { + my ($params) = @_; + for my $name (qw(include_fields exclude_fields)) { + next if !exists $params->{$name}; + $params->{$name} = [$params->{$name}] if !ref $params->{$name}; + $params->{$name} = [map { split(/[\s,]+/) } @{$params->{$name}}]; + } +} + +sub _user_to_hash { + my ($self, $user, $filters, $types, $prefix) = @_; + my $item = filter $filters, + { + id => $self->type('int', $user->id), + real_name => $self->type('string', $user->name), + nick => $self->type('string', $user->nick), + name => $self->type('email', $user->login), + email => $self->type('email', $user->email), + }, + $types, $prefix; + return $item; +} + +sub _attachment_to_hash { + my ($self, $attach, $filters, $types, $prefix) = @_; + + my $item = filter $filters, + { + creation_time => $self->type('dateTime', $attach->attached), + last_change_time => $self->type('dateTime', $attach->modification_time), + id => $self->type('int', $attach->id), + bug_id => $self->type('int', $attach->bug_id), + file_name => $self->type('string', $attach->filename), + summary => $self->type('string', $attach->description), + description => $self->type('string', $attach->description), + content_type => $self->type('string', $attach->contenttype), + is_private => $self->type('int', $attach->isprivate), + is_obsolete => $self->type('int', $attach->isobsolete), + is_patch => $self->type('int', $attach->ispatch), + }, + $types, $prefix; + + # creator/attacher require an extra lookup, so we only send them if + # the filter wants them. + foreach my $field (qw(creator attacher)) { + if (filter_wants $filters, $field, $types, $prefix) { + $item->{$field} = $self->type('email', $attach->attacher->login); + } + } + + if (filter_wants $filters, 'creator', $types, $prefix) { + $item->{'creator_detail'} + = $self->_user_to_hash($attach->attacher, $filters, undef, 'creator'); + } + + if (filter_wants $filters, 'data', $types, $prefix) { + $item->{'data'} = $self->type('base64', $attach->data); + } + + if (filter_wants $filters, 'size', $types, $prefix) { + $item->{'size'} = $self->type('int', $attach->datasize); + } + + if (filter_wants $filters, 'flags', $types, $prefix) { + $item->{'flags'} = [map { $self->_flag_to_hash($_) } @{$attach->flags}]; + } + + return $item; +} + +sub _flag_to_hash { + my ($self, $flag) = @_; + + my $item = { + id => $self->type('int', $flag->id), + name => $self->type('string', $flag->name), + type_id => $self->type('int', $flag->type_id), + creation_date => $self->type('dateTime', $flag->creation_date), + modification_date => $self->type('dateTime', $flag->modification_date), + status => $self->type('string', $flag->status) + }; + + foreach my $field (qw(setter requestee)) { + my $field_id = $field . "_id"; + $item->{$field} = $self->type('email', $flag->$field->login) + if $flag->$field_id; + } + + return $item; +} + +1; diff --git a/Bugzilla/WebService/Bug.pm b/Bugzilla/WebService/Bug.pm index a0a4acfe36..07d98d4f88 100644 --- a/Bugzilla/WebService/Bug.pm +++ b/Bugzilla/WebService/Bug.pm @@ -59,40 +59,22 @@ sub DATE_FIELDS { return $fields; } -use constant BASE64_FIELDS => {add_attachment => ['data'],}; - use constant READ_ONLY => qw( - attachments get history search ); use constant PUBLIC_METHODS => qw( - add_attachment - attachments create get history possible_duplicates search update - update_attachment update_see_also ); -use constant ATTACHMENT_MAPPED_SETTERS => - {file_name => 'filename', summary => 'description',}; - -use constant ATTACHMENT_MAPPED_RETURNS => { - description => 'summary', - ispatch => 'is_patch', - isprivate => 'is_private', - isobsolete => 'is_obsolete', - filename => 'file_name', - mimetype => 'content_type', -}; - our %api_field_types = ( %{{map { $_ => 'double' } Bugzilla::Bug::NUMERIC_COLUMNS()}}, %{{map { $_ => 'dateTime' } Bugzilla::Bug::DATE_COLUMNS()}}, @@ -705,228 +687,6 @@ sub create { return {id => $self->type('int', $bug->bug_id)}; } -sub add_attachment { - my ($self, $params) = validate(@_, 'ids'); - my $dbh = Bugzilla->dbh; - - # BMO: Don't allow updating of bugs if disabled - if (Bugzilla->params->{disable_bug_updates}) { - ThrowErrorPage( - 'bug/process/updates-disabled.html.tmpl', - 'Bug updates are currently disabled.' - ); - } - - Bugzilla->login(LOGIN_REQUIRED); - defined $params->{ids} || ThrowCodeError('param_required', {param => 'ids'}); - defined $params->{data} || ThrowCodeError('param_required', {param => 'data'}); - - my @bugs = map { Bugzilla::Bug->check($_) } @{$params->{ids}}; - foreach my $bug (@bugs) { - Bugzilla->user->can_edit_product($bug->product_id) - || ThrowUserError("product_edit_denied", {product => $bug->product}); - } - - my @created; - $dbh->bz_start_transaction(); - my $timestamp = $dbh->selectrow_array('SELECT LOCALTIMESTAMP(0)'); - - my $flags = delete $params->{flags}; - my $comment = delete $params->{comment}; - my $is_markdown = delete $params->{is_markdown}; - my $bug_flags = delete $params->{bug_flags}; - - $comment = $comment ? trim($comment) : ''; - - # Default to the system use_markdown setting, matching attachment.cgi. - if (!defined $is_markdown) { - $is_markdown = Bugzilla->params->{use_markdown} ? 1 : 0; - } - - foreach my $bug (@bugs) { - my $attachment = Bugzilla::Attachment->create({ - bug => $bug, - creation_ts => $timestamp, - data => $params->{data}, - description => $params->{summary}, - filename => $params->{file_name}, - mimetype => $params->{content_type}, - ispatch => $params->{is_patch}, - isprivate => $params->{is_private}, - }); - - if ($flags) { - my ($old_flags, $new_flags) = extract_flags($flags, $bug, $attachment); - $attachment->set_flags($old_flags, $new_flags); - } - - $attachment->update($timestamp); - - # The comment has to be added even if it's empty - $bug->add_comment( - $comment, - { - isprivate => $attachment->isprivate, - type => CMT_ATTACHMENT_CREATED, - extra_data => $attachment->id, - is_markdown => ($is_markdown ? 1 : 0), - } - ); - - if ($bug_flags) { - my ($old_flags, $new_flags) = extract_flags($bug_flags, $bug); - $bug->set_flags($old_flags, $new_flags); - } - - push(@created, $attachment); - } - $_->bug->update($timestamp) foreach @created; - $dbh->bz_commit_transaction(); - - $_->send_changes() foreach @bugs; - - my %attachments - = map { $_->id => $self->_attachment_to_hash($_, $params) } @created; - - return {attachments => \%attachments}; -} - -sub update_attachment { - my ($self, $params) = validate(@_, 'ids'); - - my $user = Bugzilla->login(LOGIN_REQUIRED); - my $dbh = Bugzilla->dbh; - - my $ids = delete $params->{ids}; - defined $ids || ThrowCodeError('param_required', {param => 'ids'}); - - # Some fields cannot be sent to set_all - foreach my $key (qw(login password token)) { - delete $params->{$key}; - } - - $params = translate($params, ATTACHMENT_MAPPED_SETTERS); - - # Get all the attachments, after verifying that they exist and are editable - my @attachments = (); - my %bugs = (); - foreach my $id (@$ids) { - detaint_natural($id) - || ThrowCodeError('param_must_be_numeric', - {function => 'Bug.update_attachment', param => 'ids'}); - my $attachment = Bugzilla::Attachment->new($id) - || ThrowUserError("invalid_attach_id", {attach_id => $id}); - my $bug = $attachment->bug; - $attachment->_check_bug; - - push @attachments, $attachment; - $bugs{$bug->id} = $bug; - } - - my $flags = delete $params->{flags}; - my $comment = delete $params->{comment}; - my $is_markdown = delete $params->{is_markdown}; - my $bug_flags = delete $params->{bug_flags}; - - $comment = $comment ? trim($comment) : ''; - - # Default to the system use_markdown setting, matching attachment.cgi. - if (!defined $is_markdown) { - $is_markdown = Bugzilla->params->{use_markdown} ? 1 : 0; - } - - # Update the values - foreach my $attachment (@attachments) { - my ($update_flags, $new_flags) - = $flags ? extract_flags($flags, $attachment->bug, $attachment) : ([], []); - if ($attachment->validate_can_edit) { - $attachment->set_all($params); - $attachment->set_flags($update_flags, $new_flags) if $flags; - } - elsif (scalar @$update_flags && !scalar(@$new_flags) && !scalar keys %$params) { - - # Requestees can set flags targeted to them, even if they cannot - # edit the attachment. Flag setters can edit their own flags too. - my %flag_list = map { $_->{id} => $_ } @$update_flags; - my $flag_objs = Bugzilla::Flag->new_from_list([keys %flag_list]); - my @editable_flags; - foreach my $flag_obj (@$flag_objs) { - if ($flag_obj->setter_id == $user->id - || ($flag_obj->requestee_id && $flag_obj->requestee_id == $user->id)) - { - push(@editable_flags, $flag_list{$flag_obj->id}); - } - } - if (!scalar @editable_flags) { - ThrowUserError("illegal_attachment_edit", {attach_id => $attachment->id}); - } - $attachment->set_flags(\@editable_flags, []); - } - else { - ThrowUserError("illegal_attachment_edit", {attach_id => $attachment->id}); - } - } - - $dbh->bz_start_transaction(); - - # Do the actual update and get information to return to user - my @result; - foreach my $attachment (@attachments) { - my $changes = $attachment->update(); - my $bug = $attachment->bug; - - if ($comment) { - $bug->add_comment( - $comment, - { - isprivate => $attachment->isprivate, - type => CMT_ATTACHMENT_UPDATED, - extra_data => $attachment->id, - is_markdown => ($is_markdown ? 1 : 0), - } - ); - } - - if ($bug_flags) { - my ($old_flags, $new_flags) = extract_flags($bug_flags, $bug); - $bug->set_flags($old_flags, $new_flags); - } - - $changes = translate($changes, ATTACHMENT_MAPPED_RETURNS); - - my %hash = ( - id => $self->type('int', $attachment->id), - last_change_time => $self->type('dateTime', $attachment->modification_time), - changes => {}, - ); - - foreach my $field (keys %$changes) { - my $change = $changes->{$field}; - - # We normalize undef to an empty string, so that the API - # stays consistent for things like Deadline that can become - # empty. - $hash{changes}->{$field} = { - removed => $self->type('string', $change->[0] // ''), - added => $self->type('string', $change->[1] // '') - }; - } - - push(@result, \%hash); - } - - $dbh->bz_commit_transaction(); - - # Email users about the change - foreach my $bug (values %bugs) { - $bug->update(); - $bug->send_changes(); - } - - # Return the information to the user - return {attachments => \@result}; -} - sub update_see_also { my ($self, $params) = @_; @@ -980,55 +740,6 @@ sub update_see_also { return {changes => \%changes}; } -sub attachments { - my ($self, $params) = validate(@_, 'ids', 'attachment_ids'); - - Bugzilla->switch_to_shadow_db() unless Bugzilla->user->id; - - if (!(defined $params->{ids} or defined $params->{attachment_ids})) { - ThrowCodeError('param_required', - {function => 'Bug.attachments', params => ['ids', 'attachment_ids']}); - } - - my $ids = $params->{ids} || []; - my $attach_ids = $params->{attachment_ids} || []; - - unless (Bugzilla->user->id) { - Bugzilla->check_rate_limit("get_attachments", remote_ip()); - } - - my %bugs; - foreach my $bug_id (@$ids) { - my $bug = Bugzilla::Bug->check($bug_id); - $bugs{$bug->id} = []; - foreach my $attach (@{$bug->attachments}) { - push @{$bugs{$bug->id}}, $self->_attachment_to_hash($attach, $params); - } - } - - my %attachments; - my @log_attachments; - foreach my $attach (@{Bugzilla::Attachment->new_from_list($attach_ids)}) { - Bugzilla::Bug->check($attach->bug_id); - if ($attach->isprivate && !Bugzilla->user->is_insider) { - ThrowUserError('auth_failure', - {action => 'access', object => 'attachment', attach_id => $attach->id}); - } - push @log_attachments, $attach; - - $attachments{$attach->id} = $self->_attachment_to_hash($attach, $params); - } - - if (Bugzilla->user->id) { - foreach my $attachment (@log_attachments) { - Bugzilla->log_user_request($attachment->bug_id, $attachment->id, - "attachment-get"); - } - } - - return {bugs => \%bugs, attachments => \%attachments}; -} - ############################## # Private Helper Subroutines # ############################## @@ -1473,424 +1184,188 @@ where applicable. =head1 Bug Information -=head2 attachments +=head2 get -B +B =over =item B -It allows you to get data about attachments, given a list of bugs -and/or attachment ids. +Gets information about particular bugs in the database. -B: Private attachments will only be returned if you are in the -insidergroup or if you are the submitter of the attachment. +Note: Can also be called as "get_bugs" for compatibility with Bugzilla 3.0 API. =item B -To get all current attachments for a bug: - -GET /bug//attachment - -To get a specific attachment based on attachment ID: +To get information about a particular bug using its ID or alias: -GET /bug/attachment/ +GET /bug/ The returned data format is the same as below. =item B -B: At least one of C or C is required. +In addition to the parameters below, this method also accepts the +standard L and +L arguments. =over =item C -See the description of the C parameter in the L method. +An array of numbers and strings. + +If an element in the array is entirely numeric, it represents a bug_id +from the Bugzilla database to fetch. If it contains any non-numeric +characters, it is considered to be a bug alias instead, and the bug with +that alias will be loaded. + +Note that it's possible for aliases to be disabled in Bugzilla, in which +case you will be told that you have specified an invalid bug_id if you +try to specify an alias. (It will be error 100.) -=item C +=item C B -C An array of integer attachment ids. +C Normally, if you request any inaccessible or invalid bug ids, +Bug.get will throw an error. If this parameter is True, instead of throwing an +error we return an array of hashes with a C, C and C +for each bug that fails, and return normal information for the other bugs that +were accessible. =back -Also accepts the L, -and L arguments. - =item B -A hash containing two elements: C and C. The return -value looks like this: +Two items are returned: - { - bugs => { - 1345 => [ - { (attachment) }, - { (attachment) } - ], - 9874 => [ - { (attachment) }, - { (attachment) } - ], - }, - - attachments => { - 234 => { (attachment) }, - 123 => { (attachment) }, - } - } +=over -The attachments of any bugs that you specified in the C argument in -input are returned in C on output. C is a hash that has integer -bug IDs for keys and the values are arrayrefs that contain hashes as attachments. -(Fields for attachments are described below.) +=item C -For any attachments that you specified directly in C, they -are returned in C on output. This is a hash where the attachment -ids point directly to hashes describing the individual attachment. +An array of hashes that contains information about the bugs with +the valid ids. Each hash contains the following items: -The fields for each attachment (where it says C<(attachment)> in the -diagram above) are: +These fields are returned by default or by specifying C<_default> +in C. =over -=item C +=item C -C The raw data of the attachment, encoded as Base64. +C The total number of hours that this bug has taken (so far). -=item C +If you are not in the time-tracking group, this field will not be included +in the return value. -C The length (in bytes) of the attachment. +=item C -=item C +C The unique alias of this bug. -C The time the attachment was created. +=item C -=item C +C The login name of the user to whom the bug is assigned. -C The last time the attachment was modified. +=item C -=item C +C A hash containing detailed user information for the assigned_to. To see the +keys included in the user detail hash, see below. -C The numeric id of the attachment. +=item C -=item C +C of hashes containing attachment details of the bug, in the same format +as CidE/attachment> (see the REST API documentation). -C The numeric id of the bug that the attachment is attached to. +This is an B field returned only by specifying C or +C<_extra> in C. -=item C +=item C -C The file name of the attachment. +C of Cs. The ids of bugs that are "blocked" by this bug. -=item C +=item C -C A short string describing the attachment. +C of Cs. The login names of users on the CC list of this +bug. -Also returned as C, for backwards-compatibility with older -Bugzillas. (However, this backwards-compatibility will go away in Bugzilla -5.0.) +=item C -=item C +C of hashes containing detailed user information for each of the cc list +members. To see the keys included in the user detail hash, see below. -C The MIME type of the attachment. +=item C -=item C +C The name of the current classification the bug is in. -C True if the attachment is private (only visible to a certain -group called the "insidergroup"), False otherwise. +=item C -=item C +C The number of the comments left on this bug. -C True if the attachment is obsolete, False otherwise. +=item C -=item C +C of hashes containing comment details of the bug. See the comments +section above for the object format. -C True if the attachment is a patch, False otherwise. +This is an B field returned only by specifying C or C<_extra> +in C. -=item C +Comments that are collapsed in the web UI are left out unless +C<_collapsed_comments> is also passed in C, as described for +CidE/comment> in the REST API documentation. -C The login name of the user that created the attachment. +=item C -Also returned as C, for backwards-compatibility with older -Bugzillas. (However, this backwards-compatibility will go away in Bugzilla -5.0.) +C The name of the current component of this bug. -=item C +=item C -C A hash containing detailed user information for the creator. The keys -included in the user detail hash are the same as ones returned for the Get Bug -method. +B -=item C +C A hash containing the numbers of the items in the following fields: +C, C, C, C, C, C, +C, C and C. -An array of hashes containing the information about flags currently set -for each attachment. Each flag hash contains the following items: +This is an B field returned only by specifying C or C<_extra> in +C. -=over +=item C -=item C +C When the bug was created. -C The id of the flag. +=item C -=item C +C The login name of the person who filed this bug (the reporter). -C The name of the flag. +=item C -=item C +C A hash containing detailed user information for the creator. To see the +keys included in the user detail hash, see below. -C The type id of the flag. +=item C -=item C +C The day that this bug is due to be completed, in the format +C. -C The timestamp when this flag was originally created. +If you are not in the time-tracking group, this field will not be included +in the return value. -=item C +=item C -C The timestamp when the flag was last modified. +C of Cs. The ids of bugs that this bug "depends on". -=item C +=item C -C The current status of the flag. +C The description (initial comment) of the bug. -=item C +This is an B field returned only by specifying C or +C<_extra> in C. -C The login name of the user who created or last modified the flag. +=item C -=item C +C The bug ID of the bug that this bug is a duplicate of. If this bug +isn't a duplicate of any bug, this will be null. -C The login name of the user this flag has been requested to be granted or denied. -Note, this field is only returned if a requestee is set. - -=back - -=back - -=item B - -This method can throw all the same errors as L. In addition, -it can also throw the following error: - -=over - -=item 304 (Auth Failure, Attachment is Private) - -You specified the id of a private attachment in the C -argument, and you are not in the "insider group" that can see -private attachments. - -=back - -=item B - -=over - -=item Added in Bugzilla B<3.6>. - -=item In Bugzilla B<4.0>, the C return value was renamed to -C. - -=item In Bugzilla B<4.0>, the C return value was renamed to -C. - -=item The C return value was added in Bugzilla B<4.0>. - -=item In Bugzilla B<4.2>, the C return value was removed -(this attribute no longer exists for attachments). - -=back - -=item The C array was added in Bugzilla B<4.4>. - -=item REST API call added in Bugzilla B<5.0>. - -=item The C field was added in Bugzilla B<6.0>. - -=back - - -=head2 get - -B - -=over - -=item B - -Gets information about particular bugs in the database. - -Note: Can also be called as "get_bugs" for compatibility with Bugzilla 3.0 API. - -=item B - -To get information about a particular bug using its ID or alias: - -GET /bug/ - -The returned data format is the same as below. - -=item B - -In addition to the parameters below, this method also accepts the -standard L and -L arguments. - -=over - -=item C - -An array of numbers and strings. - -If an element in the array is entirely numeric, it represents a bug_id -from the Bugzilla database to fetch. If it contains any non-numeric -characters, it is considered to be a bug alias instead, and the bug with -that alias will be loaded. - -Note that it's possible for aliases to be disabled in Bugzilla, in which -case you will be told that you have specified an invalid bug_id if you -try to specify an alias. (It will be error 100.) - -=item C B - -C Normally, if you request any inaccessible or invalid bug ids, -Bug.get will throw an error. If this parameter is True, instead of throwing an -error we return an array of hashes with a C, C and C -for each bug that fails, and return normal information for the other bugs that -were accessible. - -=back - -=item B - -Two items are returned: - -=over - -=item C - -An array of hashes that contains information about the bugs with -the valid ids. Each hash contains the following items: - -These fields are returned by default or by specifying C<_default> -in C. - -=over - -=item C - -C The total number of hours that this bug has taken (so far). - -If you are not in the time-tracking group, this field will not be included -in the return value. - -=item C - -C The unique alias of this bug. - -=item C - -C The login name of the user to whom the bug is assigned. - -=item C - -C A hash containing detailed user information for the assigned_to. To see the -keys included in the user detail hash, see below. - -=item C - -C of hashes containing attachment details of the bug. See the attachments -section above for the object format. - -This is an B field returned only by specifying C or -C<_extra> in C. - -=item C - -C of Cs. The ids of bugs that are "blocked" by this bug. - -=item C - -C of Cs. The login names of users on the CC list of this -bug. - -=item C - -C of hashes containing detailed user information for each of the cc list -members. To see the keys included in the user detail hash, see below. - -=item C - -C The name of the current classification the bug is in. - -=item C - -C The number of the comments left on this bug. - -=item C - -C of hashes containing comment details of the bug. See the comments -section above for the object format. - -This is an B field returned only by specifying C or C<_extra> -in C. - -Comments that are collapsed in the web UI are left out unless -C<_collapsed_comments> is also passed in C, as described for -CidE/comment> in the REST API documentation. - -=item C - -C The name of the current component of this bug. - -=item C - -B - -C A hash containing the numbers of the items in the following fields: -C, C, C, C, C, C, -C, C and C. - -This is an B field returned only by specifying C or C<_extra> in -C. - -=item C - -C When the bug was created. - -=item C - -C The login name of the person who filed this bug (the reporter). - -=item C - -C A hash containing detailed user information for the creator. To see the -keys included in the user detail hash, see below. - -=item C - -C The day that this bug is due to be completed, in the format -C. - -If you are not in the time-tracking group, this field will not be included -in the return value. - -=item C - -C of Cs. The ids of bugs that this bug "depends on". - -=item C - -C The description (initial comment) of the bug. - -This is an B field returned only by specifying C or -C<_extra> in C. - -=item C - -C The bug ID of the bug that this bug is a duplicate of. If this bug -isn't a duplicate of any bug, this will be null. - -=item C +=item C C of Cs. The ids of bugs that are marked as duplicate of this bug. @@ -2961,404 +2436,6 @@ loop errors had a generic code of C<32000>. =back -=head2 add_attachment - -B - -=over - -=item B - -This allows you to add an attachment to a bug in Bugzilla. - -=item B - -To create attachment on a current bug: - -POST /bug//attachment - -The params to include in the POST body, as well as the returned -data format are the same as below. The C param will be -overridden as it it pulled from the URL path. - -=item B - -=over - -=item C - -B C An array of ints and/or strings--the ids -or aliases of bugs that you want to add this attachment to. -The same attachment and comment will be added to all -these bugs. - -=item C - -B C or C The content of the attachment. -If the content of the attachment is not ASCII text, you must encode -it in base64 and declare it as the C type. - -=item C - -B C The "file name" that will be displayed -in the UI for this attachment. - -=item C - -B C A short string describing the -attachment. - -=item C - -B C The MIME type of the attachment, like -C or C. - -=item C - -C A comment to add along with this attachment. - -=item C - -C True if Bugzilla should treat this attachment as a patch. -If you specify this, you do not need to specify a C. -The C of the attachment will be forced to C. - -Defaults to False if not specified. - -=item C - -C True if the attachment should be private (restricted -to the "insidergroup"), False if the attachment should be public. - -Defaults to False if not specified. - -=item C - -C An array of hashes with flags to add to the attachment. to create a flag, -at least the status and the type_id or name must be provided. An optional requestee -can be passed if the flag type is requestable to a specific user. - -=over - -=item C - -C The name of the flag type. - -=item C - -C The internal flag type id. - -=item C - -C The flags new status (i.e. "?", "+", "-" or "X" to clear a flag). - -=item C - -C The login of the requestee if the flag type is requestable to a specific user. - -=back - -=item C - -C An optional array of hashes with flags to add to the attachment's -bug. See the C param for the L method for the object format. - -=back - -=item B - -A single item C, which contains the created -attachments in the same format as the C return -value from L. - -=item B - -This method can throw all the same errors as L, plus: - -=over - -=item 129 (Flag Status Invalid) - -The flag status is invalid. - -=item 130 (Flag Modification Denied) - -You tried to request, grant, or deny a flag but only a user with the required -permissions may make the change. - -=item 131 (Flag not Requestable from Specific Person) - -You can't ask a specific person for the flag. - -=item 133 (Flag Type not Unique) - -The flag type specified matches several flag types. You must specify -the type id value to update or add a flag. - -=item 134 (Inactive Flag Type) - -The flag type is inactive and cannot be used to create new flags. - -=item 600 (Attachment Too Large) - -You tried to attach a file that was larger than Bugzilla will accept. - -=item 601 (Invalid MIME Type) - -You specified a C argument that was blank, not a valid -MIME type, or not a MIME type that Bugzilla accepts for attachments. - -=item 603 (File Name Not Specified) - -You did not specify a valid for the C argument. - -=item 604 (Summary Required) - -You did not specify a value for the C argument. - -=item 606 (Empty Data) - -You set the "data" field to an empty string. - -=back - -=item B - -=over - -=item Added in Bugzilla B<4.0>. - -=item The C parameter was removed in Bugzilla B<4.2>. - -=back - -=item REST API call added in Bugzilla B<5.0>. - -=back - - -=head2 update_attachment - -B - -=over - -=item B - -This allows you to update attachment metadata in Bugzilla. - -=item B - -To update attachment metadata on a current attachment: - -PUT /bug/attachment/ - -The params to include in the POST body, as well as the returned -data format are the same as below. The C param will be -overridden as it it pulled from the URL path. - -=item B - -=over - -=item C - -B C An array of integers -- the ids of the attachments you -want to update. - -=item C - -C The "file name" that will be displayed -in the UI for this attachment. - -=item C - -C A short string describing the -attachment. - -=item C - -C An optional comment to add to the attachment's bug. - -=item C - -C The MIME type of the attachment, like -C or C. - -=item C - -C True if Bugzilla should treat this attachment as a patch. -If you specify this, you do not need to specify a C. -The C of the attachment will be forced to C. - -=item C - -C True if the attachment should be private (restricted -to the "insidergroup"), False if the attachment should be public. - -=item C - -C True if the attachment is obsolete, False otherwise. - -=item C - -C An array of hashes with changes to the flags. The following values -can be specified. At least the status and one of type_id, id, or name must -be specified. If a type_id or name matches a single currently set flag, -the flag will be updated unless new is specified. - -=over - -=item C - -C The name of the flag that will be created or updated. - -=item C - -C The internal flag type id that will be created or updated. You will -need to specify the C if more than one flag type of the same name exists. - -=item C - -C The flags new status (i.e. "?", "+", "-" or "X" to clear a flag). - -=item C - -C The login of the requestee if the flag type is requestable to a specific user. - -=item C - -C Use id to specify the flag to be updated. You will need to specify the C -if more than one flag is set of the same name. - -=item C - -C Set to true if you specifically want a new flag to be created. - -=back - -=item C - -C An optional array of hashes with changes to the flags of the attachment's -bug. See the C param for the L method for the object format. - -=item B - -A C with a single field, "attachment". This points to an array of hashes -with the following fields: - -=over - -=item C - -C The id of the attachment that was updated. - -=item C - -C The exact time that this update was done at, for this attachment. -If no update was done (that is, no fields had their values changed and -no comment was added) then this will instead be the last time the attachment -was updated. - -=item C - -C The changes that were actually done on this bug. The keys are -the names of the fields that were changed, and the values are a hash -with two keys: - -=over - -=item C (C) The values that were added to this field. -possibly a comma-and-space-separated list if multiple values were added. - -=item C (C) The values that were removed from this -field. - -=back - -=back - -Here's an example of what a return value might look like: - - { - attachments => [ - { - id => 123, - last_change_time => '2010-01-01T12:34:56', - changes => { - summary => { - removed => 'Sample ptach', - added => 'Sample patch' - }, - is_obsolete => { - removed => '0', - added => '1', - } - }, - } - ] - } - -=item B - -This method can throw all the same errors as L, plus: - -=over - -=item 129 (Flag Status Invalid) - -The flag status is invalid. - -=item 130 (Flag Modification Denied) - -You tried to request, grant, or deny a flag but only a user with the required -permissions may make the change. - -=item 131 (Flag not Requestable from Specific Person) - -You can't ask a specific person for the flag. - -=item 132 (Flag not Unique) - -The flag specified has been set multiple times. You must specify the id -value to update the flag. - -=item 133 (Flag Type not Unique) - -The flag type specified matches several flag types. You must specify -the type id value to update or add a flag. - -=item 134 (Inactive Flag Type) - -The flag type is inactive and cannot be used to create new flags. - -=item 601 (Invalid MIME Type) - -You specified a C argument that was blank, not a valid -MIME type, or not a MIME type that Bugzilla accepts for attachments. - -=item 603 (File Name Not Specified) - -You did not specify a valid for the C argument. - -=item 604 (Summary Required) - -You did not specify a value for the C argument. - -=back - -=item B - -=over - -=item Added in Bugzilla B<5.0>. - -=back - -=back - -=back - =head2 update B diff --git a/Bugzilla/WebService/Server/REST/Resources/Bug.pm b/Bugzilla/WebService/Server/REST/Resources/Bug.pm index 372c62a095..69747d34c0 100644 --- a/Bugzilla/WebService/Server/REST/Resources/Bug.pm +++ b/Bugzilla/WebService/Server/REST/Resources/Bug.pm @@ -53,37 +53,6 @@ sub _rest_resources { }, } }, - qr{^/bug/([^/]+)/attachment$}, - { - GET => { - method => 'attachments', - params => sub { - return {ids => [$_[0]]}; - } - }, - POST => { - method => 'add_attachment', - params => sub { - return {ids => [$_[0]]}; - }, - success_code => STATUS_CREATED - } - }, - qr{^/bug/attachment/([^/]+)$}, - { - GET => { - method => 'attachments', - params => sub { - return {attachment_ids => [$_[0]]}; - } - }, - PUT => { - method => 'update_attachment', - params => sub { - return {ids => [$_[0]]}; - } - } - } ]; return $rest_resources; } diff --git a/qa/t/rest_bug_update_attachment.t b/qa/t/rest_bug_update_attachment.t new file mode 100644 index 0000000000..3789aa78c1 --- /dev/null +++ b/qa/t/rest_bug_update_attachment.t @@ -0,0 +1,113 @@ +#!/usr/bin/env perl +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this +# file, You can obtain one at http://mozilla.org/MPL/2.0/. +# +# This Source Code Form is "Incompatible With Secondary Licenses", as +# defined by the Mozilla Public License, v. 2.0. + +##################################################### +# Test for REST call to Bug.update_attachment() # +# PUT /rest/bug/attachment/ # +##################################################### + +use 5.10.1; +use strict; +use warnings; +use lib qw(lib ../../lib ../../local/lib/perl5); + +use Bugzilla; +use MIME::Base64 qw(encode_base64); +use QA::Util qw(get_config); +use QA::REST::Util qw(api_headers); + +use Test::Mojo; +use Test::More; + +my $config = get_config(); +my $url = Bugzilla->localconfig->urlbase; +my $admin_api_key = $config->{admin_user_api_key}; +my $headers = api_headers($admin_api_key); + +my $t = Test::Mojo->new(); +$t->ua->max_redirects(1); + +sub new_attachment { + my ($summary) = @_; + $t->post_ok( + $url + . 'rest/bug/public_bug/attachment' => $headers => json => { + summary => $summary, + file_name => 'update.txt', + content_type => 'text/plain', + data => encode_base64('update test', ''), + } + )->status_is(201); + my ($id) = keys %{$t->tx->res->json->{attachments}}; + return $id; +} + +my $first = new_attachment('first attachment'); +my $second = new_attachment('second attachment'); + +# The id in the path is used when the body has no ids. +$t->put_ok($url + . "rest/bug/attachment/$first" => $headers => json => + {summary => 'first renamed', is_obsolete => 1, comment => 'obsoleted'}) + ->status_is(200) + ->json_is('/attachments/0/id' => $first) + ->json_is('/attachments/0/changes/summary/removed' => 'first attachment') + ->json_is('/attachments/0/changes/summary/added' => 'first renamed') + ->json_is('/attachments/0/changes/is_obsolete/added' => '1') + ->json_like('/attachments/0/last_change_time' => qr/^\d{4}-\d\d-\d\dT.*Z$/); + +# ids in the body win over the id in the path, as on the legacy REST server. +$t->put_ok($url + . "rest/bug/attachment/$first" => $headers => json => + {ids => [$second], summary => 'second renamed'}) + ->status_is(200) + ->json_is('/attachments/0/id' => $second); +is(scalar @{$t->tx->res->json->{attachments}}, 1, + 'Only the body id is updated'); + +# Credentials in the query string are not taken for attachment fields. +$t->put_ok($url + . "rest/bug/attachment/$second?api_key=$admin_api_key" => {} => json => + {summary => 'renamed with api_key'}) + ->status_is(200) + ->json_is('/attachments/0/changes/summary/added' => 'renamed with api_key'); + +# Both attachments can be fetched together: the path id is added to the query. +$t->get_ok( + $url . "rest/bug/attachment/$first?attachment_ids=$second" => $headers) + ->status_is(200) + ->json_is("/attachments/$first/summary" => 'first renamed') + ->json_is("/attachments/$first/is_obsolete" => 1) + ->json_is("/attachments/$second/summary" => 'renamed with api_key'); + +# Errors +$t->put_ok($url + . "rest/bug/attachment/$first" => api_headers(undef) => json => + {summary => 'logged out'}) + ->status_isnt(200) + ->json_like('/message' => qr/You must log in/); + +$t->put_ok( + $url . "rest/bug/attachment/$first" => $headers => json => {bogus => 1}) + ->status_isnt(200) + ->json_like('/message' => qr/set_bogus/); + +$t->put_ok($url + . 'rest/bug/attachment/999999999' => $headers => json => + {summary => 'no such attachment'}) + ->status_isnt(200) + ->json_like('/message' => qr/999999999/); + +$t->options_ok($url . "rest/bug/attachment/$first") + ->status_is(200) + ->header_is('Allow' => 'GET, PUT'); +$t->options_ok($url . 'rest/bug/public_bug/attachment') + ->status_is(200) + ->header_is('Allow' => 'GET, POST'); + +done_testing(); diff --git a/t/app-cgi-request-limit.t b/t/app-cgi-request-limit.t index 0c55f864c3..ed95346b43 100644 --- a/t/app-cgi-request-limit.t +++ b/t/app-cgi-request-limit.t @@ -159,7 +159,7 @@ $t->post_ok( ->content_like(qr{The request is too large\.}); $t->post_ok( - '/rest/bug/1/attachment' => { + '/rest/bug' => { 'Content-Length' => length($body), 'Content-Type' => "multipart/form-data; boundary=$boundary", } => $body From 622b1543c54d64fe5490309d87bc0f2a1ced88e3 Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Thu, 22 Oct 2026 12:22:39 -0400 Subject: [PATCH 4/5] Bug 2080901 - [BMO-ETL] When any table fails to export, the entire snapshot should be rolled back to avoid incorrect results in charts/dashboards --- extensions/BMO/Extension.pm | 24 ++ extensions/BMO/bin/export_bmo_etl.pl | 560 +++++++++++++++++++++------ extensions/BMO/t/bmo/bmo_etl.t | 95 ++++- 3 files changed, 563 insertions(+), 116 deletions(-) diff --git a/extensions/BMO/Extension.pm b/extensions/BMO/Extension.pm index 5fc433392f..5bf425a057 100644 --- a/extensions/BMO/Extension.pm +++ b/extensions/BMO/Extension.pm @@ -1385,11 +1385,19 @@ sub db_schema_abstract_schema { bmo_etl_cache_uniq_idx => {FIELDS => ['id', 'table_name'], TYPE => 'UNIQUE'} ], }; + # value holds the id of the export run that owns the lock. updated_ts is a + # heartbeat; a lock that stops being updated was left by a run that died. + # name is always 'export'; its unique index allows only one lock row, so + # two runs starting at once cannot both take the lock. $args->{schema}->{bmo_etl_locked} = { FIELDS => [ + name => {TYPE => 'VARCHAR(20)', NOTNULL => 1, DEFAULT => "'export'"}, value => {TYPE => 'VARCHAR(20)', NOTNULL => 1,}, creation_ts => {TYPE => 'DATETIME',}, + updated_ts => {TYPE => 'DATETIME',}, ], + INDEXES => + [bmo_etl_locked_name_idx => {FIELDS => ['name'], TYPE => 'UNIQUE'}], }; # Recorded Future @@ -1594,6 +1602,22 @@ sub install_update_db { 'creation_ts' => {TYPE => 'DATETIME'}); } + # Add bmo_etl_locked.updated_ts column + if (!$dbh->bz_column_info('bmo_etl_locked', 'updated_ts')) { + $dbh->bz_add_column('bmo_etl_locked', 'updated_ts' => {TYPE => 'DATETIME'}); + } + + # Add bmo_etl_locked.name column with a unique index so only one lock can + # exist. Old lock rows are cleared first so the index can be created; a run + # in progress notices its lock is gone and aborts without publishing. + if (!$dbh->bz_column_info('bmo_etl_locked', 'name')) { + $dbh->do('DELETE FROM bmo_etl_locked'); + $dbh->bz_add_column('bmo_etl_locked', + 'name' => {TYPE => 'VARCHAR(20)', NOTNULL => 1, DEFAULT => "'export'"}); + } + $dbh->bz_add_index('bmo_etl_locked', 'bmo_etl_locked_name_idx', + {FIELDS => ['name'], TYPE => 'UNIQUE'}); + # Add unique index for id and table name for bmo_etl_cache $dbh->bz_add_index('bmo_etl_cache', 'bmo_etl_cache_uniq_idx', {FIELDS => ['id', 'table_name'], TYPE => 'UNIQUE'}); diff --git a/extensions/BMO/bin/export_bmo_etl.pl b/extensions/BMO/bin/export_bmo_etl.pl index 79de938741..385e57c59e 100644 --- a/extensions/BMO/bin/export_bmo_etl.pl +++ b/extensions/BMO/bin/export_bmo_etl.pl @@ -39,6 +39,34 @@ # Products which we should not send data to ETL such as Legal, etc. use constant EXCLUDE_PRODUCTS => ('Legal',); +# Every BigQuery table this script writes to. Each run streams rows into a +# staging copy of these tables and then publishes them all in one transaction. +use constant BQ_TABLES => qw( + bugs attachments flags tracking_flags keywords see_also users + bug_mentors bug_dependencies bug_regressions bug_duplicates +); + +# Prefix for staging tables, and how long they live if a run is killed +# before it can drop them. +use constant STAGING_PREFIX => 'etl_staging_'; +use constant STAGING_LIFETIME => 2 * 24 * 60 * 60; # two days + +# Extra staging column holding a per-row key, used to drop rows that were +# sent twice when an insert request was retried +use constant STAGING_ROW_KEY => q(_etl_row); + +# How long to wait for a BigQuery query (such as the publish transaction) +use constant QUERY_MAX_WAIT => 60 * 60; # one hour + +# A running export refreshes its lock at least this often. A lock not +# refreshed within LOCK_STALE_AFTER was left by a run that died, so a new run +# (such as a Kubernetes retry) takes it over. By default a new run waits up to +# LOCK_MAX_WAIT for an existing lock to be released or go stale. +use constant LOCK_REFRESH_EVERY => 60; # one minute +use constant LOCK_STALE_AFTER => 10 * 60; # ten minutes +use constant LOCK_MAX_WAIT => 15 * 60; # fifteen minutes +use constant LOCK_POLL_INTERVAL => 30; + # Log levels use constant DEBUG_OUTPUT => 1; @@ -48,8 +76,14 @@ 'q|quiet' => \my $quiet, 'd|debug' => \my $debug, 's|snapshot-date=s' => \my $snapshot_date, + 'w|lock-wait=i' => \my $lock_wait, 'n|no-cache=s' => \my @no_cache; +$lock_wait //= LOCK_MAX_WAIT; + +# Identifies this run in the lock table and in staging table names +my $run_id = time() . "_$$"; + # Create a hash for faster checking later my %no_cache = map { $_ => 1 } @no_cache; @@ -65,6 +99,12 @@ my $dataset_id = Bugzilla->params->{bmo_etl_dataset_id}; $dataset_id || die "Invalid BigQuery dataset ID.\n"; +# The date is used in BigQuery SQL statements, so make sure it is sane. +# Checked before taking the lock so a bad value does not leave it behind. +if ($snapshot_date && $snapshot_date !~ /^\d{4}-\d{2}-\d{2}$/) { + die "Invalid snapshot date '$snapshot_date', expected YYYY-MM-DD.\n"; +} + # Check to make sure another instance is not currently running check_and_set_lock(); @@ -94,42 +134,91 @@ # Bugs that are private to one or more groups our %private_bugs = (); -# In order to avoid entering duplicate data, we will first query BigQuery -# to make sure other entries with this date are not already present. -check_for_duplicates(); - -# Process each table to be sent to ETL -process_bugs(); -process_attachments(); -process_flags(); -process_flag_activity(); -process_tracking_flags(); -process_keywords(); -process_see_also(); -process_users(); - -process_two_columns( - 'bug_mentors', 'bug_mentors', - ['bug_id', 'user_id'], - ['bug_id', 'user_id'] -); -process_two_columns( - 'dependencies', 'bug_dependencies', - ['blocked', 'dependson'], - ['bug_id', 'depends_on_id'] -); -process_two_columns( - 'regressions', 'bug_regressions', - ['regresses', 'regressed_by'], - ['bug_id', 'regresses_id'] -); -process_two_columns( - 'duplicates', 'bug_duplicates', - ['dupe', 'dupe_of'], - ['bug_id', 'duplicate_of_id'] -); +# Real table name => staging table name, column names, and rows sent +our %staging_tables = (); +our %table_columns = (); +our %sent_counts = (); + +# Set by the signal handlers below. Checked before each batch is sent, since +# the die from a signal can be swallowed by an inner try block. +our $abort_reason; + +# Kubernetes sends SIGTERM before killing the pod. If the process is killed +# outright (SIGKILL), the staging tables expire on their own and the real +# tables are untouched. +$SIG{TERM} = sub { + $abort_reason = "Received SIGTERM, aborting export\n"; + die $abort_reason; +}; +$SIG{INT} = sub { + $abort_reason = "Received SIGINT, aborting export\n"; + die $abort_reason; +}; + +# Rows are streamed into staging tables and only copied into the real tables +# by publish_snapshot(), in a single transaction. If anything fails before +# then, the real tables never see any data for this snapshot date. +my $already_exported; +try { + # Staging tables left by a run that died are no longer needed + drop_orphaned_staging_tables(); + + # In order to avoid entering duplicate data, we will first query BigQuery + # to make sure other entries with this date are not already present. This + # is not an error, since a retry after a run that published and then died + # should simply finish. + if (snapshot_exists()) { + $already_exported = 1; + return; + } + + create_staging_tables(); + + # Process each table to be sent to ETL + process_bugs(); + process_attachments(); + process_flags(); + process_flag_activity(); + process_tracking_flags(); + process_keywords(); + process_see_also(); + process_users(); + + process_two_columns( + 'bug_mentors', 'bug_mentors', + ['bug_id', 'user_id'], + ['bug_id', 'user_id'] + ); + process_two_columns( + 'dependencies', 'bug_dependencies', + ['blocked', 'dependson'], + ['bug_id', 'depends_on_id'] + ); + process_two_columns( + 'regressions', 'bug_regressions', + ['regresses', 'regressed_by'], + ['bug_id', 'regresses_id'] + ); + process_two_columns( + 'duplicates', 'bug_duplicates', + ['dupe', 'dupe_of'], + ['bug_id', 'duplicate_of_id'] + ); -# If we are done, remove the lock + publish_snapshot(); +} +catch { + drop_staging_tables(); + delete_lock(); + die $_; +}; + +if ($already_exported) { + logger("Snapshot date $snapshot_date was already exported. Nothing to do."); +} + +# If we are done, remove the staging tables and the lock +drop_staging_tables(); delete_lock(); ### Functions @@ -794,7 +883,6 @@ sub get_cache { # First uncompress the JSON and then decode it back to Perl data my $data; unless (gunzip \$gzipped_data => \$data) { - delete_lock(); die "gunzip failed: $GunzipError\n"; } return decode_json($data); @@ -822,7 +910,6 @@ sub store_cache { # Compress the JSON to save space in the DB my $gzipped_data; unless (gzip \$data => \$gzipped_data) { - delete_lock(); die "gzip failed: $GzipError\n"; } @@ -857,14 +944,9 @@ sub send_data { $row->{snapshot_date} = $snapshot_date; } - my @json_rows = (); - foreach my $row (@{$all_rows}) { - push @json_rows, {json => $row}; - } - - my $big_query = {rows => \@json_rows}; - if ($test) { + my $big_query = {rows => [map { +{json => $_} } @{$all_rows}]}; + my $filename = bz_locations()->{'datadir'} . '/' . $snapshot_date . '-' @@ -876,50 +958,50 @@ sub send_data { my $fh = path($filename)->open('>>'); print $fh encode_json($big_query) . "\n"; unless (close $fh) { - delete_lock(); die "Could not close $filename: $!\n"; } return; } - my $http_headers = HTTP::Headers->new; - - # Do not attempt to get access token if running in test environment - if ($base_url !~ /^http:\/\/[^\/]+:9050/) { - my $access_token = _get_access_token(); - $http_headers->header(Authorization => 'Bearer ' . $access_token); - } - - my $full_path = sprintf 'projects/%s/datasets/%s/tables/%s/insertAll', - $project_id, $dataset_id, $table; - - logger("Sending to $base_url/$full_path", DEBUG_OUTPUT); + # Stop sending if a signal was received, even if an inner try block + # swallowed the die from the signal handler. + die $abort_reason if $abort_reason; - my $request = HTTP::Request->new('POST', "$base_url/$full_path", $http_headers); - $request->header('Content-Type' => 'application/json'); + my $staging_table = $staging_tables{$table} + || die "No staging table for $table\n"; - logger('Encoding content into JSON.', DEBUG_OUTPUT); - - $request->content(encode_json($big_query)); - - logger('Sending request', DEBUG_OUTPUT); - - my $response = $ua->request($request); + # Give each row a key that is unique within this run. The HTTP client + # resends the whole batch if a request fails after BigQuery accepted it, so + # the key lets publish_snapshot() drop those duplicates without merging + # rows that are legitimately identical (cleared flags have no id). It is + # also sent as insertId so BigQuery drops most duplicates on its own. + my $row_num = $sent_counts{$table}; + my @keyed_rows; + foreach my $row (@{$all_rows}) { + my $key = "$table-" . $row_num++; + push @keyed_rows, {insertId => $key, json => {%{$row}, STAGING_ROW_KEY, $key}}; + } - logger($response->content, DEBUG_OUTPUT); + my $content = encode_json({rows => \@keyed_rows}); + my $response + = bq_request('POST', "datasets/$dataset_id/tables/$staging_table/insertAll", + $content); my $result = decode_json($response->content); if (!$response->is_success || (exists $result->{insertErrors} && @{$result->{insertErrors}})) { - delete_lock(); die "Google Big Query insert failure:\nRequest:\n" - . $request->content + . $content . "\n\nResponse:\n" . $response->content . "\n"; } + + $sent_counts{$table} += scalar @{$all_rows}; + + refresh_lock(); } sub _get_access_token { @@ -951,7 +1033,6 @@ sub _get_access_token { my $res = $ua->request($request); if (!$res->is_success) { - delete_lock(); die 'Google access token failure: ' . $res->content . "\n"; } @@ -964,84 +1045,335 @@ sub _get_access_token { return $access_token; } -# If a previous process is performing an export to BigQuery, then -# we must check the lock table and exit if true. +# Take the export lock. If another run holds it, wait for that run to +# finish. A lock that stops being refreshed belongs to a run that died +# (for example a pod that was killed), so it is removed and taken over. sub check_and_set_lock { return if $test; # No need if just dumping test files logger('Checking for previous lock or setting new one', DEBUG_OUTPUT); my $dbh_main = Bugzilla->dbh_main; + my $waited = 0; + + while (1) { + my $stale + = $dbh_main->do( + 'DELETE FROM bmo_etl_locked WHERE COALESCE(updated_ts, creation_ts) < ' + . $dbh_main->sql_date_math('NOW()', '-', LOCK_STALE_AFTER / 60, 'MINUTE')); + if ($stale > 0) { + logger('Removed stale lock left by a previous run that did not finish.'); + } + + # The unique index on name allows only one lock row, so if two runs get + # here at the same time only one insert succeeds. + my $inserted = $dbh_main->do( + 'INSERT IGNORE INTO bmo_etl_locked (value, creation_ts, updated_ts) + VALUES (?, NOW(), NOW())', undef, $run_id + ); + if ($inserted > 0) { + logger("Set new lock for run $run_id.", DEBUG_OUTPUT); + return; + } - # Clear out any locks that are greater than 24h old - $dbh_main->do('DELETE FROM bmo_etl_locked WHERE creation_ts < ' - . $dbh_main->sql_date_math('NOW()', '-', 24, 'HOUR')); + # The lock may have been released since the insert. Waiting as usual + # keeps this loop bounded if the insert keeps failing for another reason. + my $owner = $dbh_main->selectrow_array('SELECT value FROM bmo_etl_locked') + // 'unknown'; + + if ($waited >= $lock_wait) { + die "Another process ($owner) has set a lock. Exiting\n"; + } - # Now check for any pre-existing locks and do not proceed if one found - my $locked = $dbh_main->selectrow_array('SELECT COUNT(*) FROM bmo_etl_locked'); - if ($locked) { - die "Another process has set a lock. Exiting\n"; + logger("Export run $owner holds the lock. Waiting."); + sleep LOCK_POLL_INTERVAL; + $waited += LOCK_POLL_INTERVAL; } +} - logger('Previous lock not found. Setting new one.', DEBUG_OUTPUT); +# Refresh the lock so other runs know this one is still alive. Dies if the +# lock was taken over, which means this run stalled for too long and must +# not publish. Pass a true value to skip the rate limit. +sub refresh_lock { + my ($force) = @_; + return if $test; + + state $last_refresh = 0; + return if !$force && time() - $last_refresh < LOCK_REFRESH_EVERY; + + my $updated + = Bugzilla->dbh_main->do( + 'UPDATE bmo_etl_locked SET updated_ts = NOW() WHERE value = ?', + undef, $run_id); + if ($updated < 1) { + die "Lost the export lock to another run. Aborting\n"; + } - $dbh_main->do( - 'INSERT INTO bmo_etl_locked (value, creation_ts) VALUES (?, NOW())', - undef, 'locked'); + $last_refresh = time(); } -# Delete lock from bmo_etl_locked +# Delete this run's lock from bmo_etl_locked sub delete_lock { + return if $test; logger("Deleting lock in database."); - Bugzilla->dbh_main->do('DELETE FROM bmo_etl_locked'); + Bugzilla->dbh_main->do('DELETE FROM bmo_etl_locked WHERE value = ?', + undef, $run_id); } -sub check_for_duplicates { - return if $test; # no need if just dumping test files +# Returns true if BigQuery already has data for this snapshot date +sub snapshot_exists { + return 0 if $test; # no need if just dumping test files - logger("Checking for duplicate data for snapshot date $snapshot_date."); + logger("Checking for existing data for snapshot date $snapshot_date."); - my $http_headers = HTTP::Headers->new; + # Snapshots are published to all tables at once, so checking bugs is enough + my $result + = run_bq_query( + "SELECT count(*) FROM `${project_id}.${dataset_id}.bugs` WHERE snapshot_date = '$snapshot_date';" + ); - # Do not attempt to get access token if running in test environment - if ($base_url !~ /^http:\/\/[^\/]+:9050/) { - my $access_token = _get_access_token(); - $http_headers->header(Authorization => 'Bearer ' . $access_token); + return $result->{rows}->[0]->{f}->[0]->{v} ? 1 : 0; +} + +# Drop staging tables left behind by earlier runs that died. This run holds +# the lock, so no other run is using them. +sub drop_orphaned_staging_tables { + return if $test; + + my @orphans; + my $page_token = ''; + do { + my $path = "datasets/$dataset_id/tables?maxResults=1000"; + $path .= "&pageToken=$page_token" if $page_token; + + my $response = bq_request('GET', $path); + if (!$response->is_success) { + die "Unable to list tables in $dataset_id: " . $response->content . "\n"; + } + + my $result = decode_json($response->content); + push @orphans, + grep { index($_, STAGING_PREFIX) == 0 } + map { $_->{tableReference}->{tableId} } @{$result->{tables} || []}; + $page_token = $result->{nextPageToken}; + } while ($page_token); + + foreach my $staging_table (@orphans) { + logger("Dropping orphaned staging table $staging_table."); + my $response + = bq_request('DELETE', "datasets/$dataset_id/tables/$staging_table"); + if (!$response->is_success && $response->code != 404) { + die "Unable to drop staging table $staging_table: " . $response->content . "\n"; + } + } +} + +# Create an empty staging table for each real table, with the same schema. +# The run id keeps names unique, since streaming into a table that was just +# deleted and recreated with the same name can lose rows. +sub create_staging_tables { + return if $test; + + my $expiration = (time() + STAGING_LIFETIME) * 1000; + + foreach my $table (BQ_TABLES) { + my $response = bq_request('GET', "datasets/$dataset_id/tables/$table"); + if (!$response->is_success) { + die "Unable to read schema for $table: " . $response->content . "\n"; + } + my $schema = decode_json($response->content)->{schema}; + + my $staging_table = STAGING_PREFIX . "${table}_$run_id"; + logger("Creating staging table $staging_table."); + + # Staging has one extra column holding the row key set in send_data() + my $staging_schema = {fields => + [@{$schema->{fields}}, {name => STAGING_ROW_KEY, type => 'STRING'}]}; + + $response = bq_request( + 'POST', + "datasets/$dataset_id/tables", + encode_json({ + tableReference => { + projectId => $project_id, + datasetId => $dataset_id, + tableId => $staging_table, + }, + schema => $staging_schema, + expirationTime => "$expiration", + }) + ); + if (!$response->is_success) { + die "Unable to create staging table $staging_table: " + . $response->content . "\n"; + } + + $staging_tables{$table} = $staging_table; + $table_columns{$table} = [map { $_->{name} } @{$schema->{fields}}]; + $sent_counts{$table} = 0; + } +} + +# Copy every staging table into its real table in one transaction, so either +# the whole snapshot appears or none of it does. +sub publish_snapshot { + return if $test; + + # Make sure every row that was sent is visible in staging before copying. + # Streamed rows are normally queryable within a few seconds. Count row keys + # rather than rows, since a retried insert can leave duplicates. + foreach my $table (BQ_TABLES) { + my $staging_table = $staging_tables{$table}; + my $count; + foreach my $attempt (1 .. 6) { + my $result = run_bq_query( + "SELECT COUNT(DISTINCT " . STAGING_ROW_KEY . ") " + . "FROM `${project_id}.${dataset_id}.${staging_table}`;"); + $count = $result->{rows}->[0]->{f}->[0]->{v} // 0; + last if $count == $sent_counts{$table}; + logger("$staging_table has $count of $sent_counts{$table} rows, waiting."); + sleep 10; + } + if ($count != $sent_counts{$table}) { + die "Staging table $staging_table has $count rows " + . "but $sent_counts{$table} were sent\n"; + } } - my $full_path = "projects/$project_id/queries"; + logger("Publishing snapshot date $snapshot_date."); + + # snapshot_exists() ran when this run started, but a transaction submitted + # by an earlier run can keep going in BigQuery after that run died and + # commit later. So every insert also checks for the snapshot, and fails the + # whole transaction if it is already there. bugs goes last, since the + # check looks at bugs and must not see the rows this transaction adds. + my $already_published + = "IF(EXISTS(SELECT 1 FROM `${project_id}.${dataset_id}.bugs` " + . "WHERE snapshot_date = '$snapshot_date'), " + . "ERROR('Snapshot date $snapshot_date was already exported'), TRUE)"; + + # Copy one row per row key, so rows sent twice by a retried insert are + # only published once + my $row_key = STAGING_ROW_KEY; + my @statements; + foreach my $table ((grep { $_ ne 'bugs' } BQ_TABLES), 'bugs') { + my $columns = join ', ', map {"`$_`"} @{$table_columns{$table}}; + push @statements, + "INSERT INTO `${project_id}.${dataset_id}.${table}` ($columns) " + . "SELECT $columns FROM (SELECT *, ROW_NUMBER() OVER " + . "(PARTITION BY `$row_key`) AS _etl_copy " + . "FROM `${project_id}.${dataset_id}.$staging_tables{$table}`) " + . "WHERE _etl_copy = 1 AND $already_published;"; + } - logger("Querying $base_url/$full_path", DEBUG_OUTPUT); + # Make sure no other run has taken over before making the data visible + refresh_lock(1); - my $query = { - query => - "SELECT count(*) FROM ${project_id}.${dataset_id}.bugs WHERE snapshot_date = '$snapshot_date';", - useLegacySql => false, + try { + run_bq_query(join "\n", 'BEGIN TRANSACTION;', + @statements, 'COMMIT TRANSACTION;'); + logger("Snapshot date $snapshot_date published."); + } + catch { + my $error = $_; + + # The snapshot is there if the guard above fired, or if this run's + # transaction committed but waiting for the result failed. Either way + # it was published exactly once, so the run has nothing left to do. + my $exists = try { snapshot_exists() } catch {0}; + die $error if !$exists; + + logger("Snapshot date $snapshot_date is already in BigQuery. Nothing to do."); }; +} - my $request = HTTP::Request->new('POST', "$base_url/$full_path", $http_headers); - $request->header('Content-Type' => 'application/json'); - $request->content(encode_json($query)); +# Drop any staging tables created by this run. Failures are only logged, +# since the tables expire on their own. +sub drop_staging_tables { + foreach my $table (keys %staging_tables) { + my $staging_table = delete $staging_tables{$table}; + logger("Dropping staging table $staging_table."); + try { + my $response + = bq_request('DELETE', "datasets/$dataset_id/tables/$staging_table"); + if (!$response->is_success && $response->code != 404) { + WARN( + "ERROR: Unable to drop staging table $staging_table: " . $response->content); + } + } + catch { + WARN("ERROR: Unable to drop staging table $staging_table: $_"); + }; + } +} - logger(encode_json($query), DEBUG_OUTPUT); +# Run a SQL statement using the BigQuery jobs.query API and return the +# decoded response, waiting for long running jobs to finish. +sub run_bq_query { + my ($sql) = @_; - my $res = $ua->request($request); - if (!$res->is_success) { - delete_lock(); - die 'Google Big Query query failure: ' . $res->content . "\n"; + logger($sql, DEBUG_OUTPUT); + + my $response = bq_request('POST', 'queries', + encode_json({query => $sql, useLegacySql => false, timeoutMs => 60000})); + if (!$response->is_success) { + die 'Google Big Query query failure: ' . $response->content . "\n"; } - logger($res->content, DEBUG_OUTPUT); + my $result = decode_json($response->content); + my $waited = 0; - my $result = decode_json($res->content); + # The job keeps running in BigQuery after the request times out, so poll + # for the result rather than giving up and leaving its outcome unknown. + while (!$result->{jobComplete}) { + if ($waited >= QUERY_MAX_WAIT) { + die "Google Big Query query did not complete in time: $sql\n"; + } - my $row_count = $result->{rows}->[0]->{f}->[0]->{v}; + my $job = $result->{jobReference}; + my $location = $job->{location} ? "&location=$job->{location}" : ''; + $response = bq_request('GET', + "queries/$job->{jobId}?timeoutMs=60000&maxResults=0$location"); + if (!$response->is_success) { + die 'Google Big Query query failure: ' . $response->content . "\n"; + } + + $result = decode_json($response->content); + $waited += 60; - # Do not export if we have any rows with this snapshot date. - if ($row_count) { - delete_lock(); - die "Duplicate data found for snapshot date $snapshot_date\n"; + refresh_lock(); } + + logger(encode_json($result), DEBUG_OUTPUT); + + return $result; +} + +# Send a request to the BigQuery API. The path is relative to the project. +sub bq_request { + my ($method, $path, $content) = @_; + + my $http_headers = HTTP::Headers->new; + + # Do not attempt to get access token if running in test environment + if ($base_url !~ /^http:\/\/[^\/]+:9050/) { + my $access_token = _get_access_token(); + $http_headers->header(Authorization => 'Bearer ' . $access_token); + } + + my $url = "$base_url/projects/$project_id/$path"; + logger("$method $url", DEBUG_OUTPUT); + + my $request = HTTP::Request->new($method, $url, $http_headers); + if (defined $content) { + $request->header('Content-Type' => 'application/json'); + $request->content($content); + } + + my $response = $ua->request($request); + logger($response->content, DEBUG_OUTPUT); + + return $response; } sub get_multi_group_value { diff --git a/extensions/BMO/t/bmo/bmo_etl.t b/extensions/BMO/t/bmo/bmo_etl.t index 0a7a1234f8..a40e6d977f 100644 --- a/extensions/BMO/t/bmo/bmo_etl.t +++ b/extensions/BMO/t/bmo/bmo_etl.t @@ -199,7 +199,7 @@ $t->post_ok( 'http://bq:9050/bigquery/v2/projects/test/queries' => json => $query) ->status_is(200)->json_is('/rows/0/f/0/v' => $bug_id_1); -### Section 7: Exporting again on the same day (with the same snapshot date) will cause the script to exit +### Section 7: Exporting again with the same snapshot date succeeds without sending data again @cmd = ( './extensions/BMO/bin/export_bmo_etl.pl', @@ -207,6 +207,97 @@ $t->post_ok( ); ($output, $error, $rv) = capture { system @cmd; }; -ok($rv, 'Duplicate data exported to BigQuery test instance should fail'); +ok(!$rv, 'Exporting an already exported snapshot date exits successfully'); + +### Section 8: Each row is published exactly once and staging tables are removed + +$query = { + query => 'SELECT count(*) FROM test.bugzilla.bugs WHERE id = ' + . $bug_id_1 + . ' AND snapshot_date = \'' + . $snapshot_date . '\';', + useLegacySql => false +}; +$t->post_ok( + 'http://bq:9050/bigquery/v2/projects/test/queries' => json => $query) + ->status_is(200) + ->json_is('/rows/0/f/0/v' => 1); + +$t->get_ok('http://bq:9050/bigquery/v2/projects/test/datasets/bugzilla/tables') + ->status_is(200); +my @staging = grep { $_->{tableReference}->{tableId} =~ /^etl_staging_/ } + @{$t->tx->res->json->{tables} || []}; +is(scalar @staging, 0, 'No staging tables left after export'); + +my $dbh = Bugzilla->dbh; +my $lock_count = $dbh->selectrow_array('SELECT COUNT(*) FROM bmo_etl_locked'); +is($lock_count, 0, 'Lock removed after export'); + +### Section 9: A live lock blocks the export, a stale lock is taken over + +$dbh->do( + 'INSERT INTO bmo_etl_locked (value, creation_ts, updated_ts) VALUES (?, NOW(), NOW())', + undef, 'live_run' +); + +@cmd = ( + './extensions/BMO/bin/export_bmo_etl.pl', + '--snapshot-date', '2000-01-01', '--lock-wait', 0, +); + +($output, $error, $rv) = capture { system @cmd; }; +ok($rv, 'Export should fail while another live export holds the lock'); + +# Only one lock row can exist, so two runs starting at once cannot both +# take the lock +my $second_lock = $dbh->do( + 'INSERT IGNORE INTO bmo_etl_locked (value, creation_ts, updated_ts) VALUES (?, NOW(), NOW())', + undef, 'second_run' +); +ok(!($second_lock > 0), 'A second lock cannot be set while one is held'); + +# Simulate a run that was killed: its lock stopped being refreshed an hour +# ago and it left a staging table behind +$dbh->do('DELETE FROM bmo_etl_locked'); +my $stale_ts = $dbh->selectrow_array( + 'SELECT ' . $dbh->sql_date_math('NOW()', '-', 1, 'HOUR')); +$dbh->do( + 'INSERT INTO bmo_etl_locked (value, creation_ts, updated_ts) VALUES (?, ?, ?)', + undef, 'dead_run', $stale_ts, $stale_ts +); + +$t->post_ok( + 'http://bq:9050/bigquery/v2/projects/test/datasets/bugzilla/tables' => json => + { + tableReference => + {projectId => 'test', datasetId => 'bugzilla', tableId => 'etl_staging_bugs_dead'}, + schema => {fields => [{name => 'id', type => 'INTEGER'}]}, + } +)->status_is(200); + +($output, $error, $rv) = capture { system @cmd; }; +ok(!$rv, 'Export takes over a stale lock and finishes'); + +$lock_count = $dbh->selectrow_array('SELECT COUNT(*) FROM bmo_etl_locked'); +is($lock_count, 0, 'Stale lock removed after export'); + +$t->get_ok('http://bq:9050/bigquery/v2/projects/test/datasets/bugzilla/tables') + ->status_is(200); +@staging = grep { $_->{tableReference}->{tableId} =~ /^etl_staging_/ } + @{$t->tx->res->json->{tables} || []}; +is(scalar @staging, 0, 'Orphaned staging table dropped'); + +### Section 10: An invalid snapshot date is rejected + +@cmd = ( + './extensions/BMO/bin/export_bmo_etl.pl', + '--snapshot-date', "2000-01-01' OR '1'='1", +); + +($output, $error, $rv) = capture { system @cmd; }; +ok($rv, 'Export should fail with an invalid snapshot date'); + +$lock_count = $dbh->selectrow_array('SELECT COUNT(*) FROM bmo_etl_locked'); +is($lock_count, 0, 'Invalid snapshot date does not leave a lock behind'); done_testing; From 3ad2e07e3702de27b48ff0338b9c7cc36e862dc3 Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Sun, 25 Oct 2026 14:47:22 -0400 Subject: [PATCH 5/5] Fixed test suite failure --- extensions/BMO/bin/export_bmo_etl.pl | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/extensions/BMO/bin/export_bmo_etl.pl b/extensions/BMO/bin/export_bmo_etl.pl index 385e57c59e..46ea0ac94e 100644 --- a/extensions/BMO/bin/export_bmo_etl.pl +++ b/extensions/BMO/bin/export_bmo_etl.pl @@ -21,6 +21,7 @@ use Bugzilla::User; use Bugzilla::FlagActivity; +use English qw(-no_match_vars $PID); use HTTP::Headers; use HTTP::Request; use IO::Compress::Gzip qw(gzip $GzipError); @@ -82,7 +83,7 @@ $lock_wait //= LOCK_MAX_WAIT; # Identifies this run in the lock table and in staging table names -my $run_id = time() . "_$$"; +my $run_id = time() . "_$PID"; # Create a hash for faster checking later my %no_cache = map { $_ => 1 } @no_cache;