Prevent invisible skipping of ResultSource proxy overrides
Peter Rabbitson [Mon, 6 Jun 2016 11:58:31 +0000 (13:58 +0200)]
*** NOTE ***
This does not add any new default functionality, nor does it alter DBIC's
behavior from how it solidified back in 2006: all this does is alert a user
when things are 99% not DWIM-ing (10 years overdue but better late than...)
*** NOTE ***

During the original design of DBIC the "ResultSourceProxy" system was
established in order to allow easy transition from Class::DBI. Sadly
it was not well abstracted away: it is rather difficult to use a custom
ResultSource subclass. The expansion of the DBIC project never addressed
this properly in the years since.

As a result when one wishes to override a part of the ResultSource
functionality, the overwhelmingly common practice is to hook a method in a
Result class and "hope for the best".

The subtle changes of various internal call-chains (mainly 4006691d) make
this silent uncertainty untenable. As a solution any such override will now
issue a descriptive warning that it has been bypassed during a direct
$rsrc->overriden_function invocation. A user now *must* determine how each
individual override must behave in this situation, and tag it with one of
the provided attributes.

For completeness the blueprint off which this solution was devised is
provided below:

  I = indirect (helper) method, never invoked by DBIC itself

* Rsrc method types
  . = rsrc_instance_specific_attribute type accessor (getter+setter)
  s = setter calling a . internally
  g = getter calling a . internally
  c = custom accessor

* Result method types
  P = proxied directly into ::Core via ::ResultSourceProxy (overridable)
  X = a ::Core proxy to ::ResultSource with extra logic (overridable)
  m = misc... stuff

    ___ Indirect methods ( the sanity checker warns when one "covers" these )
  /
 |   __ Rsrc methods somehow tied into the metadata state
 | /
 ||   _ Available to .../Result/... via ResultSourceProxy
 || /
 |||
 |||
DBIx::Class::ResultSource::View:
  .    is_virtual,
  .    deploy_depends_on,
  .    view_definition

DBIx::Class::ResultSource:
  c    schema

  .    source_name    # no proxy, but see FIXME at top of ::ResultSourceProxy

  .    _columns
  .    _ordered_columns
  .    _primaries
  .    _relationships
  .    _unique_constraints
  .P   column_info_from_storage
  .    name
  .P   result_class
  .P   resultset_attributes
  .P   resultset_class
  .P   source_info
  .    sqlt_deploy_callback

 IsX   add_column
  sX   add_columns
  sX   add_relationship,

 IsP   remove_column
  sP   remove_columns
  sP   add_unique_constraint
 IsP   add_unique_constraints
  sP   sequence
  sP   set_primary_key

 IgP   column_info
  gP   columns_info
  gP   columns

  gP   has_column
  gP   has_relationship
  gP   primary_columns
  gP   relationship_info
  gP   relationships

  gP   unique_constraint_columns
  gP   unique_constraint_names
  gP   unique_constraints

DBIx::Class::ResultSourceProxy::Table:
   m   table
   m   _init_result_source_instance

Changes
lib/DBIx/Class/CDBICompat/ColumnCase.pm
lib/DBIx/Class/CDBICompat/ColumnGroups.pm
lib/DBIx/Class/MethodAttributes.pm
lib/DBIx/Class/ResultSource.pm
lib/DBIx/Class/ResultSourceProxy.pm
lib/DBIx/Class/Schema/SanityChecker.pm
t/resultsource/rsrc_proxy_invocation.t [new file with mode: 0644]

diff --git a/Changes b/Changes
index 236b734..1884dd0 100644 (file)
--- a/Changes
+++ b/Changes
@@ -20,6 +20,9 @@ Revision history for DBIx::Class
           invoked when an error is leaving the DBIC internals to be handled by
           the caller (n.b. https://github.com/PerlDancer/Dancer2/issues/1125)
           (also fixes the previously rejected RT#63874)
+        - Overrides of ResultSourceProxy-provided methods are no longer skipped
+          silently: a one-per-callsite warning is issued any time this tricky
+          situation is encoutered https://is.gd/dbic_rsrcproxy_methodattr
         - $result->related_resultset() no longer passes extra arguments to
           an underlying search_rs(), as by design these arguments would be
           used only on the first call to ->related_resultset(), and ignored
index efbb881..7f308e8 100644 (file)
@@ -11,7 +11,7 @@ sub _register_column_group {
   return $class->next::method($group => map lc, @cols);
 }
 
-sub add_columns {
+sub add_columns :DBIC_method_is_bypassable_resultsource_proxy {
   my ($class, @cols) = @_;
   return $class->result_source->add_columns(map lc, @cols);
 }
index 73f845c..f65a358 100644 (file)
@@ -12,7 +12,7 @@ use namespace::clean;
 
 __PACKAGE__->mk_classdata('_column_groups' => { });
 
-sub columns {
+sub columns :DBIC_method_is_bypassable_resultsource_proxy {
   my $proto = shift;
   my $class = ref $proto || $proto;
   my $group = shift || "All";
@@ -34,7 +34,7 @@ sub _add_column_group {
   $class->_register_column_group($group => @cols);
 }
 
-sub add_columns {
+sub add_columns :DBIC_method_is_bypassable_resultsource_proxy {
   my ($class, @cols) = @_;
   $class->result_source->add_columns(@cols);
 }
index 6c23988..8dfb072 100644 (file)
@@ -159,6 +159,8 @@ sub VALID_DBIC_CODE_ATTRIBUTE {
   $_[1] =~ /^ DBIC_method_is_ (?:
     indirect_sugar
       |
+    (?: bypassable | mandatory ) _resultsource_proxy
+      |
     generated_from_resultsource_metadata
       |
     (?: inflated_ | filtered_ )? column_ (?: extra_)? accessor
@@ -237,6 +239,63 @@ L<DBIx::Class::ResultSet/create> and L<DBIx::Class::Schema/connect>.
 See also the check
 L<DBIx::Class::Schema::SanityChecker/no_indirect_method_overrides>.
 
+=head3 DBIC_method_is_mandatory_resultsource_proxy
+
+=head3 DBIC_method_is_bypassable_resultsource_proxy
+
+The presence of one of these attributes on a L<proxied ResultSource
+method|DBIx::Class::Manual::ResultClass/DBIx::Class::ResultSource> indicates
+how DBIC will behave when someone calls e.g.:
+
+  $some_result->result_source->add_columns(...)
+
+as opposed to the conventional
+
+  SomeResultClass->add_columns(...)
+
+This distinction becomes important when someone declares a sub named after
+one of the (currently 22) methods proxied from a
+L<Result|DBIx::Class::Manual::ResultClass> to
+L<ResultSource|DBIx::Class::ResultSource>. While there are obviously no
+problems when these methods are called at compile time, there is a lot of
+ambiguity whether an override of something like
+L<columns_info|DBIx::Class::ResultSource/columns_info> will be respected by
+DBIC and various plugins during runtime operations.
+
+It must be noted that there is a reason for this weird situation: during the
+original design of DBIC the "ResultSourceProxy" system was established in
+order to allow easy transition from Class::DBI. Unfortunately it was not
+well abstracted away: it is rather difficult to use a custom ResultSource
+subclass. The expansion of the DBIC project never addressed this properly
+in the years since. As a result when one wishes to override a part of the
+ResultSource functionality, the overwhelming practice is to hook a method
+in a Result class and "hope for the best".
+
+The subtle changes of various internal call-chains in C<DBIC v0.0829xx> make
+this silent uncertainty untenable. As a solution any such override will now
+issue a descriptive warning that it has been bypassed during a
+C<< $rsrc->overriden_function >> invocation. A user B<must> determine how
+each individual override must behave in this situation, and tag it with one
+of the above two attributes.
+
+Naturally any override marked with C<..._bypassable_resultsource_proxy> will
+behave like it did before: it will be silently ignored. This is the attribute
+you want to set if your code appears to work fine, and you do not wish to
+receive the warning anymore (though you are strongly encouraged to understand
+the other option).
+
+However overrides marked with C<..._mandatory_resultsource_proxy> will always
+be reinvoked by DBIC itself, so that any call of the form:
+
+  $some_result->result_source->columns_info(...)
+
+will be transformed into:
+
+  $some_result->result_source->result_class->columns_info(...)
+
+with the rest of the callchain flowing out of that (provided the override did
+invoke L<next::method|mro/next::method> where appropriate)
+
 =head3 DBIC_method_is_generated_from_resultsource_metadata
 
 This attribute is applied to all methods dynamically installed after various
index 85d0bfc..9470546 100644 (file)
@@ -1,5 +1,15 @@
 package DBIx::Class::ResultSource;
 
+### !!!NOTE!!!
+#
+# Some of the methods defined here will be around()-ed by code at the
+# end of ::ResultSourceProxy. The reason for this strange arrangement
+# is that the list of around()s of methods in this # class depends
+# directly on the list of may-not-be-defined-yet methods within
+# ::ResultSourceProxy itself.
+# If this sounds terrible - it is. But got to work with what we have.
+#
+
 use strict;
 use warnings;
 
index cd18d2e..b8f0082 100644 (file)
@@ -6,12 +6,25 @@ use warnings;
 
 use base 'DBIx::Class';
 
+# ! LOAD ORDER SENSITIVE !
 # needs to be loaded early to query method attributes below
+# and to do the around()s properly
 use DBIx::Class::ResultSource;
+my @wrap_rsrc_methods = qw(
+  add_columns
+  add_relationship
+);
 
-use DBIx::Class::_Util qw( quote_sub fail_on_internal_call );
+use DBIx::Class::_Util qw(
+  quote_sub perlstring fail_on_internal_call describe_class_methods
+);
 use namespace::clean;
 
+# FIXME: this is truly bizarre, not sure why it is this way since 93405cf0
+# This value *IS* *DIFFERENT* from source_name in the underlying rsrc
+# instance, and there is *ZERO EFFORT* made to synchronize them...
+# FIXME: Due to the above marking this as a rsrc_proxy method is also out
+# of the question...
 __PACKAGE__->mk_group_accessors('inherited_ro_instance' => 'source_name');
 
 sub get_inherited_ro_instance { $_[0]->get_inherited($_[1]) }
@@ -23,10 +36,11 @@ sub set_inherited_ro_instance {
   $_[0]->set_inherited( $_[1], $_[2] );
 }
 
-
-sub add_columns {
+sub add_columns :DBIC_method_is_bypassable_resultsource_proxy {
   my ($class, @cols) = @_;
   my $source = $class->result_source;
+  local $source->{__callstack_includes_rsrc_proxy_method} = "add_columns";
+
   $source->add_columns(@cols);
 
   my $colinfos;
@@ -46,10 +60,11 @@ sub add_column :DBIC_method_is_indirect_sugar {
   shift->add_columns(@_)
 }
 
-
-sub add_relationship {
+sub add_relationship :DBIC_method_is_bypassable_resultsource_proxy {
   my ($class, $rel, @rest) = @_;
   my $source = $class->result_source;
+  local $source->{__callstack_includes_rsrc_proxy_method} = "add_relationship";
+
   $source->add_relationship($rel => @rest);
   $class->register_relationship($rel => $source->relationship_info($rel));
 }
@@ -92,18 +107,272 @@ for my $method_to_proxy (qw/
   relationship_info
   has_relationship
 /) {
+  my $qsub_opts = { attributes => [
+    do {
+      no strict 'refs';
+      attributes::get( \&{"DBIx::Class::ResultSource::$method_to_proxy"} );
+    }
+  ] };
 
-  my $qsub_opts = { attributes => [ do {
-    no strict 'refs';
-    attributes::get( \&{"DBIx::Class::ResultSource::$method_to_proxy"} )
-  } ] };
+  # bypassable default for backcompat, except for indirect methods
+  # ( those will simply warn during the sanheck )
+  if(! grep
+    { $_ eq 'DBIC_method_is_indirect_sugar' }
+    @{ $qsub_opts->{attributes} }
+  ) {
+    push @wrap_rsrc_methods, $method_to_proxy;
+    push @{ $qsub_opts->{atributes} }, 'DBIC_method_is_bypassable_resultsource_proxy';
+  }
 
   quote_sub __PACKAGE__."::$method_to_proxy", sprintf( <<'EOC', $method_to_proxy ), {}, $qsub_opts;
     DBIx::Class::_ENV_::ASSERT_NO_INTERNAL_INDIRECT_CALLS and DBIx::Class::_Util::fail_on_internal_call;
 
-    shift->result_source->%s (@_);
+    my $rsrc = shift->result_source;
+    local $rsrc->{__callstack_includes_rsrc_proxy_method} = q(%1$s);
+    $rsrc->%1$s (@_);
+EOC
+
+}
+
+# This is where the "magic" of detecting/invoking the proper overridden
+# Result method takes place. It isn't implemented as a stateless out-of-band
+# SanityCheck as invocation requires certain state in the $rsrc object itself
+# in order not to loop over itself. It is not in ResultSource.pm either
+# because of load order and because the entire stack is just terrible :/
+#
+# The code is not easily readable, as it it optimized for execution time
+# (this stuff will be run all the time across the entire install base :/ )
+#
+{
+  our %__rsrc_proxy_meta_cache;
+
+  sub DBIx::Class::__RsrcProxy_iThreads_handler__::CLONE {
+    # recreating this cache is pretty cheap: just blow it away
+    %__rsrc_proxy_meta_cache = ();
+  }
+
+  for my $method_to_wrap (@wrap_rsrc_methods) {
+
+    my @src_args = (
+      perlstring $method_to_wrap,
+    );
+
+    my $orig = do {
+      no strict 'refs';
+      \&{"DBIx::Class::ResultSource::$method_to_wrap"}
+    };
+
+    my %unclassified_override_warn_emitted;
+
+    my @qsub_args = (
+      {
+        # ref to hashref, this is how S::Q works
+        '$rsrc_proxy_meta_cache' => \\%__rsrc_proxy_meta_cache,
+        '$unclassified_override_warn_emitted' => \\%unclassified_override_warn_emitted,
+        '$orig' => \$orig,
+      },
+      { attributes => [ attributes::get($orig) ] }
+    );
+
+    quote_sub "DBIx::Class::ResultSource::$method_to_wrap", sprintf( <<'EOC', @src_args ), @qsub_args;
+
+      my $overridden_proxy_cref;
+
+      # fall through except when...
+      return &$orig unless (
+
+        # FIXME - this may be necessary some day, but skip the hit for now
+        # Scalar::Util::reftype $_[0] eq 'HASH'
+        #   and
+
+        # there is a class to check in the first place
+        defined $_[0]->{result_class}
+
+          and
+        # we are not in a reinvoked callstack
+        (
+          ( $_[0]->{__callstack_includes_rsrc_proxy_method} || '' )
+            ne
+          %1$s
+        )
+
+          and
+        # there is a proxied method in the first place
+        (
+          ( $rsrc_proxy_meta_cache->{address}{%1$s} ||= 0 + (
+            DBIx::Class::ResultSourceProxy->can(%1$s)
+              ||
+            -1
+          ) )
+            >
+          0
+        )
+
+          and
+        # the proxied method *is overridden*
+        (
+          $rsrc_proxy_meta_cache->{address}{%1$s}
+            !=
+          # the can() should not be able to fail in theory, but the
+          # result class may not inherit from ::Core *at all*
+          # hence we simply ||ourselves to paper over this eventuality
+          (
+            ( $overridden_proxy_cref = $_[0]->{result_class}->can(%1$s) )
+              ||
+            $rsrc_proxy_meta_cache->{address}{%1$s}
+          )
+        )
+
+          and
+        # no short-circuiting atributes
+        (! grep
+          {
+            # checking that:
+            #
+            # - Override is not something DBIC plastered on top of things
+            #   One would think this is crazy, yet there it is... sigh:
+            #   https://metacpan.org/source/KARMAN/DBIx-Class-RDBOHelpers-0.12/t/lib/MyDBIC/Schema/Cd.pm#L26-27
+            #
+            # - And is not an m2m crapfest
+            #
+            # - And is not something marked as bypassable
+
+            $_ =~ / ^ DBIC_method_is_ (?:
+              generated_from_resultsource_metadata
+                |
+              m2m_ (?: extra_)? sugar (?:_with_attrs)?
+                |
+              bypassable_resultsource_proxy
+            ) $ /x
+          }
+          keys %%{ $rsrc_proxy_meta_cache->{attrs}{$overridden_proxy_cref} ||= {
+            map { $_ => 1 } attributes::get($overridden_proxy_cref)
+          }}
+        )
+      );
+
+      # Getting this far means that there *is* an override
+      # and it is *not* marked for a skip
+
+      # we were asked to loop back through the Result override
+      if (
+        $rsrc_proxy_meta_cache->{attrs}
+                                 {$overridden_proxy_cref}
+                                  {DBIC_method_is_mandatory_resultsource_proxy}
+      ) {
+        local $_[0]->{__callstack_includes_rsrc_proxy_method} = %1$s;
+
+        # replace $self without compromising aliasing
+        splice @_, 0, 1, $_[0]->{result_class};
+
+        return &$overridden_proxy_cref;
+      }
+      # complain (sparsely) and carry on
+      else {
+
+        # FIXME!!! - terrible, need to swap for something saner later
+        my ($cs) = DBIx::Class::Carp::__find_caller( __PACKAGE__ );
+
+        my $key = $cs . $overridden_proxy_cref;
+
+        unless( $unclassified_override_warn_emitted->{$key} ) {
+
+          # find the real origin
+          my @meth_stack = @{ DBIx::Class::_Util::describe_class_methods(
+            ref $_[0]->{result_class} || $_[0]->{result_class}
+          )->{methods}{%1$s} };
+
+          my $in_class = (shift @meth_stack)->{via_class};
+
+          my $possible_supers;
+          while (
+            @meth_stack
+              and
+            $meth_stack[0]{via_class} ne __PACKAGE__
+          ) {
+            push @$possible_supers, (shift @meth_stack)->{via_class};
+          }
+
+          $possible_supers = $possible_supers
+            ? sprintf(
+              ' ( and possible SUPERs: %%s )',
+              join ', ', map
+                { join '::', $_, %1$s }
+                @$possible_supers
+            )
+            : ''
+          ;
+
+          my $fqmeth = $in_class . '::' . %1$s . '()';
+
+          DBIx::Class::_Util::emit_loud_diag(
+          # Repurpose the assertion envvar ( the override-check is independent
+          # from the schema san-checker, but the spirit is the same )
+            confess => $ENV{DBIC_ASSERT_NO_FAILING_SANITY_CHECKS},
+            msg =>
+              "The override method $fqmeth$possible_supers has been bypassed "
+            . "$cs\n"
+            . "In order to silence this warning you must tag the "
+            . "definition of $fqmeth with one of the attributes "
+            . "':DBIC_method_is_bypassable_resultsource_proxy' or "
+            . "':DBIC_method_is_mandatory_resultsource_proxy' ( see "
+            . "https://is.gd/dbic_rsrcproxy_methodattr for more info )\n"
+          );
+
+          # only set if we didn't throw
+          $unclassified_override_warn_emitted->{$key} = 1;
+        }
+
+        return &$orig;
+      }
 EOC
 
+  }
+
+  Class::C3->reinitialize() if DBIx::Class::_ENV_::OLD_MRO;
+}
+
+# CI sanity check that all annotations make sense
+if(
+  DBIx::Class::_ENV_::ASSERT_NO_ERRONEOUS_METAINSTANCE_USE
+    and
+  # no point taxing 5.8 with this
+  ! DBIx::Class::_ENV_::OLD_MRO
+) {
+
+  my ( $rsrc_methods, $rsrc_proxy_methods, $base_methods ) = map {
+    describe_class_methods($_)->{methods}
+  } qw(
+    DBIx::Class::ResultSource
+    DBIx::Class::ResultSourceProxy
+    DBIx::Class
+  );
+
+  delete $rsrc_methods->{$_}, delete $rsrc_proxy_methods->{$_}
+    for keys %$base_methods;
+
+  (
+    $rsrc_methods->{$_}
+      and
+    ! $rsrc_proxy_methods->{$_}[0]{attributes}{DBIC_method_is_indirect_sugar}
+  )
+    or
+  delete $rsrc_proxy_methods->{$_}
+    for keys %$rsrc_proxy_methods;
+
+  # see fat FIXME at top of file
+  delete @{$rsrc_proxy_methods}{qw( source_name _source_name_accessor )};
+
+  if (
+    ( my $proxied = join "\n", map "\t$_", sort keys %$rsrc_proxy_methods )
+      ne
+    ( my $wrapped = join "\n", map "\t$_", sort @wrap_rsrc_methods )
+  ) {
+    Carp::confess(
+      "Unexpected mismatch between the list of proxied methods:\n\n$proxied"
+    . "\n\nand the list of wrapped rsrc methods:\n\n$wrapped\n\n"
+    );
+  }
 }
 
 1;
index c6b3e50..e4ca5b3 100644 (file)
@@ -371,7 +371,12 @@ sub check_no_indirect_method_overrides {
       push @err, {
         overriden => {
           name => $_->{name},
-          via_class => $_->{via_class}
+          via_class => (
+            # this way we report a much better Dwarn oneliner in the error
+            $_->{attributes}{DBIC_method_is_bypassable_resultsource_proxy}
+              ? 'DBIx::Class::ResultSource'
+              : $_->{via_class}
+          ),
         },
         by => [ map { "$_->{via_class}::$_->{name}" } @$nonsugar_methods ],
       } if (
diff --git a/t/resultsource/rsrc_proxy_invocation.t b/t/resultsource/rsrc_proxy_invocation.t
new file mode 100644 (file)
index 0000000..dc4c9d4
--- /dev/null
@@ -0,0 +1,61 @@
+BEGIN { do "./t/lib/ANFANG.pm" or die ( $@ || $! ) }
+
+$ENV{DBIC_ASSERT_NO_FAILING_SANITY_CHECKS} = 1;
+
+use strict;
+use warnings;
+
+use Test::More;
+
+use DBICTest;
+use Sub::Quote 'quote_sub';
+
+my $colinfo = DBICTest::Schema::Artist->result_source->column_info('artistid');
+
+my $schema = DBICTest->init_schema ( no_deploy => 1 );
+my $rsrc = $schema->source("Artist");
+
+for my $overrides_marked_mandatory (0, 1) {
+  my $call_count;
+  my @methods_to_override = qw(
+    add_columns columns_info
+  );
+
+  my $attr = { attributes => [
+    $overrides_marked_mandatory
+      ? 'DBIC_method_is_mandatory_resultsource_proxy'
+      : 'DBIC_method_is_bypassable_resultsource_proxy'
+  ] };
+
+  for (@methods_to_override) {
+    $call_count->{$_} = 0;
+
+    quote_sub( "DBICTest::Schema::Artist::$_", <<'EOC', { '$cnt' => \\($call_count->{$_}) }, $attr );
+      $$cnt++;
+      shift->next::method(@_);
+EOC
+  }
+
+  Class::C3->reinitialize() if DBIx::Class::_ENV_::OLD_MRO;
+
+  is_deeply
+    $rsrc->columns_info->{artistid},
+    $colinfo,
+    'Expected result from rsrc getter',
+  ;
+
+  $rsrc->add_columns("bar");
+
+  is_deeply
+    $call_count,
+    {
+      add_columns => ($overrides_marked_mandatory ? 1 : 0),
+
+      # ResultSourceProxy::add_columns will call colinfos as well
+      columns_info => ($overrides_marked_mandatory ? 2 : 0),
+    },
+    'expected rsrc proxy override callcounts',
+  ;
+}
+
+done_testing;