Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixes a bug with the _dequalify_idents piece that stripped out schema. before a table name #18

Closed
wants to merge 1 commit into from

Conversation

klkane
Copy link
Contributor

@klkane klkane commented Mar 7, 2013

for certain delete statements the dequalification process would result in the schema. portion of a table name to be stripped out. This solution simply is to make the dequalifier aware of the table name so if its looking at the table name it will do nothing instead of blindly stripping off everything before a .

…t schema.table if that is how the table is defined
@ribasushi
Copy link
Collaborator

If this patch is needed in the first place (I am not sure about this - see further) - it will not work for a case of a schema 'foo', a table 'foo' and a column 'foo'.

In any case - this patch should not be necessary at all, since the table name is never passed in while a dequalifier is activated, courtesy of fd8076c. Please add a an extra test around https://github.com/dbsrgits/dbix-class/blob/master/t/resultset/update_delete.t#L266 demonstrating the problem you are actually trying to solve, because as far as I can tell everything has already been fixed...

@klkane
Copy link
Contributor Author

klkane commented Mar 7, 2013

The only time this little patch fails is if the alias.column_name is equal to schema.table_name. Now that is a very ugly scenario that should be avoided simply for the sake of clarity, but I see your point that it potentially could come up.

The path to reproduce the problem is this:

my $rs = $schema->resultset('Artist')->find( 1 );
eval { $rs->cds_unordered->delete };

In the CD.pm I changed the table from 'cd' to 'schema_qualified.cd'.

The SQL that will be generated will be 'DELETE FROM cd WHERE ( artist = ? )' even though you have specified schema_qualified.cd as the table name in the result class.

@ribasushi
Copy link
Collaborator

Leaving aside the discussion whether DBIC should disallow "ugly" use cases or not - I think you are running the test on an old version of DBIC or something like this.

Bottom line is - no fix can go in without a test case, for 2 reasons.

  1. There is no other definitive way to verify that the patch indeed does what it is supposed to do (i.e. prove -l test.t should pass prove test.t should fail)
  2. There is no other definitive way to ensure a different edit of the code in question later on will not reintroduce the same breakage

I already did the tricky part for you in bd35f5fa3. Please modify this particular test https://github.com/dbsrgits/dbix-class/blob/master/t/resultset/update_delete.t#L326 (lifted from your comment above) so it actually fails without your patch. Then we can talk about the correct way of fixing this.

@ribasushi
Copy link
Collaborator

Just checking if you had a chance to look further into why my test doesn't fail...

@klkane
Copy link
Contributor Author

klkane commented Mar 11, 2013

Got a little sidetracked, working on it currently.

On Mon, Mar 11, 2013 at 11:10 AM, Peter Rabbitson
notifications@github.comwrote:

Just checking if you had a chance to look further into why my test doesn't
fail...


Reply to this email directly or view it on GitHubhttps://github.com//pull/18#issuecomment-14718480
.

Kevin L. Kane
kevin.kane at gmail.com

@klkane
Copy link
Contributor Author

klkane commented Mar 11, 2013

Your test doesn't fail because it works like it should, I had a messed environment that had a couple different versions of DBIx::Class installed. In other words the problem existed between screen and chair.

@klkane klkane closed this Mar 11, 2013
@ribasushi
Copy link
Collaborator

No problem. The side effect are improved tests, so this wasn't a waste of time by any means.

Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants