Fix table name source with Join by samsouthardjr · Pull Request #909 · atk4/data

samsouthardjr · 2021-10-22T16:33:44Z

No description provided.

… generating [where] or [having] portions of queries that referenced the alias instead of the de-aliased table.

mvorisek · 2021-10-22T17:03:00Z

Code to reproduce:

<?php

use \Atk4\Data\Model;

require 'vendor/autoload.php';

class Par extends Model
{
    public $table = 'parent';
    
    protected function init(): void 
    {
        parent::init();
        $this->addField('name');
        $this->hasMany('children', ['model' => [Child::class]]);
    }
}

class Child extends Model
{
    public $table = 'child';
    
    protected function init(): void
    {
        parent::init();
        $j = $this->join('jointable');
        $j->hasOne('parent_id', ['model' => [Par::class]]);
    }
}


$a = new App();

$p = new Par($a->db);
$e = $p->createEntity();
$e->save(['name' => 'foo']);
$e2 = $e->ref('children')->createEntity();
$e2->save();
$e->ref('children')->action('delete')->execute();

samsouthardjr · 2021-10-22T17:05:26Z

Code to reproduce:

Due to the join, the execute example is bad -- it will still produce an error. A better example is:

foreach ($e->ref('children') as $c) {
    $c->delete();
}

mvorisek · 2021-10-22T17:07:26Z

src/Persistence/Sql/Query.php


    protected function _render_table_noalias(): ?string
    {
+        $this->deAliasMainTable = true;


this relies on a fact that the _render_table_noalias is called first which is very bad assumption as it must not be always true like when WITH clause is used

IMHO we should check why some clauses use non-aliased table name in general

Would a better version be to do something like changing a template to:
'[noalias][with]update [table_noalias] set [set] [where]' (and perhaps if [noalias] was at the beginning, [table_noalias] would be redundant)? With that syntax, we could just have a _render_noalias function that did nothing but set deAliasMaintable to true.

samsouthardjr · 2021-10-22T17:13:59Z

Probably true – I’m a relatively new SQL person. The alternative would seem to be something like having [noalias] in the initial part of the template definition, which resulted in no aliases used in the entire query. However, I’m not expert enough to know if that would break something else (i.e., is it necessary to use aliases in deletes in sub queries, perhaps?). However, the code example to repeat the bug does seem to demonstrate something seriously wrong. It seems to be impossible to use ATK4 to delete a record if a join is involved. That seems fairly serious.

src/Persistence/Sql/Query.php

mvorisek · 2021-11-10T14:35:53Z

How is the code supposed to work when there is no jointable defined?

here is a testable demo https://github.com/atk4/data/compare/fix_upd_join_w_alias#diff-92f78239a1e95b6ddebce64dec3b75d71cfa81ee4073bb544f9f8f491a37218cR736

please make it passing, eg. add extra 1:1 model /w jointable

samsouthardjr · 2021-11-10T15:03:53Z

I’m not sure what you mean. That code was a demonstration of the problem when a jointable is defined – not defining a jointable would completely miss the point. Also, the “$deleteAction = $e->ref('children')->action('delete');” code at line 709/710 won’t work properly because of the join. I just had it to force it to generate a SQL Query dump that I could see was causing the problem. The correct code for a test would be foreach ($e->ref(‘children’) as $child) $child->delete();

…

-- Sam From: Michael Voříšek ***@***.***> Sent: Wednesday, November 10, 2021 6:36 AM To: atk4/data ***@***.***> Cc: samsouthardjr ***@***.***>; Author ***@***.***> Subject: Re: [atk4/data] Fix table name source with Join (PR #909) How it the code supposed to work when there is no jointable defined? https://github.com/atk4/data/compare/fix_upd_join_w_alias#diff-92f78239a1e95b6ddebce64dec3b75d71cfa81ee4073bb544f9f8f491a37218cR735 — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#909 (comment)> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/AUECRVKMYGFHQSERXOPMIHLULJ7NLANCNFSM5GQ6CYMQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub> . <https://github.com/notifications/beacon/AUECRVMCNK5WPJVBQFYKBNLULJ7NLA5CNFSM5GQ6CYM2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOHGERLWY.gif>

mvorisek · 2021-11-10T15:07:39Z

these two lines from your example: https://github.com/atk4/data/compare/fix_upd_join_w_alias#diff-92f78239a1e95b6ddebce64dec3b75d71cfa81ee4073bb544f9f8f491a37218cR700-R701

create only parent and child table names only, add 3rd model with jointable

you can fork the branch from my experiment and you can see the actual queries executed - https://github.com/atk4/data/runs/4166653222?check_suite_focus=true

samsouthardjr · 2021-11-10T15:39:26Z

Oh, I think I see the confusion. Jointable refers to a database table, not a separate model. Isn’t that the syntax for $this->join(), referring to a table, not a model? In fact, the public table = ‘child’ in the Child class definition is unnecessary, and probably a red herring. There is no ‘child’ table in the database. Notice that I don’t actually have any $this->hasOne()s in Child’s init? What is going on here is that I basically have a transaction table, with many different types of transactions. All of the transactions have a common set of fields (think timestamp, user, etc.), but each type of transaction has a number of fields that are specific to only that kind of transaction. The Par class would be for one of those kinds of transactions (so there would also be Par2, Par3, etc.). But it’s a one-to-one relationship between Par and Child – so the hasMany in your Par model should be as hasOne. I think that’s the confusion.

…

-- Sam From: Michael Voříšek ***@***.***> Sent: Wednesday, November 10, 2021 7:08 AM To: atk4/data ***@***.***> Cc: samsouthardjr ***@***.***>; Author ***@***.***> Subject: Re: [atk4/data] Fix table name source with Join (PR #909) these two lines from your example: https://github.com/atk4/data/compare/fix_upd_join_w_alias#diff-92f78239a1e95b6ddebce64dec3b75d71cfa81ee4073bb544f9f8f491a37218cR700-R701 create only parent and child table names only, add 3rd model with jointable you can fork the branch from my experiment and you can see the actual queries executed - https://github.com/atk4/data/runs/4166653222?check_suite_focus=true — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#909 (comment)> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/AUECRVOEHUCHKUKVGGMAJPLULKDENANCNFSM5GQ6CYMQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub> . <https://github.com/notifications/beacon/AUECRVK3JBIC6C5T5YJAEFTULKDENA5CNFSM5GQ6CYM2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOHGFB44Q.gif>

mvorisek · 2021-11-10T16:12:14Z

Oh, I think I see the confusion. Jointable refers to a database table, not a separate model. Isn’t that the syntax for $this->join(), referring to a table, not a model?

yes, table, but we need a model to create the table using migrator...

mvorisek · 2021-11-23T12:30:39Z

closing in favor of #784 , thank you for the proposed solution, the dealiasing condition is correct

Fixed bug where using a template with [table_noalias] would result in…

38777de

… generating [where] or [having] portions of queries that referenced the alias instead of the de-aliased table.

mvorisek added the bug label Oct 22, 2021

mvorisek requested review from DarkSide666, abbadon1334 and georgehristov October 22, 2021 17:03

mvorisek reviewed Oct 22, 2021

View reviewed changes

mvorisek changed the title ~~Fixed bug where using a template with [table_noalias] would result in…~~ Fix table name source with Join Oct 22, 2021

Fixed coding style and static analysis errors

b4f1938

mvorisek reviewed Oct 22, 2021

View reviewed changes

src/Persistence/Sql/Query.php Outdated Show resolved Hide resolved

mvorisek reviewed Oct 22, 2021

View reviewed changes

src/Persistence/Sql/Query.php Outdated Show resolved Hide resolved

samsouthardjr added 3 commits October 22, 2021 10:57

Additional CodingStyle error fixes

628ef94

Adapting recommended changes

41b38b6

Remove one pesky space

137c3b9

mvorisek closed this Nov 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix table name source with Join#909

Fix table name source with Join#909
samsouthardjr wants to merge 5 commits intoatk4:developfrom
samsouthardjr:develop

samsouthardjr commented Oct 22, 2021 •

edited by mvorisek

Loading

Uh oh!

mvorisek commented Oct 22, 2021

Uh oh!

samsouthardjr commented Oct 22, 2021 •

edited by mvorisek

Loading

Uh oh!

mvorisek Oct 22, 2021

Uh oh!

samsouthardjr Oct 23, 2021

Uh oh!

samsouthardjr commented Oct 22, 2021 via email •

edited by mvorisek

Loading

Uh oh!

Uh oh!

Uh oh!

mvorisek commented Nov 10, 2021 •

edited

Loading

Uh oh!

samsouthardjr commented Nov 10, 2021 via email

Uh oh!

mvorisek commented Nov 10, 2021

Uh oh!

samsouthardjr commented Nov 10, 2021 via email

Uh oh!

mvorisek commented Nov 10, 2021

Uh oh!

mvorisek commented Nov 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

Conversation

samsouthardjr commented Oct 22, 2021 • edited by mvorisek Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mvorisek commented Oct 22, 2021

Uh oh!

samsouthardjr commented Oct 22, 2021 • edited by mvorisek Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mvorisek Oct 22, 2021

Choose a reason for hiding this comment

Uh oh!

samsouthardjr Oct 23, 2021

Choose a reason for hiding this comment

Uh oh!

samsouthardjr commented Oct 22, 2021 via email • edited by mvorisek Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mvorisek commented Nov 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samsouthardjr commented Nov 10, 2021 via email

Uh oh!

mvorisek commented Nov 10, 2021

Uh oh!

samsouthardjr commented Nov 10, 2021 via email

Uh oh!

mvorisek commented Nov 10, 2021

Uh oh!

mvorisek commented Nov 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

samsouthardjr commented Oct 22, 2021 •

edited by mvorisek

Loading

samsouthardjr commented Oct 22, 2021 •

edited by mvorisek

Loading

samsouthardjr commented Oct 22, 2021 via email •

edited by mvorisek

Loading

mvorisek commented Nov 10, 2021 •

edited

Loading