Skip to content

We are dumping our ResultSets and not the underlied database. Use schema->clone instead of _schema_from_database#102

Open
KES777 wants to merge 5 commits intojjn1056:masterfrom
KES777:fix_dataset_dump
Open

We are dumping our ResultSets and not the underlied database. Use schema->clone instead of _schema_from_database#102
KES777 wants to merge 5 commits intojjn1056:masterfrom
KES777:fix_dataset_dump

Conversation

@KES777
Copy link

@KES777 KES777 commented Jul 18, 2018

FIXES #101

@mohawk2
Copy link
Collaborator

mohawk2 commented Sep 19, 2018

Hi, thanks for this. Could you also add tests to a) stop this error happening again; and b) prove in code terms what problem it's solving?

@KES777
Copy link
Author

KES777 commented Sep 19, 2018

This is more as example. We need to fix other places too. Also I do not understand problem what is described by jnap

@mohawk2
Copy link
Collaborator

mohawk2 commented Sep 19, 2018

A test would provide a real example, rather than just what we have now, a sort of description. I'm afraid I can't accept code changes, even small ones like this, without tests.

@KES777
Copy link
Author

KES777 commented Sep 19, 2018

Ok. I will provide it. Would be cool if @jjn1056 provide more description on underlaid problem about why we can not clone current $schema

@mohawk2
Copy link
Collaborator

mohawk2 commented Feb 24, 2019

@KES777 Still waiting for tests?

@KES777
Copy link
Author

KES777 commented Feb 25, 2019

I do not wait tests, just patch local copy of code and use it.

I do not write tests for patched code because I do not know: is this behavior expected and correct.

@mohawk2
Copy link
Collaborator

mohawk2 commented Feb 25, 2019

Sorry, I was unclear. I am still waiting for you to add tests. Without those, I will not be merging this PR, and will in fact close it in a while. Please provide tests :-)

@KES777
Copy link
Author

KES777 commented Feb 25, 2019

I will not be able to add tests in the foreseeable future. sorry.

@KES777 KES777 closed this Feb 25, 2019
@KES777
Copy link
Author

KES777 commented Apr 6, 2025

Hi, @mohawk2. I found the way to reproduce this easily:

t/lib/Local/Schema/Result/ArtistCd.pm
@@ -1,7 +1,7 @@
 package Local::Schema::Result::ArtistCd;
 use base qw/DBIx::Class::Core/;
 
-__PACKAGE__->table('artist_cd');
+__PACKAGE__->table('artist_cd_not_match');
 __PACKAGE__->add_columns(
   artist_fk => {
     data_type => 'integer',

Last time I go through all the guts, but the reason was right in front of us.

@KES777 KES777 reopened this Apr 6, 2025
@KES777
Copy link
Author

KES777 commented Apr 6, 2025

A bit of debug information. From the below we can see that _schema_from_database returns ArtistCdNotMatch class instead of defined ResultSet ArtistCd.

Also I noticed that there is no ViewTest, which means that we will not be able to dump from there.

This happens because ResultSet could have different name in compare to the name of underlying table.

DBG>l 450

/home/kes/work/projects/github-forks/DBIx-Class-Migration/lib/DBIx/Class/Migration.pm
   x440:       my $fixture_conf_dir = catfile($fixture->config_dir,updir,$set);
   x441:       mkpath($fixture_conf_dir)
    442:         unless -d $fixture_conf_dir;
   x443:       return $fixture_conf_dir;
    444:     },
   x445:   });
    446: }
    447:
    448: sub dump_all_sets {
   x449:   (my $self = shift)->dbic_dh->version_storage_is_installed
    450:     || print "Target DB is not versioned.  Dump may not be reliable.\n";
    451:
   x452:   my $schema = $self->_schema_from_database;
   x453:   DB::x;
    454:   # my $schema = $self->schema->clone;
    455:
    456:   $self->build_dbic_fixtures->dump_all_config_sets({
    457:     schema => $schema,
    458:     directory_template => sub {
   x459:       my ($fixture, $params, $set) = @_;
   x460:       $set =~s/\.json//;
   x461:       my $fixture_conf_dir = catfile($fixture->config_dir,updir,$set);
   x462:       mkpath($fixture_conf_dir)
    463:         unless -d $fixture_conf_dir;
   x464:       return $fixture_conf_dir;
    465:     },
  >>466:   });
    467: }
    468:
    469: sub delete_named_sets {
   x470:   my ($self, @sets) = @_;


DBG> $schema->source_registrations;
{
  Artist => DBIx::Class::ResultSource::Table IGNORED,
  ArtistCdNotMatch => DBIx::Class::ResultSource::Table IGNORED,
  Cd => DBIx::Class::ResultSource::Table IGNORED,
  Country => DBIx::Class::ResultSource::Table IGNORED,
  DbixClassDeploymenthandlerVersion => DBIx::Class::ResultSource::Table IGNORED,
  Track => DBIx::Class::ResultSource::Table IGNORED,
}

DBG>$self->schema->clone->source_registrations
{
  Artist => DBIx::Class::ResultSource::Table IGNORED,
  ArtistCd => DBIx::Class::ResultSource::Table IGNORED,
  Cd => DBIx::Class::ResultSource::Table IGNORED,
  Country => DBIx::Class::ResultSource::Table IGNORED,
  Track => DBIx::Class::ResultSource::Table IGNORED,
  ViewTest => DBIx::Class::ResultSource::View {
    _columns => {
      artist_id => {
        data_type => integer,
      },
      artist_name => {
        data_type => varchar,
        size => 96,
      },
      cd_id => {
        data_type => integer,
      },
      cd_title => {
        data_type => varchar,
        size => 96,
      },
    },
    _columns_info_loaded => 0,
    _ordered_columns => [
      artist_id,
      artist_name,
      cd_id,
      cd_title,
    ],
    _relationships => {},
    deploy_depends_on => {},
    is_virtual => 1,
    name => viewtest,
    result_class => Local::Schema::Result::ViewTest,
    resultset_attributes => {},
    resultset_class => DBIx::Class::ResultSet,
    schema => IGNORED,
    source_name => ViewTest,
    view_definition => 
  SELECT
    a.id        as artist_id,
    a.name      as artist_name,
    cd.id       as cd_id,
    cd.title    as cd_title,
  FROM artist a
    INNER JOIN artist_cd as acd ON a.id = acd.artist_fk
    INNER JOIN cd ON acd.cd_fk = cd.id
  ORDER BY a.name, cd.title
,
  },
  __VERSION => DBIx::Class::ResultSource::Table IGNORED,
}

@KES777 KES777 force-pushed the fix_dataset_dump branch from 88a1f83 to 3496f45 Compare April 6, 2025 21:15
@KES777 KES777 changed the title We dumping our result sets not underlied database We are dumping our ResultSets and not the underlied database. Use schema->clone instead of _schema_from_database Apr 7, 2025
@jjn1056
Copy link
Owner

jjn1056 commented Aug 26, 2025

@KES777 I appreciate the work you've put into this, but to be honest I'm having a tough time following the reasoning. You've pinged me on IRC a few times and I guess we're in wildly different timezones so it's been tough to talk it out.

One thing that I can think of is if you could give me access to a repo that has the problem you are describing, along with the commands you run to make the problem happen, maybe that will help me figure out your examples. Sadly even though I wrote this I never got to use it on the job and I don't honestly recall all the reasons for the choices I made here, most of which was about 15 years ago :). Let me know if that can work for you.

@KES777
Copy link
Author

KES777 commented Aug 30, 2025

@jjn1056 Thank you.

You do not need access to any repo. Everything required to reproduce the issue is inside this PR.

Let me provide more context on the problem.

So most of the time ::Result modules have the same name as underlie table. Eg.

package Local::Schema::Result::ArtistCd;
...
__PACKAGE__->table('artist_cd');

Though this is true in most cases it does not cover all of them.

Just change artist_cd table name to a different one. As I did it at this PR at commit and tests will fail.

Another example is here:

package App::Schema::Result::Permission;
...
$T->table( 'roles_models' ); # <<<<<<< Actual table name is different.

You can see that ::Result module is named as ::Permission, but underlie table is named as roles_models. Which causes Can't find source for Permission at inline delegation error when a user tries to dump all datasets:

$(which dbic-migration) --schema_class App::Schema --database PostgreSQL -Ilib dump_all_sets

Or t/ugrade-downgrade-sqlite.t:86:

  $migration->dump_all_sets;

Why this error happens

Inside your module $schema have two mappings:
One mapping is build on PerlSources. The second mapping is build on DataBase structure.

So here:

    DBIx-Class-Migration/lib/DBIx/Class/Migration.pm
    456:   $self->build_dbic_fixtures->dump_all_config_sets({
    457:     schema => $schema,

$schema has these values:
1.

DBG> $schema->source_registrations;
{
  ArtistCdNotMatch => DBIx::Class::ResultSource::Table IGNORED,
  ...
}
DBG>$self->schema->clone->source_registrations
{
  ArtistCd => DBIx::Class::ResultSource::Table IGNORED,
  ...
}

You can see that ArtistCdNotMatch does not correspond to ArtistCd.

Details are here

Thoughts

It is useful to use $schema->source_registrations; when we want to create ::Result classes for a first time from a database. But once these perl modules were generated the $self->schema->clone->source_registrations should be used, because all the code communicates with that table as it was called in ::Result package: ArtistCd instead of artist_cd_not_match.

Or Permission instead of roles_models as it was first discovered here

@KES777
Copy link
Author

KES777 commented Sep 2, 2025

What is your discord and TZ? Let's find a better time for a live chat.

Eugen Konkov and others added 3 commits September 5, 2025 21:30
…056#66

Quote char was requested at jjn1056#66 (comment)

The better way to set quote char by default would be to pass quote_names as schema_args.
	# https://metacpan.org/pod/DBIx::Class::Storage::DBI#quote_names
But this is not easy, because connection info/schema_args could be generated by migration
adapters in many different ways: https://metacpan.org/pod/DBIx::Class::Storage::DBI#cursor_class
Thus we leverage STORAGE->sql_quote_char function similar to as it was used by ::Storate::DBI
	https://metacpan.org/dist/DBIx-Class/source/lib/DBIx/Class/Storage/DBI.pm#L1006
@KES777
Copy link
Author

KES777 commented Nov 9, 2025

@jjn1056 Have you managed to reproduce the issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Can't find source for Permission

3 participants