Join table aliasing doesn't work when database has a table prefix

[glamorous]

I have a model Model_Newsitem that worked before the latest commit that closes #93

This was a part of it:

'author' => new Field_BelongsTo(array(
     'foreign' => 'user',
)),

And I did a load_with in the end on author

With the latest commit, it breaks so I changed my code to this:

'author' => new Field_BelongsTo(array(
    'foreign' => 'user',
    'column' => 'user_id',
)),

What seems correct because I have 'user_id' as a column... Did I something wrong or is there something wrong in the code?

This is were it breaks:

Unknown column 'rcc__user:author.id'

Looks like the join is not correct:

LEFT JOIN `rcc_users` AS `_user:author` ON (`rcc_newsitems`.`user_id` = `rcc__user:author`.`id`)

Maybe this is important, in my model I have a

load_with(array('author','event'));

[glamorous]

With the latest commit rolled back I recieve this error wich is related to #93 I suppose:

I call 2 times this code:

public function find_by_category($category)
{
    return $this->join('categories_newsitems', 'INNER')
                ->on('newsitem.id', '=', 'categories_newsitems.newsitem_id')
                ->join('category', 'INNER')
                ->on('category.id', '=', 'categories_newsitems.category_id')
                ->where('category.uri', '=', $category);
}

The error:

Not unique table/alias

[banks]

Yep this looks like I missed something with the 'fix' for this. It is probably not a big deal just an issue with the particular format of the configuration.

The commit passed our unit tests although Jelly is so flexible that they are not really that comprehensive (yet!).

You say

With the latest commit, it breaks so I changed my code to this:

But you didn't say if that actually changed anything. Did that fix it?

I recieve this error wich is related to #93 I suppose:

Yes that will relate to the fix for #93 since the way to correctly select joined columns for with()'ed relationships has now changed. For joins you make yourself though, it is up to you how you alias and refer to them and fix for #93 shouldn't affect that.

[banks]

Hold on, just realised this is to do with the table prefix you have on your tables. I haven't tested this so the problem is that the join is being aliased correctly (nothing to do with configuration) but the aliases are then having a table prefix prepended to them. This is a bit of a pain. I need to work out at what level the Database class is putting the table prefixes on and why it happens to the fields but not the join alias.

[glamorous]

My change didn't do a think. It's still broken... like the code I showed...

How should I handle the custom join? Do you have a solution?

[banks]

How should I handle the custom join? Do you have a solution?

The code you posted should still work the same as before. The only change is that when you are referring to n:one relations (any you can use in with()) you now need to refer to the joined columns using relationship_alias.field rather than related_model.field. In your case, as I said, nothing should have changed.

[banks]

The error you are receiving seems to be related to something else. Can you post the actual query that gets run using your function?

[banks]

Hold on,

I call 2 times this code:

Do you mean you get the error when you call the function twice on the same object? In that case of course you will get an error, It is like writing SQL with two identical joins. If you really want to be able to call this as many times as you like without re-creating the join each time, you need to add code to store the state and only join the categories if they haven't been joined already.

Something like

if ( ! $this->has_joined_categories)
{
    // do join
}
// Add where to select category

[glamorous]

The query for the first (original issue) of second (first comment) problem?

EDIT: Indeed, 2 times on the same object, gonna write an extra method to do it with more "categories"

[banks]

Yeah when working with joins in query builder it will be quite a common problem to have to manually keep track of whether certain joins have been applied yet or not. In another similar system I came up with a whole set of functions for declaring builder methods as depending on other methods to have run but only running the dependent methods once. It was really just a glorified version of what I posted above and it is probably unnecessary in jelly core.

[glamorous]

Hmmz, looks not that simple to do a good join for multiple "categories"

EDIT:

public function find_by_categories($categories = array())
{
    $this->join('categories_events', 'INNER')
        ->on('event.id', '=', 'categories_events.event_id')
        ->join('category', 'INNER')
        ->on('category.id', '=', 'categories_events.category_id');

    foreach($categories as $category)
    {
        $this->where('category.uri', '=', $category);
    }

    return $this;
}

But as expected, this isn't resulting a thing because this can't return any results :s

[glamorous]

Like promised my query with the alias problem:

SELECT `rcc_newsitems`.`id` AS `id`, `rcc_newsitems`.`user_id` AS `author`,
`rcc_newsitems`.`event_id` AS `event`, ... ,  
`rcc__user:author`.`id` AS `:author:id`, `rcc__user:author`.`firstname` AS `:author:firstname`,
`rcc__user:author`.`familyname` AS `:author:familyname`, `rcc__user:author`.`team_id` AS `:author:team`, 
`rcc__event:event`.`id` AS `:event:id`, `rcc__event:event`.`uri` AS `:event:uri`, `rcc__event:event`.`name` AS `:event:name`, ... 
FROM `rcc_newsitems` 
LEFT JOIN `rcc_users` AS `_user:author` 
ON (`rcc_newsitems`.`user_id` = `rcc__user:author`.`id`) 
LEFT JOIN `rcc_events` AS `_event:event` 
ON (`rcc_newsitems`.`event_id` = `rcc__event:event`.`id`)

[banks]

Hmmz, looks not that simple to do a good join for multiple "categories"

Surely you would just need to use OR instead of AND:

public function find_by_categories($categories = array())
{
    $this->join('categories_events', 'INNER')
        ->on('event.id', '=', 'categories_events.event_id')
        ->join('category', 'INNER')
        ->on('category.id', '=', 'categories_events.category_id')
        ->where_open();

    foreach($categories as $category)
    {
        $this->or_where('category.uri', '=', $category);
    }

    return $this->where_close();
}

[glamorous]

Problem is that I want events listed who has both categories... So a OR isn't a solution I suppose

[banks]

Problem is that I want events listed who has both categories

OK, in that case you do need to make multiple joins but you will have to take responsibility of aliasing them differently and referring to the correct one in each where(). In your case, you don't have to be able to refer to the alias outside of the function so it would be easier than the solution I have for with(). Something like:

public function find_by_category($category)
{
    // Doesn't matter what the alias is since we only need to refer to it here
    $join_alias_1 = Text::random(10);
    $join_alias_2 = Text::random(10);

    return $this->join(array('categories_events', $join_alias_1), 'INNER')
        ->on('event.id', '=', $join_alias_1.'.event_id')
        ->join(array('category', $join_alias_2), 'INNER')
        ->on($join_alias_2.'.id', '=', $join_alias_1.'.category_id')
        ->where($join_alias_2.'.uri', '=', $category);
}

Note you will probably have the same issues with your table prefixes as in the issue above. This is actually a problem in Database class and I'm reporting it as a bug there.

[glamorous]

yes indeed, even with the old commit before #93

[glamorous]

Banks, can I partly fix this temporarily? I have seen this http://dev.kohanaphp.com/issues/2634 but was wondering myself how I could get the db-prefix from within the Jelly-builder?

[banks]

Banks, can I partly fix this temporarily?

You can probably find a work around but I can't see any way this can be properly resolved outside of the Database core. I've not looked in detail about how to implement a work around as I don't need it and I'd rather put effort into necessary fixes rather than temporary work-arounds! We'll see what happens to that bug report...

[banks]

I you find a (temporary) solution, please post it here so others cn benefit in the mean time.

[glamorous]

If you could tell me where I can get the db-prefix from within the builder-core from Jelly, I can post a fix I suppose ;-)

[banks]

If you could tell me where I can get the db-prefix from within the builder-core from Jelly

This is nothing to do with Jelly. The prefix is a property of the database object and you will need to look at how that is defined etc. Jelly has know knowledge of the database prefix as that is all abstracted by the database class. That is why it needs to be fixed in Database not in Jelly!

[glamorous]

I "fixed" it by adding some lines to the function "join" in Jelly_Builder_Core. My function is now:

public function join($table, $type = NULL)
{
    if (is_array($table))
    {
        $db = Database::instance();
        $table[0] = $this->_table($table[0]);
        $table[1] = $db->table_prefix().$table[1];
    }
    else
    {
        $table = $this->_table($table);
    }

    return parent::join($table, $type);
}

I added 2 lines... One for having an instance of the Database class and one 2 add the prefix to the table_name before calling the parent function.

[jonathangeiger]

Thanks for that solution glamorous. It doesn't really seem like something I'd be comfortable merging into core, but it's a useful workaround until this is fixed in Database.

I saw the issue posted for ORM, which was resolved, but has an issue been created to fix the problem in Database?

[glamorous]

Jup, banks did that, but no response yet... :(

[banks]

It doesn't really seem like something I'd be comfortable merging into core

Def not. This is a temporary 'work around' (i.e. hack) until the problem is fixed in Database module. If people need this they can implement this work around themselves until the issue is resolved. No idea when that will be...

[glamorous]

We all hope very soon :) Does anyone know if I can merge updates from Jelly in my "changed" local repo without loosing my changes? (GIT noob question)

[jonathangeiger]

Just give it a shot. Maybe just back it up before you go through with the merge. If there are conflicts you'll have to fix them manually. If there are more conflicts than you feel like dealing with just revert to the backup.

[banks]

One of us could look at providing a patch for Database to fix the issue - I guess it won't happen that quickly otherwise. I have a feeling it could be quite hard though and may not be accepted too quickly by the devs if they feel it is not neat.

[jonathangeiger]

Paul, I can throw up a patch in the next day or two. The solution you proposed seems reasonable enough and is essentially what the ORM patch is doing. The only problem is that the ORM's fix will break, so they'll have to revert that back.

The other obvious workaround is to just manually specify the table prefixes in the meta and turn off prefixing at the database level.

[jerfowler]

I think a better SQL query to the original problem would look more like this: SELECT rcc_newsitems. FROM rcc_newsitems WHERE EXISTS ( SELECT categories_newsitems.newsitem_id FROM categories_newsitems JOIN categories ON categories_newsitems.category_id = categories.id WHERE categories_newsitems.newsitem_id = rcc_newsitems.id AND categories.uri IN ('news', 'politics') GROUP BY categories_newsitems.newsitem_id HAVING COUNT() = 2 )

If I have time tomorrow, I'll write that as a Jelly_Builder function.

[jerfowler]

I was thinking about the current solution for joining related models and I think it should be changed so that all related model joins are aliased as the model field name. In fact, all tables names should be aliased. This means changing $table in the model meta class to an array.

Since all model field names are unique, you wouldn't have to worry about duplicate table joins. Also, when adding related model fields into the select list, they get prefixed with the field name so one could potentially load a model from a complex query with all joined related tables and not have to query the related models separately for their field data.

Perhaps an example is in order:

    SELECT 
        `user`.`id` AS `user->id`
    ,   `user`.`username` AS `user->username`
    , `user->profile`.`birthday` AS `user->profile->birthday`
    , `user->profile->home`.`address` AS `user->profile->home->address`
    , `user->profile->work`.`address` AS `user->profile->work->address`
    FROM `users` AS `user`
    JOIN `profiles` AS `user->profile` ON `user`.`id` = `user->profile`.`user_id`
    JOIN `addresses` AS `user->profile->home` ON `user->profile`.`home_id` = `user->profile->home`.`id`
    JOIN `addresses` AS `user->profile->work` ON `user->profile`.`work_id` = `user->profile->work`.`id`

So, What this does...

1 All originating tables are aliased as the model name (users to user) 2 All joined tables are aliased as the model related field (profiles to user->profile) 3 All returned columns are aliased as the full path to that field from the originating model 4 All tables and fields are unique and we can have multiple joins to the same table 5 When accessing related model fields, we can easily check the $_unmapped array for the value 6 Allows us to use SQL Views to store complex queries and map the results to a model hierarchy.

[banks]

Interesting idea Jeremy, we'll consider the implications of this. Jon, do you see potential problems with this proposal.

FWIW I think that Database should be fixed anyway as this issue affects more than just Jelly but the solution above has other adantages.

[jerfowler]

To shorten the column aliases up a bit, we could leave off the model-> and just start with the model's field name. The originating model would already be known. Aliases have, I believe, a 255 char max and so we would have to check for that and have a graceful way to deal with it. Granted, It would have to be a really deep relationship join or someone using extremely long field names... so the likelihood of that is slim.

[jonathangeiger]

This is a really good idea and also has the potential to cut down on some of the need for the aliasing logic that currently happens in the builder.

The only thing I'm not sure of is the '->' connector. If we thought it out properly, we could make it so that most of the where()s and such simply "fall through" without having to be aliased by Jelly_Builder, since they're aliased in the SELECT part.

[glamorous]

The initial issue may be closed because there is been an update in the Database Class. Maybe start a new issue as a feature request for the second discussion in this topic?

[banks]

Indeed although it should be noted by anyone having problems with this that they will need to use the 3.1.0 branch of Database to benefit from the fix (until 3.1.0 is released that is).

[glamorous]

or just use the latest github version like I always do :p