Bug 1482145 - add more type checking to phabbugz code

8 files changed, 124 insertions, 38 deletions

diff --git a/extensions/PhabBugz/lib/Feed.pm b/extensions/PhabBugz/lib/Feed.pm
index 1cc73d134..4799bd0a3 100644
--- a/extensions/PhabBugz/lib/Feed.pm
+++ b/extensions/PhabBugz/lib/Feed.pm
@@ -16,6 +16,9 @@ use List::MoreUtils qw(any uniq);
 use Moo;
 use Scalar::Util qw(blessed);
 use Try::Tiny;
+use Type::Params qw( compile );
+use Type::Utils;
+use Types::Standard qw( :types );
 
 use Bugzilla::Constants;
 use Bugzilla::Error;
@@ -24,7 +27,8 @@ use Bugzilla::Logging;
 use Bugzilla::Mailer;
 use Bugzilla::Search;
 use Bugzilla::Util qw(diff_arrays format_time with_writable_database with_readonly_database);
-
+use Bugzilla::Types qw(:types);
+use Bugzilla::Extension::PhabBugz::Types qw(:types);
 use Bugzilla::Extension::PhabBugz::Constants;
 use Bugzilla::Extension::PhabBugz::Policy;
 use Bugzilla::Extension::PhabBugz::Revision;
@@ -39,6 +43,8 @@ use Bugzilla::Extension::PhabBugz::Util qw(
 
 has 'is_daemon' => ( is => 'rw', default => 0 );
 
+my $Invocant = class_type { class => __PACKAGE__ };
+
 sub start {
     my ($self) = @_;
 
@@ -369,7 +375,8 @@ sub process_revision_change {
             return;
         }
     }
-    
+
+
     my $log_message = sprintf(
         "REVISION CHANGE FOUND: D%d: %s | bug: %d | %s",
         $revision->id,
@@ -618,7 +625,8 @@ sub process_revision_change {
 }
 
 sub process_new_user {
-    my ( $self, $user_data ) = @_;
+    state $check = compile($Invocant, HashRef);
+    my ( $self, $user_data ) = $check->(@_);
 
     # Load the user data into a proper object
     my $phab_user = Bugzilla::Extension::PhabBugz::User->new($user_data);
@@ -819,8 +827,8 @@ sub save_last_id {
 }
 
 sub get_group_members {
-    my ( $self, $group ) = @_;
-
+    state $check = compile( $Invocant, Group | Str );
+    my ( $self, $group ) = $check->(@_);
     my $group_obj =
       ref $group ? $group : Bugzilla::Group->check( { name => $group, cache => 1 } );
 
@@ -844,7 +852,19 @@ sub get_group_members {
 }
 
 sub add_flag_comment {
-    my ( $self, $params ) = @_;
+    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)};
 
diff --git a/extensions/PhabBugz/lib/Policy.pm b/extensions/PhabBugz/lib/Policy.pm
index a86c83036..415ea20fb 100644
--- a/extensions/PhabBugz/lib/Policy.pm
+++ b/extensions/PhabBugz/lib/Policy.pm
@@ -13,11 +13,13 @@ use Moo;
 use Bugzilla::Error;
 use Bugzilla::Extension::PhabBugz::Util qw(request);
 use Bugzilla::Extension::PhabBugz::Project;
+use Bugzilla::Extension::PhabBugz::Types qw(:types);
 
 use List::Util qw(first);
 
 use Types::Standard -all;
 use Type::Utils;
+use Type::Params qw( compile );
 
 has 'phid'      => ( is => 'ro', isa => Str );
 has 'type'      => ( is => 'ro', isa => Str );
@@ -41,7 +43,7 @@ has 'rules' => (
 
 has 'rule_projects' => (
     is => 'lazy',
-    isa => ArrayRef[Object],
+    isa => ArrayRef[Project],
 );
 
 # {
@@ -79,8 +81,11 @@ has 'rule_projects' => (
 #   }
 # }
 
+my $Invocant = class_type { class => __PACKAGE__ };
+
 sub new_from_query {
-    my ($class, $params) = @_;
+    state $check = compile($Invocant | ClassName, Dict[phids => ArrayRef[Str]]);
+    my ($class, $params) = $check->(@_);
     my $result = request('policy.query', $params);
     if (exists $result->{result}{data} && @{ $result->{result}{data} }) {
         return $class->new($result->{result}->{data}->[0]);
@@ -88,7 +93,8 @@ sub new_from_query {
 }
 
 sub create {
-    my ($class, $projects) = @_;
+    state $check = compile($Invocant | ClassName, ArrayRef[Project]);
+    my ($class, $projects) = $check->(@_);
 
     my $data = {
         objectType => 'DREV',
diff --git a/extensions/PhabBugz/lib/Project.pm b/extensions/PhabBugz/lib/Project.pm
index b93a6eb9e..c18708887 100644
--- a/extensions/PhabBugz/lib/Project.pm
+++ b/extensions/PhabBugz/lib/Project.pm
@@ -12,10 +12,12 @@ use Moo;
 use Scalar::Util qw(blessed);
 use Types::Standard -all;
 use Type::Utils;
+use Type::Params qw( compile );
 
 use Bugzilla::Error;
 use Bugzilla::Util qw(trim);
 use Bugzilla::Extension::PhabBugz::User;
+use Bugzilla::Extension::PhabBugz::Types qw(:types);
 use Bugzilla::Extension::PhabBugz::Util qw(request);
 
 #########################
@@ -33,7 +35,9 @@ has view_policy     => ( is => 'ro', isa => Str );
 has edit_policy     => ( is => 'ro', isa => Str );
 has join_policy     => ( is => 'ro', isa => Str );
 has members_raw     => ( is => 'ro', isa => ArrayRef [ Dict [ phid => Str ] ] );
-has members => ( is => 'lazy', isa => ArrayRef [Object] );
+has members         => ( is => 'lazy', isa => ArrayRef[PhabUser] );
+
+my $Invocant = class_type { class => __PACKAGE__ };
 
 sub new_from_query {
     my ( $class, $params ) = @_;
@@ -142,12 +146,20 @@ sub BUILDARGS {
 #########################
 
 sub create {
-    my ( $class, $params ) = @_;
-
-    my $name = trim( $params->{name} );
-    $name || ThrowCodeError( 'param_required', { param => 'name' } );
+    state $check = compile(
+        $Invocant | ClassName,
+        Dict[
+            name => Str,
+            description => Str,
+            view_policy => Str,
+            edit_policy => Str,
+            join_policy => Str,
+        ]
+    );
+    my ( $class, $params ) = $check->(@_);
 
-    my $description = $params->{description} || 'Need description';
+    my $name        = trim($params->{name});
+    my $description = $params->{description};
     my $view_policy = $params->{view_policy};
     my $edit_policy = $params->{edit_policy};
     my $join_policy = $params->{join_policy};
@@ -324,5 +336,4 @@ sub _build_members {
     );
 }
 
-1;
-
+1;
\ No newline at end of file
diff --git a/extensions/PhabBugz/lib/Revision.pm b/extensions/PhabBugz/lib/Revision.pm
index d2df62e27..295713aaf 100644
--- a/extensions/PhabBugz/lib/Revision.pm
+++ b/extensions/PhabBugz/lib/Revision.pm
@@ -15,10 +15,12 @@ use Types::Standard -all;
 use Type::Utils;
 
 use Bugzilla::Bug;
+use Bugzilla::Types qw(JSONBool);
 use Bugzilla::Error;
 use Bugzilla::Util qw(trim);
 use Bugzilla::Extension::PhabBugz::Project;
 use Bugzilla::Extension::PhabBugz::User;
+use Bugzilla::Extension::PhabBugz::Types qw(:types);
 use Bugzilla::Extension::PhabBugz::Util qw(request);
 
 #########################
@@ -39,16 +41,16 @@ has edit_policy      => ( is => 'ro',   isa => Str );
 has subscriber_count => ( is => 'ro',   isa => Int );
 has bug              => ( is => 'lazy', isa => Object );
 has author           => ( is => 'lazy', isa => Object );
-has reviewers        => ( is => 'lazy', isa => ArrayRef [Object] );
-has subscribers      => ( is => 'lazy', isa => ArrayRef [Object] );
-has projects         => ( is => 'lazy', isa => ArrayRef [Object] );
+has reviewers        => ( is => 'lazy', isa => ArrayRef [PhabUser] );
+has subscribers      => ( is => 'lazy', isa => ArrayRef [PhabUser] );
+has projects         => ( is => 'lazy', isa => ArrayRef [Project] );
 has reviewers_raw => (
     is  => 'ro',
     isa => ArrayRef [
         Dict [
             reviewerPHID => Str,
             status       => Str,
-            isBlocking   => Bool,
+            isBlocking   => Bool | JSONBool,
             actorPHID    => Maybe [Str],
         ],
     ]
@@ -58,7 +60,7 @@ has subscribers_raw => (
     isa => Dict [
         subscriberPHIDs => ArrayRef [Str],
         subscriberCount => Int,
-        viewerIsSubscribed => Bool,
+        viewerIsSubscribed => Bool | JSONBool,
     ]
 );
 has projects_raw => (
diff --git a/extensions/PhabBugz/lib/Types.pm b/extensions/PhabBugz/lib/Types.pm
new file mode 100644
index 000000000..44987bfee
--- /dev/null
+++ b/extensions/PhabBugz/lib/Types.pm
@@ -0,0 +1,25 @@
+# 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::Extension::PhabBugz::Types;
+
+use 5.10.1;
+use strict;
+use warnings;
+
+use Type::Library
+    -base,
+    -declare => qw( Revision PhabUser Policy Project );
+use Type::Utils -all;
+use Types::Standard -types;
+
+class_type Revision, { class => 'Bugzilla::Extension::PhabBugz::Revision' };
+class_type Policy, { class => 'Bugzilla::Extension::PhabBugz::Policy' };
+class_type Project, { class => 'Bugzilla::Extension::PhabBugz::Project' };
+class_type PhabUser, { class => 'Bugzilla::Extension::PhabBugz::User' };
+
+1;
diff --git a/extensions/PhabBugz/lib/User.pm b/extensions/PhabBugz/lib/User.pm
index da573be37..209425bdf 100644
--- a/extensions/PhabBugz/lib/User.pm
+++ b/extensions/PhabBugz/lib/User.pm
@@ -11,12 +11,13 @@ use 5.10.1;
 use Moo;
 
 use Bugzilla::User;
-
+use Bugzilla::Types qw(:types);
 use Bugzilla::Extension::PhabBugz::Util qw(request);
 
 use List::Util qw(first);
 use Types::Standard -all;
 use Type::Utils;
+use Type::Params qw(compile);
 
 #########################
 #    Initialization     #
@@ -33,7 +34,9 @@ has 'roles'           => ( is => 'ro', isa => ArrayRef [Str] );
 has 'view_policy'     => ( is => 'ro', isa => Str );
 has 'edit_policy'     => ( is => 'ro', isa => Str );
 has 'bugzilla_id'     => ( is => 'ro', isa => Maybe [Int] );
-has 'bugzilla_user'   => ( is => 'lazy' );
+has 'bugzilla_user'   => ( is => 'lazy', isa => Maybe [User] );
+
+my $Invocant = class_type { class => __PACKAGE__ };
 
 sub BUILDARGS {
     my ( $class, $params ) = @_;
@@ -113,7 +116,8 @@ sub new_from_query {
 }
 
 sub match {
-    my ( $class, $params ) = @_;
+    state $check = compile( $Invocant | ClassName, Dict[ ids => ArrayRef[Int] ] | Dict[ phids => ArrayRef[Str] ] );
+    my ( $class, $params ) = $check->(@_);
 
     # BMO id search takes precedence if bugzilla_ids is used.
     my $bugzilla_ids = delete $params->{ids};
@@ -158,7 +162,8 @@ sub _build_bugzilla_user {
 }
 
 sub get_phab_bugzilla_ids {
-    my ( $class, $params ) = @_;
+    state $check = compile($Invocant | ClassName, Dict[ids => ArrayRef[Int]]);
+    my ( $class, $params ) = $check->(@_);
 
     my $memcache = Bugzilla->memcached;
 
diff --git a/extensions/PhabBugz/lib/Util.pm b/extensions/PhabBugz/lib/Util.pm
index 34a322240..4e846badc 100644
--- a/extensions/PhabBugz/lib/Util.pm
+++ b/extensions/PhabBugz/lib/Util.pm
@@ -15,14 +15,19 @@ use Bugzilla::Bug;
 use Bugzilla::Constants;
 use Bugzilla::Error;
 use Bugzilla::User;
+use Bugzilla::Types qw(:types);
 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 base qw(Exporter);
 
@@ -38,7 +43,8 @@ our @EXPORT = qw(
 );
 
 sub create_revision_attachment {
-    my ( $bug, $revision, $timestamp, $submitter ) = @_;
+    state $check = compile(Bug, Revision, Str, User);
+    my ( $bug, $revision, $timestamp, $submitter ) = $check->(@_);
 
     my $phab_base_uri = Bugzilla->params->{phabricator_base_uri};
     ThrowUserError('invalid_phabricator_uri') unless $phab_base_uri;
@@ -101,7 +107,8 @@ sub intersect {
 }
 
 sub get_bug_role_phids {
-    my ($bug) = @_;
+    state $check = compile(Bug);
+    my ($bug) = $check->(@_);
 
     my @bug_users = ( $bug->reporter );
     push(@bug_users, $bug->assigned_to)
@@ -120,12 +127,14 @@ sub get_bug_role_phids {
 }
 
 sub is_attachment_phab_revision {
-    my ($attachment) = @_;
+    state $check = compile(Attachment);
+    my ($attachment) = $check->(@_);
     return $attachment->contenttype eq PHAB_CONTENT_TYPE;
 }
 
 sub get_attachment_revisions {
-    my $bug = shift;
+    state $check = compile(Bug);
+    my ($bug) = $check->(@_);
 
     my @attachments =
       grep { is_attachment_phab_revision($_) } @{ $bug->attachments() };
@@ -154,7 +163,8 @@ sub get_attachment_revisions {
 }
 
 sub request {
-    my ($method, $data) = @_;
+    state $check = compile(Str, HashRef);
+    my ($method, $data) = $check->(@_);
     my $request_cache = Bugzilla->request_cache;
     my $params        = Bugzilla->params;
 
diff --git a/extensions/PhabBugz/t/basic.t b/extensions/PhabBugz/t/basic.t
index ba2f35e1d..9a6723ccb 100644
--- a/extensions/PhabBugz/t/basic.t
+++ b/extensions/PhabBugz/t/basic.t
@@ -223,15 +223,22 @@ JSON
             },
         ],
     );
-    my $bug = mock {
-        bug_id => 23,
+    my $Attachment = mock 'Bugzilla::Attachment' => (
+        add_constructor => [ fake_new => 'hash' ],
+    );
+    my $Bug = mock 'Bugzilla::Bug' => (
+        add_constructor => [ fake_new => 'hash' ],
+    );
+    my $bug = Bugzilla::Bug->fake_new(
+        bug_id      => 23,
         attachments => [
-            mock {
-                contenttype => 'text/x-phabricator-request',
+            Bugzilla::Attachment->fake_new(
+                mimetype => 'text/x-phabricator-request',
                 filename => 'phabricator-D9999-url.txt',
-            },
+            ),
         ]
-    };
+    );
+
     my $revisions = get_attachment_revisions($bug);
     is(ref($revisions), 'ARRAY', 'it is an array ref');
     isa_ok($revisions->[0], 'Bugzilla::Extension::PhabBugz::Revision');
@@ -240,4 +247,4 @@ JSON
 
 };
 
-done_testing;
\ No newline at end of file
+done_testing;