Introducing DBIx::Class::Schema::SanityChecker

This gives us comprehensive diagnostic on incorrect component composition and other hard to track... stuff. Given the huge amount of changes to call chains (specifically the changes in 77c3a5d and e505369), and the fallout seen on CPAN and darkpan due to these modifications, the status quo became clearly untennable. To mitigate the (often silent) breakage a brand new "sanity check" framework was introduced as part of the ::Schema setup-cycle (and is enabled by default) Same DBIx::Class::Helper v2.032002 test time shoots from 65.5s all the way to 76.0s, a 16% slowdown. However the moment the framework is disabled by flipping $schema->schema_sanity_checker to a defined-but-false value - the startup impact is entirely gone. The changset was extensively tested against the following set of downstream dists (a superset of c8b1011), with each warning hand-confirmed to be a valid description of a real problem: --- actual bash script passing on a *heavily* massaged PERL5LIB set -o pipefail export PERL_CPANM_OPT= export PERL5LIB="/home/rabbit/devel/dbic/dbgit/lib:$PERL5LIB" export DBICTEST_SQLT_DEPLOY=0 export DBIC_ASSERT_NO_ERRONEOUS_METAINSTANCE_USE=1 export DBIC_ASSERT_NO_FAILING_SANITY_CHECKS=1 # these fail with ERRONEOUS_METAINSTANCE_USE alone # (S::L fails due to PG_DSN but I think is ok besides that) for d in \ DBICx::Shortcuts \ DBIx::Class::Bootstrap::Simple \ DBIx::Class::Preview \ DBIx::Class::Schema::Loader \ Pinto \ ; do \ DBIC_ASSERT_NO_ERRONEOUS_METAINSTANCE_USE=0 \ DBIC_ASSERT_NO_FAILING_SANITY_CHECKS=0 \ DBICTEST_PG_DSN= \ cpanm -v --reinstall $d 2>&1 \ | tee -a /dev/shm/umpfh \ | grep -P -B1 'sanity check|emit_|^(Building and testing|Result:)' || exit 1 \ ; done # these emit various san-check related problems for d in \ RapidApp \ Data::OFAC \ DBIx::Class::VirtualColumns \ "DBD::SQLite@1.35 Handel" \ DBIx::Class::RDBOHelpers \ CatalystX::CRUD::ModelAdapter::DBIC \ DBICx::Indexing \ DBICx::TestDatabase \ DBIx::Class::BitField \ DBIx::Class::I18NColumns \ DBIx::Class::PhoneticSearch \ DBIx::Class::RandomColumns \ DBIx::Class::ResultSource::MultipleTableInheritance \ DBIx::Class::Result::ProxyField \ DBIx::Class::Schema::PopulateMore \ DBIx::Class::Tree \ Foorum \ Interchange6::Schema \ Test::DBIx::Class \ TreePath \ ; do \ DBIC_ASSERT_NO_FAILING_SANITY_CHECKS=0 \ cpanm -v --reinstall $d 2>&1 \ | tee -a /dev/shm/umpfh \ | grep -P -B1 'sanity check|emit_|^(Building and testing|Result:)' || exit 1 \ ; done # these are entirely unaffected \o/ for d in \ Dancer2::Plugin::DBIC \ App::DBCritic \ App::DH \ AproJo \ Articulate \ Authorization::RBAC \ BackPAN::Index \ Bio::Chado::Schema \ Bot::BasicBot::Pluggable::Module::Notes \ Bracket \ Business::Cart::Generic \ Business::DPD \ Catalyst::Authentication::Credential::Facebook \ Catalyst::Authentication::Store::DBIx::Class \ Catalyst::Controller::DBIC::API \ Catalyst::Model::DBIC::Plain \ Catalyst::Model::DBIC::Schema \ Catalyst::Model::DBIC::Schema::PerRequest \ Catalyst::Model::FormFu \ Catalyst::Plugin::Authentication::Store::DBIC \ Catalyst::Plugin::Authorization::Abilities \ Catalyst::Plugin::AutoCRUD \ Catalyst::Plugin::DBIC::Schema::Profiler \ Catalyst::Plugin::Session::Store::DBIC \ Catalyst::TraitFor::Controller::DBIC::DoesPaging \ Catalyst::TraitFor::Model::DBIC::Schema::RequestConnectionPool \ Catalyst::TraitFor::Model::DBIC::Schema::Result \ Catalyst::TraitFor::Model::DBIC::Schema::WithCurrentUser \ Catalyst::View::CSV \ CatalystX::Controller::ExtJS::REST::SimpleExcel \ CatalystX::Crudite \ CatalystX::Eta \ CatalystX::OAuth2 \ CatalystX::Resource \ CGI::Application::Plugin::Authentication::Driver::DBIC \ CGI::Application::Plugin::DBIC::Schema \ CGI::Application::Plugin::DBIx::Class \ CGI::Application::Plugin::ExtJS \ CGI::Session::Driver::dbic \ Cookieville \ Dancer2::Plugin::Auth::Extensible::Provider::DBIC \ Dancer2::Session::DBIC \ Dancer::Plugin::Auth::Extensible::Provider::DBIC \ Dancer::Plugin::Auth::RBAC::Credentials::DBIC \ Dancer::Plugin::Auth::RBAC::Permissions::DBIC \ Dancer::Plugin::DBIC \ Dancer::Session::DBIC \ Data::Morph \ DBICx::AutoDoc \ DBICx::Backend::Move \ DBICx::DataDictionary \ DBICx::Deploy \ DBICx::Hooks \ DBICx::MapMaker \ DBICx::MaterializedPath \ DBICx::Modeler \ DBICx::Sugar \ DBICx::TxnInsert \ DBIx::Class::AlwaysUpdate \ DBIx::Class::AuditAny \ DBIx::Class::AuditLog \ DBIx::Class::BatchUpdate \ DBIx::Class::Candy \ DBIx::Class::ColumnDefault \ DBIx::Class::CompressColumns \ DBIx::Class::Cursor::Cached \ DBIx::Class::CustomPrefetch \ DBIx::Class::DateTime::Epoch \ DBIx::Class::DeleteAction \ DBIx::Class::DeploymentHandler \ DBIx::Class::DigestColumns \ DBIx::Class::DynamicDefault \ DBIx::Class::DynamicSubclass \ DBIx::Class::EasyFixture \ DBIx::Class::ElasticSync \ DBIx::Class::EncodeColumns \ DBIx::Class::EncodedColumn \ DBIx::Class::Factory \ DBIx::Class::Fixtures \ DBIx::Class::ForceUTF8 \ DBIx::Class::FormatColumns \ DBIx::Class::FormTools \ DBIx::Class::FromSledge \ DBIx::Class::FrozenColumns \ DBIx::Class::GeomColumns \ DBIx::Class::Graph \ DBIx::Class::Helpers \ DBIx::Class::HTML::FormFu \ DBIx::Class::HTMLWidget \ DBIx::Class::Indexed \ DBIx::Class::InflateColumn::Authen::Passphrase \ DBIx::Class::InflateColumn::BigFloat \ DBIx::Class::InflateColumn::Boolean \ DBIx::Class::InflateColumn::Currency \ DBIx::Class::InflateColumn::DateTime::Duration \ DBIx::Class::InflateColumn::DateTime::WithTimeZone \ DBIx::Class::InflateColumn::DateTimeX::Immutable \ DBIx::Class::InflateColumn::FS \ DBIx::Class::InflateColumn::IP \ DBIx::Class::InflateColumn::Markup::Unified \ DBIx::Class::InflateColumn::Math::Currency \ DBIx::Class::InflateColumn::Object::Enum \ DBIx::Class::InflateColumn::Path::Class \ DBIx::Class::InflateColumn::Serializer \ DBIx::Class::InflateColumn::Serializer::JSYNC \ DBIx::Class::InflateColumn::Serializer::Role::HashContentAccessor \ DBIx::Class::InflateColumn::Serializer::Sereal \ DBIx::Class::InflateColumn::Time \ DBIx::Class::InflateColumn::TimeMoment \ DBIx::Class::InflateColumn::URI \ DBIx::Class::IntrospectableM2M \ DBIx::Class::Journal \ DBIx::Class::LibXMLdoc \ DBIx::Class::LookupColumn \ DBIx::Class::MaterializedPath \ DBIx::Class::Migration \ DBIx::Class::Numeric \ DBIx::Class::Objects \ DBIx::Class::OptimisticLocking \ DBIx::Class::ParameterizedJoinHack \ DBIx::Class::PassphraseColumn \ DBIx::Class::QueriesTime \ DBIx::Class::QueryLog \ DBIx::Class::QueryLog::WithStackTrace \ DBIx::Class::QueryProfiler \ DBIx::Class::RandomStringColumns \ DBIx::Class::Relationship::Predicate \ DBIx::Class::Report \ DBIx::Class::Result::ColumnData \ DBIx::Class::ResultSet::AccessorsEverywhere \ DBIx::Class::ResultSet::Data::Pageset \ DBIx::Class::ResultSet::Excel \ DBIx::Class::ResultSet::Faceter \ DBIx::Class::ResultSet::HashRef \ DBIx::Class::ResultSet::RecursiveUpdate \ DBIx::Class::Result::Validation \ DBIx::Class::SaltedPasswords \ DBIx::Class::Schema::Config \ DBIx::Class::Schema::Diff \ DBIx::Class::Schema::RestrictWithObject \ DBIx::Class::Schema::ResultSetAccessors \ DBIx::Class::Schema::Versioned::Inline \ DBIx::Class::Service \ DBIx::Class::SingletonRows \ DBIx::Class::Storage::DBI::mysql::backup \ DBIx::Class::Storage::DBI::ODBC::OPENEDGE \ DBIx::Class::Storage::DBI::OpenEdge \ DBIx::Class::StorageReadOnly \ DBIx::Class::Storage::TxnEndHook \ DBIx::Class::TimeStamp \ DBIx::Class::Tokenize \ DBIx::Class::TopoSort \ DBIx::Class::Tree::CalculateSets \ DBIx::Class::Tree::Mobius \ DBIx::Class::UnicornLogger \ DBIx::Class::UserStamp \ DBIx::Class::UUIDColumns \ DBIx::Class::Validation \ DBIx::Class::Validation::Structure \ DBIx::Class::WebForm \ DBIx::Class::Wrapper \ DBIx::Table::TestDataGenerator \ Data::Importer \ Dwimmer \ ETLp \ ExtJS::Generator::DBIC \ Finance::QuoteDB \ Form::Processor::Model::DBIC \ Form::Sensible::Reflector::DBIC \ FormValidator::Simple::Plugin::DBIC::Unique \ Galileo \ GenOO \ HTML::FormFu::ExtJS \ HTML::FormFu::Model::DBIC \ HTML::FormHandler::Model::DBIC \ Hyle \ IronMan::Schema \ KiokuDB::Backend::DBI \ Log::Log4perl::Appender::DBIx::Class \ Mixin::ExtraFields::Driver::DBIC \ Module::CPANTS::ProcessCPAN \ Mojolicious::Plugin::DBICAdmin \ MooseX::Types::DBIx::Class \ OpusVL::AppKit \ OpusVL::AppKit::Schema::AppKitAuthDB \ OpusVL::Preferences \ OpusVL::SysParams \ Prosody \ Pulp \ RackMan \ Reaction \ Schema::RackTables \ Tapper::MCP \ Tapper::Schema \ Template::Provider::CustomDBIC \ Template::Provider::DBIC \ Template::Provider::PerContextDBIC \ Template::Provider::PrefixDBIC \ Test::DBIC::ExpectedQueries \ Test::DBIC::Schema::Connector \ Test::DBIx::Class::Schema \ Test::Fixture::DBIC::Schema \ Tie::DBIx::Class \ Types::DBIx::Class \ WebAPI::DBIC \ WebNano::Controller::CRUD \ Web::Util::DBIC::Paging \ Web::Util::ExtPaging \ WWW::Hashbang::Pastebin \ WWW::RobotRules::DBIC \ YAWF \ YATT::Lite \ Yeb::Plugin::DBIC \ "DBD::SQLite@1.35 Catalyst::ActionRole::BuildDBICResult DBIx::NoSQL Jedi::Plugin::Session Jedi::Plugin::Auth" \ "Test::More@1.001014 Test::DBIx::Class::Stats" \ "Mojolicious@3.91 ExpenseTracker" \ "Dancer2@0.166001 Strehler Strehler::Element::Extra Strehler::RSS" \ ; do \ cpanm -v --reinstall $d 2>&1 \ | tee -a /dev/shm/umpfh \ | grep -P -B1 '^(Building and testing|Result:)' || exit 1 \ ; done echo echo 'YAY!' exit 0
Perl5 · Jul 26, 2016 · 12e7015 · 12e7015
1 parent 1b822bd
commit 12e7015
Show file tree

Hide file tree

Showing 17 changed files with 883 additions and 5 deletions.
diff --git a/Changes b/Changes
@@ -33,6 +33,10 @@ Revision history for DBIx::Class
           instead of silently discarding the argument
 
     * New Features
+        - DBIC now performs a range of sanity checks on the entire hierarchy
+          of Schema/Result/ResultSet classes loudly alerting the end user to
+          potential extremely hard-to-diagnose pitfalls ( RT#93976, also fully
+          addresses https://blog.afoolishmanifesto.com/posts/mros-and-you/ )
         - InflateColumn::DateTime now accepts the ecosystem-standard option
           'time_zone', in addition to the DBIC-only 'timezone' (GH#28)
         - Massively optimised literal SQL snippet scanner - fixes all known

diff --git a/examples/Schema/MyApp/Schema.pm b/examples/Schema/MyApp/Schema.pm
@@ -6,4 +6,9 @@ use strict;
 use base qw/DBIx::Class::Schema/;
 __PACKAGE__->load_namespaces;
 
+# no point taxing 5.8, but otherwise leave the default: a user may
+# be interested in exploring and seeing what broke
+__PACKAGE__->schema_sanity_checker('')
+  if DBIx::Class::_ENV_::OLD_MRO;
+
 1;
diff --git a/lib/DBIx/Class/MethodAttributes.pm b/lib/DBIx/Class/MethodAttributes.pm
@@ -222,6 +222,9 @@ itself and various plugins are much more likely to invoke alternative direct
 call paths, bypassing your override entirely. Good examples of this are
 L<DBIx::Class::ResultSet/create> and L<DBIx::Class::Schema/connect>.
 
+See also the check
+L<DBIx::Class::Schema::SanityChecker/no_indirect_method_overrides>.
+
 =head1 METHODS
 
 =head2 MODIFY_CODE_ATTRIBUTES

diff --git a/lib/DBIx/Class/Schema.pm b/lib/DBIx/Class/Schema.pm
@@ -7,8 +7,9 @@ use base 'DBIx::Class';
 
 use DBIx::Class::Carp;
 use Try::Tiny;
-use Scalar::Util qw/weaken blessed/;
+use Scalar::Util qw( weaken blessed refaddr );
 use DBIx::Class::_Util qw(
+  false emit_loud_diag refdesc
   refcount quote_sub scope_guard
   is_exception dbic_internal_try
   fail_on_internal_call emit_loud_diag
@@ -27,6 +28,12 @@ __PACKAGE__->mk_classaccessor('default_resultset_attributes' => {});
 __PACKAGE__->mk_classaccessor('class_mappings' => {});
 __PACKAGE__->mk_classaccessor('source_registrations' => {});
 
+__PACKAGE__->mk_group_accessors( component_class => 'schema_sanity_checker' );
+__PACKAGE__->schema_sanity_checker(
+  DBIx::Class::_ENV_::OLD_MRO ? false :
+  'DBIx::Class::Schema::SanityChecker'
+);
+
 =head1 NAME
 
 DBIx::Class::Schema - composable schemas
@@ -454,6 +461,42 @@ Example:
    use base qw/DBIx::Class::Schema/;
    __PACKAGE__->default_resultset_attributes( { software_limit => 1 } );
 
+=head2 schema_sanity_checker
+
+=over 4
+
+=item Arguments: L<perform_schema_sanity_checks()|DBIx::Class::Schema::SanityChecker/perform_schema_sanity_checks> provider
+
+=item Return Value: L<perform_schema_sanity_checks()|DBIx::Class::Schema::SanityChecker/perform_schema_sanity_checks> provider
+
+=item Default value: L<DBIx::Class::Schema::SanityChecker>
+
+=back
+
+On every call to L</connection> if the value of this attribute evaluates to
+true, DBIC will invoke
+C<< L<$schema_sanity_checker|/schema_sanity_checker>->L<perform_schema_sanity_checks|DBIx::Class::Schema::SanityChecker/perform_schema_sanity_checks>($schema) >>
+before returning. The return value of this invocation is ignored.
+
+B<YOU ARE STRONGLY URGED> to
+L<learn more about the reason|DBIx::Class::Schema::SanityChecker/WHY> this
+feature was introduced. Blindly disabling the checker on existing projects
+B<may result in data corruption> after upgrade to C<< DBIC >= v0.082900 >>.
+
+Example:
+
+   package My::Schema;
+   use base qw/DBIx::Class::Schema/;
+   __PACKAGE__->schema_sanity_checker('My::Schema::SanityChecker');
+
+   # or to disable all checks:
+   __PACKAGE__->schema_sanity_checker('');
+
+Note: setting the value to C<undef> B<will not> have the desired effect,
+due to an implementation detail of L<Class::Accessor::Grouped> inherited
+accessors. In order to disable any and all checks you must set this
+attribute to an empty string as shown in the second example above.
+
 =head2 exception_action
 
 =over 4
@@ -859,12 +902,17 @@ Similar to L</connect> except sets the storage object and connection
 data B<in-place> on C<$self>. You should probably be calling
 L</connect> to get a properly L<cloned|/clone> Schema object instead.
 
+If the accessor L</schema_sanity_checker> returns a true value C<$checker>,
+the following call will take place before return:
+C<< L<$checker|/schema_sanity_checker>->L<perform_schema_sanity_checks(C<$self>)|DBIx::Class::Schema::SanityChecker/perform_schema_sanity_checks> >>
+
 =head3 Overloading
 
 Overload C<connection> to change the behaviour of C<connect>.
 
 =cut
 
+my $default_off_stderr_blurb_emitted;
 sub connection {
   my ($self, @info) = @_;
   return $self if !@info && $self->storage;
@@ -888,7 +936,53 @@ sub connection {
   my $storage = $storage_class->new( $self => $args||{} );
   $storage->connect_info(\@info);
   $self->storage($storage);
-  return $self;
+
+
+###
+### Begin 5.8 "you have not selected a checker" warning
+###
+  # We can not blanket-enable this on 5.8 - it is just too expensive for
+  # day to day execution. We also can't just go silent - there are genuine
+  # regressions ( due to core changes) for which this is the only line of
+  # defense. So instead we whine on STDERR that folks need to do something
+  #
+  # Beyond suboptimal, but given the constraints the best we can do :(
+  #
+  # This should stay around for at least 3~4 years
+  #
+  DBIx::Class::_ENV_::OLD_MRO
+    and
+  ! $default_off_stderr_blurb_emitted
+    and
+  length ref $self->schema_sanity_checker
+    and
+  length ref __PACKAGE__->schema_sanity_checker
+    and
+  (
+    refaddr( $self->schema_sanity_checker )
+      ==
+    refaddr( __PACKAGE__->schema_sanity_checker )
+  )
+    and
+  emit_loud_diag(
+    msg => sprintf(
+    "Sanity checks for schema %s are disabled on this perl $]: "
+  . '*THIS IS POTENTIALLY VERY DANGEROUS*. You are strongly urged to '
+  . "read http://is.gd/dbic_sancheck_5_008 before proceeding\n",
+    ( defined( blessed $self ) ? refdesc $self : "'$self'" )
+  ))
+    and
+  $default_off_stderr_blurb_emitted = 1;
+###
+### End 5.8 "you have not selected a checker" warning
+###
+
+
+  if( my $checker = $self->schema_sanity_checker ) {
+    $checker->perform_schema_sanity_checks($self);
+  }
+
+  $self;
 }
 
 sub _normalize_storage_type {