jonathangeiger / kohana-jelly

See the link below for the most up-to-date code
https://github.com/creatoro/jelly
MIT License
146 stars 34 forks source link

table name specified more than once error #93

Closed chrisgo closed 14 years ago

chrisgo commented 14 years ago

The SQL statement generated from a model referencing the same model twice results in an SQL statement being executed that returns

* table name specified more than once error *

Per banks: this is a bug in belongsTo field it should alias the join with the field alias rather than just the table name.

More info/code here: http://forum.kohanaphp.com/comments.php?DiscussionID=5283

banks commented 14 years ago

While we're at it we should fix the $model var to be $builder and the API docs in BelongsTo::with() and HasOne::with().

banks commented 14 years ago

The difficulty here is aliasing the selects. Currently, Jelly_Builder::with() uses $this->select(array($model.'.'.$alias, $chain.':'.$alias));.

Obviously the model.aliased bit gets aliased to table.column. The problem is, we need to find a way to get it to alias to table_alias.column.

Aliasing the join is trivial - just use something like join(array($this->foreign['model'], $this-name), ...). The question is, how do you get the model in the select() above to alias to the field alias rather than the model table in this case.

I guess we will need to add some sort of internal-only alias syntax to specify that the model should be aliased to something else. Perhaps it would work to have a convention that model names starting with an underscore should be left alone while aliasing and then we could simply alias joined tables to _field_alias. Need to investigate whether underscored table identifiers are allowed in most DBMSs. I'll look into this.

chrisgo commented 14 years ago

I can confirm the underscored table identifiers work in MySQL and PostgreSQL

select _users_alias.* from users as _users_alias;
banks commented 14 years ago

OK, I guess this is the way to go.

banks commented 14 years ago

Hmm, this is even more complex. If I use the alias for the join through out, it causes problems trying to resolve _join_alias.:primary_key since it doesn't know which model to look at!

I'll have to rethink this otherwise it will become a big messy hack.

banks commented 14 years ago

I can get around that by resolving meta aliases in the field with() column before creating the join. Now we have the problem that none of the fields get aliased properly since their 'model' is not actually a model - this works when the field alias and column are the same but clearly not when they are different.

This is quickly resembling a hack. What is really needed is a join alias syntax that can still be aliased as if it was an actual model name... So the alias() method converts the model aliases to the correct field but looking at an aliased table. I'll see what I can come up with. Any suggestions welcome.

banks commented 14 years ago

Hmm. Solved that but the problem goes even deeper...

Now the user needs to select thing based on the join_alias. So this example from our unit tests now breaks:

    $count = Jelly::select('post')
        // Where condition includes a column from joined table
        // this will cause a SQL error if load_with hasn't been taken into account
        ->where('author.name', '=', 'Jonathan Geiger')
        ->count();

Because author.name gets aliased to authors.name but the joined table is now joined as _author:author. So either we must require that users manually specify join aliases when using query builder and with() joined columns, or we will have to go in at the meta level and keep track of join context throughout query builder so we know that author.name should alias to authors.name in a query on authors but should alias to _author:relationship_alias.name when used in a query where it is a joined table...

banks commented 14 years ago

In fact it's even worse than that. Even if we check the context, the user fundamentally can't refer to joined columns sensibly.

Take @chrosgo's example. He is joining to his User model twice, one user is the 'to_user' the other is 'from_user'. In the example above, if he wanted to use one of the joined fields in the query explicitly it would be best to be able to do something like something like:

Jelly::select('shipment')->where('from_user.status', '=', 'active')->execute();

NOw the problem here is that 'from_user' is a field alias and yet we must treat it like it's a model/table name just to be unambiguous.

In fact, allowing this syntax to work may make the whole thing simpler as this is the fundamental problem with aliased relationships to the same model - the field alias effectively becomes a model alias.

The problem with the syntax above is how you would go about disambiguating the fields used as models from actual models, and, how would you know which model's the fields came from?

The only possible answer comes back to making Jelly::alias() context aware. So if you are querying the shipment model, Jelly::alias() would first check to see if the model passed is actually a valid, withable field alias and if it is it would use the field's join alias rather than the actual table name. Since you only attempt to alias field aliases to models when you know the query context, there shouldn't be problems with false positives.

If you did this without regard to context. It would be impossible to know reliably whether author.name reffered to table.column, model.field or relationship_alias.field. And the ambiguity between the last two would inevitably result in unpredictable behaviour.

Passing context to alias() though is not an insignificant internal API change. I'll see if it is a viable solution but I guess we need to think about the wider ramifications of this - it significantly complicates the concept of aliases which many people were struggling with already!

banks commented 14 years ago

Actually this mightnot be so bad as you should be able to keep the context stuff inside Jelly_Builder::_column() where we do other special casing anyway.

banks commented 14 years ago

Allow with() to work when multiple relationships relate to the same model. Closed by 60ef40c1003fc34799519a2a2ccb7681e24d9e29.

banks commented 14 years ago

Note that the commit above subtly changes the API.

Now fields joined using with() are correctly referred to using their relationship names. So in the example posted by chrisgo, you could use the following syntax:

Jelly::select('shipment')->where('to_user.status', '=', 'active')->execute();

This actually makes more sense I think but needs to be documented.

chrisgo commented 14 years ago

Thanks much guys!

banks commented 14 years ago

This fix doesn't (yet) work where using a Database table prefix. See #94.

chrisgo commented 14 years ago

I also don't have a table_prefix so my "simplest" case works (as described) -- it's doing it across multiple joins now (not just once). I could help you test outside the unit tests for these specific things if you would like. I still have to test the where() stuff

jonathangeiger commented 14 years ago

Beautiful work, banks. Thanks so much.