[1.1.2+] belongsToMany relation causes "integrity constraint violation" error (column is ambiguous)

[adrenth]

• OctoberCMS Build: 1.1.2
• PHP Version: 7.4
• Database Engine: MySQL
• Plugins Installed: Vdlp.Acme (see: https://github.com/vdlp/octobercms-issues/tree/new-pivot-query)

Description:

Unfortunately after upgrading to OC version 1.1.2 we ran into multiple issues due to the following accepted PR’s.

• Apply conditions/scopes to related model query library#536
• Fix belongsToMany relation library#543
• Initial issue which resulted in these PR's: Relation conditions aren't being applied when saving models #4952

Which took us a lot of debugging and resulted in database query errors in multiple projects…

The added function newPivotQuery in particular is causing issues: octobercms/library@v1.1.1...v1.1.2#diff-56f77bab4b0aa050a15e11466ccccbe82ce391f7adeed778ddc5c66c41152127R133

The $query object is extended with a join clause.

After newPivotQuery where conditions are being added (outside of this function) without fully qualified column names. Which results in an "integrity constraint violation" (ambiguous column name) error.

Example table setup:

vdlp_acme_acmes

• acme_id (PK)
• name

vdlp_acme_items

• item_id (PK)
• name

vdlp_acme_acme_item

• acme_id (PK)
• item_id (PK)
• name

We propose to rollback this breaking change and let it be reviewed in more detail on how to fix the issue properly without too much extending Laravels' framework code.

Steps To Reproduce:

Use October CMS version 1.1.2 (or 1.1.3).

• Download the Vdlp.Acme plugin from https://github.com/vdlp/octobercms-issues/tree/new-pivot-query
• Put it in the plugin directory plugins/vdlp/acme.
• Run command php artisan october:up
• Then run the command php artisan vdlp:acme:test to run the example code.

This is an example, you still need the full example plugin Vdlp.Acme (see: https://github.com/vdlp/octobercms-issues/tree/new-pivot-query) for it to run.

<?php

declare(strict_types=1);

namespace Vdlp\Acme\Console;

use Composer\Script\Event;
use Illuminate\Console\Command;
use Illuminate\Support\Facades\DB;
use Vdlp\Acme\Models\Acme;
use Vdlp\Acme\Models\Item;

final class TestCommand extends Command
{
    public function __construct()
    {
        $this->signature = 'vdlp:acme:test';
        $this->description = 'A test command';

        parent::__construct();
    }

    public function handle(): void
    {
        DB::connection()->enableQueryLog();

        // Make sure all tables are truncated before we start the test.
        Acme::query()->truncate();
        Item::query()->truncate();
        DB::table('vdlp_acme_acme_item');

        $acme = Acme::create([
            'acme_id' => 2,
            'name' => 'Acme 2',
        ]);

        $item = Item::create([
            'item_id' => 4,
            'name' => 'Item 4',
        ]);

        $acme->items()->add($item, [
            'name' => 'Acme 2 - Item 4',
        ]);

        /*
         * Results in a "integrity constrant violation"-error:
         * SQLSTATE[23000]: Integrity constraint violation: 1052 Column 'item_id' in where clause is ambiguous
         * (SQL: delete `vdlp_acme_acme_item` from `vdlp_acme_acme_item` inner join `vdlp_acme_items`
         * on `vdlp_acme_items`.`item_id` = `vdlp_acme_acme_item`.`item_id` where `acme_id` = 2 and `item_id` in (4))
         */
        $acme->items()->detach(4);

        $queries = DB::getQueryLog();

        dd($queries);
    }
}

[daftspunk]

Apologies regarding this. We will get this sorted in v1.1.4 today. Appreciate the detailed report

[daftspunk]

Test completed OK

λ php artisan vdlp:acme:test
^ array:6 [
  0 => array:3 [
    "query" => "truncate table `vdlp_acme_acmes`"
    "bindings" => []
    "time" => 499.93
  ]
  1 => array:3 [
    "query" => "truncate table `vdlp_acme_items`"
    "bindings" => []
    "time" => 405.29
  ]
  2 => array:3 [
    "query" => "insert into `vdlp_acme_acmes` (`acme_id`, `name`, `updated_at`, `created_at`) values (?, ?, ?, ?)"
    "bindings" => array:4 [
      0 => 2
      1 => "Acme 2"
      2 => "2021-03-23 23:55:15"
      3 => "2021-03-23 23:55:15"
    ]
    "time" => 68.58
  ]
  3 => array:3 [
    "query" => "insert into `vdlp_acme_items` (`item_id`, `name`, `updated_at`, `created_at`) values (?, ?, ?, ?)"
    "bindings" => array:4 [
      0 => 4
      1 => "Item 4"
      2 => "2021-03-23 23:55:15"
      3 => "2021-03-23 23:55:15"
    ]
    "time" => 72.05
  ]
  4 => array:3 [
    "query" => "insert into `vdlp_acme_acme_item` (`acme_id`, `created_at`, `item_id`, `name`, `updated_at`) values (?, ?, ?, ?, ?)"
    "bindings" => array:5 [
      0 => 2
      1 => October\Rain\Argon\Argon @1616543715^ {#990
        #constructedObjectId: "000000003e092bad000000005511f5ff"
        #localMonthsOverflow: null
        #localYearsOverflow: null
        #localStrictModeEnabled: null
        #localHumanDiffOptions: null
        #localToStringFormat: null
        #localSerializer: null
        #localMacros: null
        #localGenericMacros: null
        #localFormatFunction: null
        #localTranslator: null
        #dumpProperties: array:3 [
          0 => "date"
          1 => "timezone_type"
          2 => "timezone"
        ]
        #dumpLocale: null
        date: 2021-03-23 23:55:15.151512 e (+00:00)
      }
      2 => 4
      3 => "Acme 2 - Item 4"
      4 => October\Rain\Argon\Argon @1616543715^ {#990}
    ]
    "time" => 66.08
  ]
  5 => array:3 [
    "query" => "delete from `vdlp_acme_acme_item` where `acme_id` = ? and `item_id` in (?)"
    "bindings" => array:2 [
      0 => 2
      1 => 4
    ]
    "time" => 73.19
  ]
]

[daftspunk]

This will be fixed in patch release 1.1.4

Thanks for using October CMS 🙏

[vdlp-mw]

We created a PR that was recently merged into laravel/framework v6.x (laravel/framework#36720) that would theoretically allow the rolled back PR to function. What we are not sure of yet is wether the implemented fix in #4952 was the best road to take.

If we override the newPivotQuery method with a join then all methods calling this function will be joining a third table. This may have some unexpected impact that calls for further investigating.

[daftspunk]

Cool, overall that is a good improvement to Laravel internals

If we override the newPivotQuery method with a join then all methods calling this function will be joining a third table. This may have some unexpected impact that calls for further investigating.

Correct, the solution increases the surface area of the defined constraints to affect the pivot query, in addition to the relational query. It is asking for trouble for any implementation with even moderate complexity. Taking a look at the original reasoning behind this, it is hard to justify. There are several ways the original reporter could have solved this in their own design, such as using MorphByMany type or use an independent join table for each data type or extending the form query

We will leave this out for now. If it comes up again, we can revisit. Thank you for following up