From: Peter Rabbitson Date: Mon, 23 May 2016 09:08:17 +0000 (+0200) Subject: Introducing DBIx::Class::Schema::SanityChecker X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=12e7015aa9372aeaf1aaa7e125b8ac8da216deb5;p=dbsrgits%2FDBIx-Class.git 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 77c3a5dc and e5053694), 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 c8b1011e), 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 --- diff --git a/Changes b/Changes index 45b2f87..236b734 100644 --- 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 index 3642e82..fdfa82b 100644 --- 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 index 0dec0b3..0365dad 100644 --- 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 and L. +See also the check +L. + =head1 METHODS =head2 MODIFY_CODE_ATTRIBUTES diff --git a/lib/DBIx/Class/Schema.pm b/lib/DBIx/Class/Schema.pm index 618b585..83d7e09 100644 --- 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 provider + +=item Return Value: L provider + +=item Default value: L + +=back + +On every call to L if the value of this attribute evaluates to +true, DBIC will invoke +C<< L<$schema_sanity_checker|/schema_sanity_checker>->L($schema) >> +before returning. The return value of this invocation is ignored. + +B to +L this +feature was introduced. Blindly disabling the checker on existing projects +B 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 B have the desired effect, +due to an implementation detail of L 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 except sets the storage object and connection data B on C<$self>. You should probably be calling L to get a properly L Schema object instead. +If the accessor L returns a true value C<$checker>, +the following call will take place before return: +C<< L<$checker|/schema_sanity_checker>->L)|DBIx::Class::Schema::SanityChecker/perform_schema_sanity_checks> >> + =head3 Overloading Overload C to change the behaviour of C. =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 { diff --git a/lib/DBIx/Class/Schema/SanityChecker.pm b/lib/DBIx/Class/Schema/SanityChecker.pm new file mode 100644 index 0000000..c6b3e50 --- /dev/null +++ b/lib/DBIx/Class/Schema/SanityChecker.pm @@ -0,0 +1,590 @@ +package DBIx::Class::Schema::SanityChecker; + +use strict; +use warnings; + +use DBIx::Class::_Util qw( + dbic_internal_try refdesc uniq serialize + describe_class_methods emit_loud_diag +); +use DBIx::Class (); +use DBIx::Class::Exception (); +use Scalar::Util qw( blessed refaddr ); +use namespace::clean; + +=head1 NAME + +DBIx::Class::Schema::SanityChecker - Extensible "critic" for your Schema class hierarchy + +=head1 SYNOPSIS + + package MyApp::Schema; + use base 'DBIx::Class::Schema'; + + # this is the default on Perl v5.10 and later + __PACKAGE__->schema_sanity_checker('DBIx::Class::Schema::SanityChecker'); + ... + +=head1 DESCRIPTION + +This is the default implementation of the Schema and related classes +L. + +The validator is B on perls C and above. See +L for discussion of the runtime effects. + +Use of this class begins by invoking L +(usually via L), which in turn starts +invoking validators I> in the order listed in +L. For each set of returned errors (if any) +I> is called and the resulting strings are +passed to L, where final headers are prepended and the entire +thing is printed on C. + +The class does not provide a constructor, due to the lack of state to be +passed around: object orientation was chosen purely for the ease of +overriding parts of the chain of events as described above. The general +pattern of communicating errors between the individual methods (both +before and after formatting) is an arrayref of hash references. + +=head2 WHY + +DBIC existed for more than a decade without any such setup validation +fanciness, let alone something that is enabled by default (which in turn +L). The reason for this relatively +drastic change is a set of revamps within the metadata handling framework, +in order to resolve once and for all problems like +L, +L, etc. While +DBIC internals are now way more robust than they were before, this comes at +a price: some non-issues in code that has been working for a while, will +now become hard to explain, or if you are unlucky: B. + +Thus, in order to protect existing codebases to the fullest extent possible, +the executive decision (and substantial effort) was made to introduce this +on-by-default setup validation framework. A massive amount of work has been +invested ensuring that none of the builtin checks emit a false-positive: +each and every complaint made by these checks B. + +=head2 Performance considerations + +First of all - after your connection has been established - there is B whenever the checks are enabled. + +By default the checks are triggered every time +L is called. Thus there is a +noticeable startup slowdown, most notably during testing (each test is +effectively a standalone program connecting anew). As an example the test +execution phase of the L C distribution +suffers a consistent slowdown of about C<16%>. This is considered a relatively +small price to pay for the benefits provided. + +Nevertheless, there are valid cases for disabling the checks during +day-to-day development, and having them run only during CI builds. In fact +the test suite of DBIC does exactly this as can be seen in +F: + + ~/dbic_repo$ git show 39636786 | perl -ne "print if 16..61" + +Whatever you do, B: it is not +worth the risk. + +=head3 Perl5.8 + +The situation with perl interpreters before C is sadly more +complicated: due to lack of built-in L, the +mechanism used to interrogate various classes is +L<< B slower|https://github.com/dbsrgits/dbix-class/commit/296248c3 >>. +As a result the very same version of L +L takes a C> hit on its +test execution time (these numbers are observed with the speedups of +L available, without them the slowdown reaches the whopping +C<350%>). + +Therefore, on these versions of perl the sanity checks are B by +default. Instead a C placeholder value is inserted into the +L, +urging the user to decide for themselves how to proceed. + +It is the author's B recommendation to find a way to run the +checks on your codebase continuously, even if it takes much longer. Refer to +the last paragraph of L above for an example how +to do this during CI builds only. + +=head2 Validations provided by this module + +=head3 no_indirect_method_overrides + +There are many methods within DBIC which are +L<"strictly sugar"|DBIx::Class::MethodAttributes/DBIC_method_is_indirect_sugar> +and should never be overridden by your application (e.g. see warnings at the +end of L and L). +Starting with C DBIC is much more aggressive in calling the +underlying non-sugar methods directly, which in turn means that almost all +user-side overrides of sugar methods are never going to be invoked. These +situations are now reliably detected and reported individually (you may +end up with a lot of output on C due to this). + +Note: B reported by this check B<*MUST*> be resolved +before upgrading DBIC in production. Malfunctioning business logic and/or +B may result otherwise. + +=head3 valid_c3_composition + +Looks through everything returned by L, and +for any class that B already utilize L a +L is calculated and then +compared to the shadowing map as if C was requested in the first place. +Any discrepancies are reported in order to clearly identify L especially when +encountered within complex inheritance hierarchies. + +=head3 no_inheritance_crosscontamination + +Checks that every individual L, +L, L, +L +and L class does not inherit from +an unexpected DBIC base class: e.g. an error will be raised if your +C inherits from both C and +C. + +=head1 METHODS + +=head2 perform_schema_sanity_checks + +=over + +=item Arguments: L<$schema|DBIx::Class::Schema> + +=item Return Value: unspecified (ignored by caller) + +=back + +The entry point expected by the +L. See +L for details. + +=cut + +sub perform_schema_sanity_checks { + my ($self, $schema) = @_; + + local $DBIx::Class::_Util::describe_class_query_cache->{'!internal!'} = {} + if + # does not make a measurable difference on 5.10+ + DBIx::Class::_ENV_::OLD_MRO + and + # the callstack shouldn't really be recursive, but for completeness... + ! $DBIx::Class::_Util::describe_class_query_cache->{'!internal!'} + ; + + my (@errors_found, $schema_desc); + for my $ch ( @{ $self->available_checks } ) { + + my $err = $self->${\"check_$ch"} ( $schema ); + + push @errors_found, map + { + { + check_name => $ch, + formatted_error => $_, + schema_desc => ( $schema_desc ||= + ( length ref $schema ) + ? refdesc $schema + : "'$schema'" + ), + } + } + @{ + $self->${\"format_${ch}_errors"} ( $err ) + || + [] + } + if @$err; + } + + $self->emit_errors(\@errors_found) + if @errors_found; +} + +=head2 available_checks + +=over + +=item Arguments: none + +=item Return Value: \@list_of_check_names + +=back + +The list of checks L will perform on the +provided L<$schema|DBIx::Class::Schema> object. For every entry returned +by this method, there must be a pair of I> and +I> methods available. + +Override this method to add checks to the +L. + +=cut + +sub available_checks { [qw( + valid_c3_composition + no_inheritance_crosscontamination + no_indirect_method_overrides +)] } + +=head2 emit_errors + +=over + +=item Arguments: \@list_of_formatted_errors + +=item Return Value: unspecified (ignored by caller) + +=back + +Takes an array reference of individual errors returned by various +I> formatters, and outputs them on C. + +This method is the most convenient integration point for a 3rd party logging +framework. + +Each individual error is expected to be a hash reference with all values being +plain strings as follows: + + { + schema_desc => $human_readable_description_of_the_passed_in_schema + check_name => $name_of_the_check_as_listed_in_available_checks() + formatted_error => $error_text_as_returned_by_format_$checkname_errors() + } + +If the environment variable C is set to +a true value this method will throw an exception with the same text. Those who +prefer to take no chances could set this variable permanently as part of their +deployment scripts. + +=cut + +# *NOT* using carp_unique and the warn framework - make +# it harder to accidentaly silence problems via $SIG{__WARN__} +sub emit_errors { + #my ($self, $errs) = @_; + + my @final_error_texts = map { + sprintf( "Schema %s failed the '%s' sanity check: %s\n", + @{$_}{qw( schema_desc check_name formatted_error )} + ); + } @{$_[1]}; + + emit_loud_diag( + msg => $_ + ) for @final_error_texts; + + # Do not use the constant - but instead check the env every time + # This will allow people to start auditing their apps piecemeal + DBIx::Class::Exception->throw( join "\n", @final_error_texts, ' ' ) + if $ENV{DBIC_ASSERT_NO_FAILING_SANITY_CHECKS}; +} + +=head2 all_schema_related_classes + +=over + +=item Arguments: L<$schema|DBIx::Class::Schema> + +=item Return Value: @sorted_list_of_unique_class_names + +=back + +This is a convenience method providing a list (not an arrayref) of +"interesting classes" related to the supplied schema. The returned list +currently contains the following class names: + +=over + +=item * The L class itself + +=item * The associated L class if any + +=item * The classes of all L if any + +=item * All L classes for all registered ResultSource instances + +=item * All L classes for all registered ResultSource instances + +=back + +=cut + +sub all_schema_related_classes { + my ($self, $schema) = @_; + + sort( uniq( map { + ( not defined $_ ) ? () + : ( defined blessed $_ ) ? ref $_ + : $_ + } ( + $schema, + $schema->storage, + ( map { + $_, + $_->result_class, + $_->resultset_class, + } map { $schema->source($_) } $schema->sources ), + ))); +} + + +sub format_no_indirect_method_overrides_errors { + # my ($self, $errors) = @_; + + [ map { sprintf( + "Method(s) %s override the convenience shortcut %s::%s(): " + . 'it is almost certain these overrides *MAY BE COMPLETELY IGNORED* at ' + . 'runtime. You MUST reimplement each override to hook a method from the ' + . "chain of calls within the convenience shortcut as seen when running:\n " + . '~$ perl -M%2$s -MDevel::Dwarn -e "Ddie { %3$s => %2$s->can(q(%3$s)) }"', + join (', ', map { "$_()" } sort @{ $_->{by} } ), + $_->{overriden}{via_class}, + $_->{overriden}{name}, + )} @{ $_[1] } ] +} + +sub check_no_indirect_method_overrides { + my ($self, $schema) = @_; + + my( @err, $seen_shadowing_configurations ); + + METHOD_STACK: + for my $method_stack ( map { + values %{ describe_class_methods($_)->{methods_with_supers} || {} } + } $self->all_schema_related_classes($schema) ) { + + my $nonsugar_methods; + + for (@$method_stack) { + + push @$nonsugar_methods, $_ and next + unless $_->{attributes}{DBIC_method_is_indirect_sugar}; + + push @err, { + overriden => { + name => $_->{name}, + via_class => $_->{via_class} + }, + by => [ map { "$_->{via_class}::$_->{name}" } @$nonsugar_methods ], + } if ( + $nonsugar_methods + and + ! $seen_shadowing_configurations->{ + join "\0", + map + { refaddr $_ } + ( + $_, + @$nonsugar_methods, + ) + }++ + ) + ; + + next METHOD_STACK; + } + } + + \@err +} + + +sub format_valid_c3_composition_errors { + # my ($self, $errors) = @_; + + [ map { sprintf( + "Class '%s' %s using the '%s' MRO affecting the lookup order of the " + . "following method(s): %s. You MUST add the following line to '%1\$s' " + . "right after strict/warnings:\n use mro 'c3';", + $_->{class}, + ( ($_->{initial_mro} eq $_->{current_mro}) ? 'is' : 'was originally' ), + $_->{initial_mro}, + join (', ', map { "$_()" } sort keys %{$_->{affected_methods}} ), + )} @{ $_[1] } ] +} + + +my $base_ISA = { + map { $_ => 1 } @{mro::get_linear_isa("DBIx::Class")} +}; + +sub check_valid_c3_composition { + my ($self, $schema) = @_; + + my @err; + + # + # A *very* involved check, to absolutely minimize false positives + # If this check returns an issue - it *better be* a real one + # + for my $class ( $self->all_schema_related_classes($schema) ) { + + my $desc = do { + no strict 'refs'; + describe_class_methods({ + class => $class, + ( ${"${class}::__INITIAL_MRO_UPON_DBIC_LOAD__"} + ? ( use_mro => ${"${class}::__INITIAL_MRO_UPON_DBIC_LOAD__"} ) + : () + ), + }) + }; + + # is there anything to check? + next unless ( + ! $desc->{mro}{is_c3} + and + $desc->{methods_with_supers} + and + my @potentially_problematic_method_stacks = + grep + { + # at least 2 variants came via inheritance (not ours) + ( + (grep { $_->{via_class} ne $class } @$_) + > + 1 + ) + and + # + # last ditch effort to skip examining an alternative mro + # IFF the entire "foreign" stack is located in the "base isa" + # + # This allows for extra efficiency (as there are several + # with_supers methods that would always be there), but more + # importantly saves one from tripping on the nonsensical yet + # begrudgingly functional (as in - no adverse effects): + # + # use base 'DBIx::Class'; + # use base 'DBIx::Class::Schema'; + # + ( + grep { + # not ours + $_->{via_class} ne $class + and + # not from the base stack either + ! $base_ISA->{$_->{via_class}} + } @$_ + ) + } + values %{ $desc->{methods_with_supers} } + ); + + my $affected_methods; + + for my $stack (@potentially_problematic_method_stacks) { + + # If we got so far - we need to see what the class would look + # like under c3 and compare, sigh + # + # Note that if the hierarchy is *really* fucked (like the above + # double-base e.g.) then recalc under 'c3' WILL FAIL, hence the + # extra eval: if we fail we report things as "jumbled up" + # + $affected_methods->{$stack->[0]{name}} = [ + map { $_->{via_class} } @$stack + ] unless dbic_internal_try { + + serialize($stack) + eq + serialize( + describe_class_methods({ class => $class, use_mro => 'c3' }) + ->{methods} + ->{$stack->[0]{name}} + ) + }; + } + + push @err, { + class => $class, + isa => $desc->{isa}, + initial_mro => $desc->{mro}{type}, + current_mro => mro::get_mro($class), + affected_methods => $affected_methods, + } if $affected_methods; + } + + \@err; +} + + +sub format_no_inheritance_crosscontamination_errors { + # my ($self, $errors) = @_; + + [ map { sprintf( + "Class '%s' registered in the role of '%s' unexpectedly inherits '%s': " + . 'you must resolve this by either removing an erroneous `use base` call ' + . "or switching to Moo(se)-style delegation (i.e. the 'handles' keyword)", + $_->{class}, + $_->{type}, + $_->{unexpectedly_inherits}, + )} @{ $_[1] } ] +} + +sub check_no_inheritance_crosscontamination { + my ($self, $schema) = @_; + + my @err; + + my $to_check = { + Schema => [ $schema ], + Storage => [ $schema->storage ], + ResultSource => [ map { $schema->source($_) } $schema->sources ], + }; + + $to_check->{ResultSet} = [ + map { $_->resultset_class } @{$to_check->{ResultSource}} + ]; + + $to_check->{Core} = [ + map { $_->result_class } @{$to_check->{ResultSource}} + ]; + + # Reduce everything to a unique sorted list of class names + $_ = [ sort( uniq( map { + ( not defined $_ ) ? () + : ( defined blessed $_ ) ? ref $_ + : $_ + } @$_ ) ) ] for values %$to_check; + + for my $group ( sort keys %$to_check ) { + for my $class ( @{ $to_check->{$group} } ) { + for my $foreign_base ( + map { "DBIx::Class::$_" } sort grep { $_ ne $group } keys %$to_check + ) { + + push @err, { + class => $class, + type => ( $group eq 'Core' ? 'ResultClass' : $group ), + unexpectedly_inherits => $foreign_base + } if $class->isa($foreign_base); + } + } + } + + \@err; +} + +1; + +__END__ + +=head1 FURTHER QUESTIONS? + +Check the list of L. + +=head1 COPYRIGHT AND LICENSE + +This module is free software L +by the L. You can +redistribute it and/or modify it under the same terms as the +L. diff --git a/lib/DBIx/Class/Schema/Versioned.pm b/lib/DBIx/Class/Schema/Versioned.pm index 8101f2e..f84bd05 100644 --- a/lib/DBIx/Class/Schema/Versioned.pm +++ b/lib/DBIx/Class/Schema/Versioned.pm @@ -49,6 +49,13 @@ use base 'DBIx::Class::Schema'; use strict; use warnings; +# no point sanity checking, unless we are running asserts +__PACKAGE__->schema_sanity_checker( + DBIx::Class::_ENV_::ASSERT_NO_FAILING_SANITY_CHECKS + ? 'DBIx::Class::Schema::SanityChecker' + : '' +); + __PACKAGE__->register_class('Table', 'DBIx::Class::Version::Table'); package # Hide from PAUSE @@ -57,6 +64,13 @@ use base 'DBIx::Class::Schema'; use strict; use warnings; +# no point sanity checking, unless we are running asserts +__PACKAGE__->schema_sanity_checker( + DBIx::Class::_ENV_::ASSERT_NO_FAILING_SANITY_CHECKS + ? 'DBIx::Class::Schema::SanityChecker' + : '' +); + __PACKAGE__->register_class('TableCompat', 'DBIx::Class::Version::TableCompat'); diff --git a/lib/DBIx/Class/_Util.pm b/lib/DBIx/Class/_Util.pm index e94d98d..ac3a937 100644 --- a/lib/DBIx/Class/_Util.pm +++ b/lib/DBIx/Class/_Util.pm @@ -50,6 +50,7 @@ BEGIN { DBIC_ASSERT_NO_INTERNAL_WANTARRAY DBIC_ASSERT_NO_INTERNAL_INDIRECT_CALLS DBIC_ASSERT_NO_ERRONEOUS_METAINSTANCE_USE + DBIC_ASSERT_NO_FAILING_SANITY_CHECKS DBIC_STRESSTEST_UTF8_UPGRADE_GENERATED_COLLAPSER_SOURCE DBIC_STRESSTEST_COLUMN_INFO_UNAWARE_STORAGE ) diff --git a/t/cdbi/DeepAbstractSearch/01_search.t b/t/cdbi/DeepAbstractSearch/01_search.t index 8b2101a..5c87cb0 100644 --- a/t/cdbi/DeepAbstractSearch/01_search.t +++ b/t/cdbi/DeepAbstractSearch/01_search.t @@ -19,6 +19,24 @@ my @DSN = ("dbi:SQLite:dbname=$DB", '', '', { AutoCommit => 0 }); package Music::DBI; use base qw(DBIx::Class::CDBICompat); use Class::DBI::Plugin::DeepAbstractSearch; + +BEGIN { + # offset the warning from DBIx::Class::Schema on 5.8 + # keep the ::Schema default as-is otherwise + DBIx::Class::_ENV_::OLD_MRO + and + ( eval <<'EOS' or die $@ ); + + sub setup_schema_instance { + my $s = shift->next::method(@_); + $s->schema_sanity_checker(''); + $s; + } + + 1; +EOS +} + __PACKAGE__->connection(@DSN); my $sql = <<'SQL_END'; diff --git a/t/cdbi/testlib/DBIC/Test/SQLite.pm b/t/cdbi/testlib/DBIC/Test/SQLite.pm index 72aa0c1..87a17f2 100644 --- a/t/cdbi/testlib/DBIC/Test/SQLite.pm +++ b/t/cdbi/testlib/DBIC/Test/SQLite.pm @@ -43,6 +43,23 @@ use DBICTest; use base qw/DBIx::Class/; +BEGIN { + # offset the warning from DBIx::Class::Schema on 5.8 + # keep the ::Schema default as-is otherwise + DBIx::Class::_ENV_::OLD_MRO + and + ( eval <<'EOS' or die $@ ); + + sub setup_schema_instance { + my $s = shift->next::method(@_); + $s->schema_sanity_checker(''); + $s; + } + + 1; +EOS +} + __PACKAGE__->load_components(qw/CDBICompat Core DB/); my $DB = DBICTest->_sqlite_dbfilename; diff --git a/t/cdbi/testlib/MyBase.pm b/t/cdbi/testlib/MyBase.pm index 8cffd74..106b359 100644 --- a/t/cdbi/testlib/MyBase.pm +++ b/t/cdbi/testlib/MyBase.pm @@ -7,6 +7,23 @@ use strict; use DBI; use DBICTest; +BEGIN { + # offset the warning from DBIx::Class::Schema on 5.8 + # keep the ::Schema default as-is otherwise + DBIx::Class::_ENV_::OLD_MRO + and + ( eval <<'EOS' or die $@ ); + + sub setup_schema_instance { + my $s = shift->next::method(@_); + $s->schema_sanity_checker(''); + $s; + } + + 1; +EOS +} + use base qw(DBIx::Class::CDBICompat); my @connect = (@ENV{map { "DBICTEST_MYSQL_${_}" } qw/DSN USER PASS/}, { PrintError => 0}); diff --git a/t/lib/DBICTest/BaseSchema.pm b/t/lib/DBICTest/BaseSchema.pm index 6c293cc..3963678 100644 --- a/t/lib/DBICTest/BaseSchema.pm +++ b/t/lib/DBICTest/BaseSchema.pm @@ -11,8 +11,55 @@ use DBIx::Class::_Util qw( emit_loud_diag scope_guard set_subname get_subname ); use DBICTest::Util::LeakTracer qw(populate_weakregistry assert_empty_weakregistry); use DBICTest::Util qw( local_umask tmpdir await_flock dbg DEBUG_TEST_CONCURRENCY_LOCKS ); use Scalar::Util qw( refaddr weaken ); +use Devel::GlobalDestruction (); use namespace::clean; +# Unless we are running assertions there is no value in checking ourselves +# during regular tests - the CI will do it for us +# +if ( + DBIx::Class::_ENV_::ASSERT_NO_FAILING_SANITY_CHECKS + and + # full-blown 5.8 sanity-checking is waaaaaay too slow, even for CI + ( + ! DBIx::Class::_ENV_::OLD_MRO + or + # still run a couple test with this, even on 5.8 + $ENV{DBICTEST_OLD_MRO_SANITY_CHECK_ASSERTIONS} + ) +) { + + __PACKAGE__->schema_sanity_checker('DBIx::Class::Schema::SanityChecker'); + + # Repeat the check on going out of scope (will catch weird runtime tinkering) + # Add only in case we will be using it, as it slows tests down + eval <<'EOD' or die $@; + + sub DESTROY { + if ( + ! Devel::GlobalDestruction::in_global_destruction() + and + my $checker = $_[0]->schema_sanity_checker + ) { + $checker->perform_schema_sanity_checks($_[0]); + } + + # *NOT* using next::method here - it (currently) will confuse Class::C3 + # in some obscure cases ( 5.8 naturally ) + shift->SUPER::DESTROY(); + } + + 1; + +EOD + +} +else { + # otherwise just unset the default + __PACKAGE__->schema_sanity_checker(''); +} + + if( $ENV{DBICTEST_ASSERT_NO_SPURIOUS_EXCEPTION_ACTION} ) { my $ea = __PACKAGE__->exception_action( sub { diff --git a/t/storage/txn.t b/t/storage/txn.t index 9af0040..0edca6c 100644 --- a/t/storage/txn.t +++ b/t/storage/txn.t @@ -1,3 +1,6 @@ +# Test is sufficiently involved to *want* to run with "maximum paranoia" +BEGIN { $ENV{DBICTEST_OLD_MRO_SANITY_CHECK_ASSERTIONS} = 1 } + BEGIN { do "./t/lib/ANFANG.pm" or die ( $@ || $! ) } use strict; diff --git a/t/storage/txn_scope_guard.t b/t/storage/txn_scope_guard.t index 09efcd7..00d81a4 100644 --- a/t/storage/txn_scope_guard.t +++ b/t/storage/txn_scope_guard.t @@ -1,3 +1,6 @@ +# Test is sufficiently involved to *want* to run with "maximum paranoia" +BEGIN { $ENV{DBICTEST_OLD_MRO_SANITY_CHECK_ASSERTIONS} = 1 } + BEGIN { do "./t/lib/ANFANG.pm" or die ( $@ || $! ) } use strict; diff --git a/xt/dist/pod_coverage.t b/xt/dist/pod_coverage.t index e2389af..859f0e3 100644 --- a/xt/dist/pod_coverage.t +++ b/xt/dist/pod_coverage.t @@ -8,6 +8,7 @@ use Test::More; use Module::Runtime 'require_module'; use lib 'maint/.Generated_Pod/lib'; use DBICTest; +use DBIx::Class::Schema::SanityChecker; use namespace::clean; # this has already been required but leave it here for CPANTS static analysis @@ -102,6 +103,11 @@ my $exceptions = { connection /] }, + 'DBIx::Class::Schema::SanityChecker' => { + ignore => [ map { + qr/^ (?: check_${_} | format_${_}_errors ) $/x + } @{ DBIx::Class::Schema::SanityChecker->available_checks } ] + }, 'DBIx::Class::Admin' => { ignore => [ qw/ @@ -181,9 +187,10 @@ foreach my $module (@modules) { # build parms up from ignore list my $parms = {}; - $parms->{trustme} = - [ map { qr/^$_$/ } @{ $ex->{ignore} } ] - if exists($ex->{ignore}); + $parms->{trustme} = [ map + { ref $_ eq 'Regexp' ? $_ : qr/^\Q$_\E$/ } + @{ $ex->{ignore} } + ] if exists($ex->{ignore}); # run the test with the potentially modified parm set Test::Pod::Coverage::pod_coverage_ok($module, $parms, "$module POD coverage"); diff --git a/xt/extra/diagnostics/invalid_component_composition.t b/xt/extra/diagnostics/invalid_component_composition.t new file mode 100644 index 0000000..ac162d5 --- /dev/null +++ b/xt/extra/diagnostics/invalid_component_composition.t @@ -0,0 +1,48 @@ +BEGIN { do "./t/lib/ANFANG.pm" or die ( $@ || $! ) } + +BEGIN { delete $ENV{DBIC_ASSERT_NO_FAILING_SANITY_CHECKS} } + +use strict; +use warnings; + +use Test::More; + +use DBICTest::Util 'capture_stderr'; +use DBICTest; + + +{ + package DBICTest::Some::BaseResult; + use base "DBIx::Class::Core"; + + # order is important + __PACKAGE__->load_components(qw( FilterColumn InflateColumn::DateTime )); +} + +{ + package DBICTest::Some::Result; + use base "DBICTest::Some::BaseResult"; + + __PACKAGE__->table("sometable"); + + __PACKAGE__->add_columns( + somecolumn => { data_type => "datetime" }, + ); +} + +{ + package DBICTest::Some::Schema; + use base "DBIx::Class::Schema"; + __PACKAGE__->schema_sanity_checker("DBIx::Class::Schema::SanityChecker"); + __PACKAGE__->register_class( some_result => "DBICTest::Some::Result" ); +} + +like( + capture_stderr { + DBICTest::Some::Schema->connection(sub {} ); + }, + qr/Class 'DBICTest::Some::Result' was originally using the 'dfs' MRO affecting .+ register_column\(\)/, + 'Proper incorrect composition warning emitted on StdErr' +); + +done_testing; diff --git a/xt/extra/internals/ithread_stress.t b/xt/extra/internals/ithread_stress.t index 0b1602f..dc56d49 100644 --- a/xt/extra/internals/ithread_stress.t +++ b/xt/extra/internals/ithread_stress.t @@ -1,5 +1,8 @@ BEGIN { do "./t/lib/ANFANG.pm" or die ( $@ || $! ) } +# Test is sufficiently involved to *want* to run with "maximum paranoia" +BEGIN { $ENV{DBICTEST_OLD_MRO_SANITY_CHECK_ASSERTIONS} = 1 } + use warnings; use strict; diff --git a/xt/extra/lean_startup.t b/xt/extra/lean_startup.t index 435a5ba..2731f0c 100644 --- a/xt/extra/lean_startup.t +++ b/xt/extra/lean_startup.t @@ -105,6 +105,10 @@ BEGIN { DBICTEST_DEBUG_CONCURRENCY_LOCKS )}; + # ensures the checker won't be disabled in + # t/lib/DBICTest/BaseSchema.pm + $ENV{DBIC_ASSERT_NO_FAILING_SANITY_CHECKS} = 1; + $ENV{DBICTEST_ANFANG_DEFANG} = 1; # make sure extras do not load even when this is set