Prevent invisible skipping of ResultSource proxy overrides

*** 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 4006691) 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

@@ -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

lib/DBIx/Class/CDBICompat/ColumnCase.pm

@@ -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);
 }

lib/DBIx/Class/CDBICompat/ColumnGroups.pm

@@ -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);
 }

lib/DBIx/Class/MethodAttributes.pm

@@ -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

lib/DBIx/Class/ResultSource.pm

@@ -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;