Ticket 105 saving habtm between different db-schemas - reworked and hopefully final version #92

Closed
wants to merge 6 commits into
from

Conversation

Projects
None yet
3 participants
Member

rchavik commented May 22, 2011

Hi,

This is a reworked of: #69.

I have also included @lorenzo's suggestion from rchavik/cakephp@4f12dde#commitcomment-393555

Notable changes on this changeset:

  • change of DboSource->fullTableName() by adding a new parameter $schema defaulting to true
  • addition of property $schemaName in Model. Although not exactly required to fix #105, i think having the schema name is handy.
  • CakeTestCase inspect $fixture->db and use it when it has been set. This is needed for testing for cross schema operations.

The following commits are not directly related to #105, detected when running tests

  • rchavik@935d17d is a fix for mysqli driver (virtualField tests was failing)
  • rchavik@80c64d8 is a fix for postsql driver (adding IF EXISTS in DROP statements)

I have updated the Dboes as best as I can. I have tested model_cross_schema_habtm.test.php with mysql, mysqli, and postgres with all test passes. For mysql, mysqli, all model, behavior, and database group tests passes (with the exception on mysqli in testArrayConditionsParsing (dbo_source.test.php:2552 and 2556).

On my env, running postgres tests is very slow, so I only run minimal tests on this driver.

Thanks.

Member

AD7six commented May 23, 2011

I've only skimmed the code changes, but it seems the scope of what you're proposing here is greater than the problem described in ticket #105.

The original pull request touched 2 files (excluding tests) now you modify 10. Is is really necessary to change so many files?

Also, why are there changes specific to postgres for a ticket which does not mention this datasource?

Member

rchavik commented May 23, 2011

rchavik@bfeffbf modifies DboSource->fullTableName() method and adds a new property Model->schemaName.
Since databases may have different notion on what a schema is, I add a 'virtual' function getSchemaName(), and the dbo files were updated accordingly. That's the reason the number of modified files are more than the original pull request.

For mysql driver, it uses $config['database'], oracle uses $config['login'], while postgres uses $config['schema'], etc.

rchavik@12ba059 adapted the use of fullTableName() in dbos inline with the changes introduced in rchavik@bfeffbf.

rchavik@c846022 contains the fix for 105 with dependency of the previous two commits.

This postgres commit rchavik@80c64d8 does not directly related to ticket #105. I encounter this when testing my changes against the three drivers that I have access to.
The same with rchavik@935d17d when I encountered failed test for the virtualFields.
Should I exclude or split these two commits into a separate branch?

Member

AD7six commented May 23, 2011

Adding a new property is not somthing to do lightly and most likely cannot be accepted for a bug fix release.

Mixing things together like that makes it very difficult to integrate your changes. Whereas originally there was one fix for one problem, there are now 3 (related or not, they are 3 different logical changes) attached to a ticket for one problem. The issue there is if any one of your logical changes isn't accepted - that prevents the whole pull request from being used as submitted.

It would make it easier to have 3 distinct tickets + pull requests for the 3 issues you've found; Especially since 2 of them are postgres specific, don't have a failing test case of their own, and also do not affect most users for the 'real' problem #105.

As I previously mentioned, I don't know why mysql users drop if table exists - to some extent this seems like hiding a problem (where cake thinks a table exists which does not, due to a stale cache problem).

In any event I'll leave to Jose (or discuss with him) to see what he says, and thanks for your effort :)

Member

rchavik commented May 23, 2011

AD7six,

Thanks for the insights. I'll keep that in mind for my next pull request.

Member

rchavik commented May 24, 2011

As per @AD7six suggestion, I have removed the unrelated commits, and also Model->schemaName property. DboSource->fullTableName() now calls getSchemaName() from itself or uses the model's datasource.

The DboSource->getSchemaName is a new public function. So this still might not be considered 'bugfix only' patch.

The only other way to achieve portability across dbs without adding a new function is to have an ugly switch inside fullTableName() and use the appropriate value as the schema, eg:

if (is_object($model)) {
    $db = $model->getDataSource();
    switch ($db->config['driver']) {
    case 'postgres':
        $schemaName = $db->config['schema']; break;
    case 'oracle':
        $schemaName = $db->config['login']; break;
    default:
        $schemaName = $db->config['database']; break;
    }
    $table = $model->tablePrefix . $model->table;
}

With the ugly switch approach, we don't need to modify the dbo files and still achieve the same functionality. But it feels dirty.

Owner

markstory commented May 25, 2011

Thanks for taking the time to look at this issue :)

I haven't looked very hard at this one, but wouldn't just using the 'with' model's datasource solve most of the issues? Cross database testing is a bit of a mess right now, and something we should definitely fix, but I don't know if that needs to get rolled in with this.

Member

rchavik commented May 25, 2011

Yes, specifying 'with' is required for this patch to work properly because it will use the model's datasource when performing DML operations.

However, cross database joins cannot correctly be executed. Consider the following:

class User extends AppModel {
    var $useDbConfig = 'sandbox';
    var $hasAndBelongsToMany => array(
        'Group' => array(
            'className' => 'UsersGroup',
            'unique' => true,
            'with' => 'UsersGroup',
        ));
class UsersGroup extends AppModel {
    var $useDbConfig = 'sandbox2';
}

UsersGroup could be a plugin table that is on a different schema (a separate database in mysql).
When editing user, Model::find() will generate the following invalid SQL:

SQL Error: 1146: Table 'sandbox.users_groups' doesn't exist [CORE/cake/libs/model/datasources/dbo_source.php, line 684]
SELECT `Group`.`id`, `Group`.`name`, `UsersGroup`.`id`, `UsersGroup`.`user_id`, `UsersGroup`.`group_id`, `UsersGroup`.`approved` FROM `groups` AS `Group` JOIN `users_groups` AS `UsersGroup` ON (`UsersGroup`.`user_id` = 1 AND `UsersGroup`.`group_id` = `Group`.`id`)

This is because fullTableName() in DboSource::generateAssociationQuery() (dbo_source.php:1245) passes in incomplete table name (without the schema name) to DboSource::buildStatement() -> DboSource::buildJoinStatement() -> DboSource::renderJoinStatement. Hence the query assumes that the table resides in the same schema as User model.

For your second concern: Commit rchavik/cakephp@35e7cba is an attempt to make cross schema testing easier. But it would be nice if we can specify which datasource to use in CakeTestFixture.

In test model_cross_schema_habtm.test.php, i use two datasources and manualy drops and recreate the tables in the correct schema, assign the second database to the Fixture and then loads the fixture data.

Without this, the test won't pass and writing proper test for cross database with existing code would be difficult

Member

rchavik commented Oct 20, 2011

Obsoleted by #262

rchavik closed this Oct 20, 2011

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