isCrossRef bug for ScheduledForDeletion collection #518

Open
iBeb opened this Issue Nov 27, 2012 · 15 comments

Comments

Projects
None yet
7 participants

iBeb commented Nov 27, 2012

I have this schema (simplified):

  • table marketing_keyword isCroffRed=true
    • column marketing_keywordid, primaryKey
    • column marketing_id, foreign table marketing (local marketing_id, foreign marketing_id)
    • column keyword_id, foreign table keyword (local keyword_id, foreign keyword_id)
  • table marketing
    • column marketing_id, primaryKey
    • other columns
  • table keyword
    • column keyword_id
    • other columns

When I went to add a collection of keywords to marketing, I select a collection of keywords and use the setMarketing(PropelCollection $keywords) method found in BaseKeyword.php.

When I use it, $currentKeywords and keywordsScheduledForDeletion are correct.

in the doSave method, however, there's something wrong:

if ($this->keywordsScheduledForDeletion !== null) {
if (!$this->keywordsScheduledForDeletion->isEmpty()) {
$pks = array();
$pk = $this->getPrimaryKey();
foreach ($this->keywordsScheduledForDeletion->getPrimaryKeys(false) as $remotePk) {
$pks[] = array($pk, $remotePk);
}
MarketingKeywordQuery::create()
->filterByPrimaryKeys($pks) returns nothing!
->delete($con);
$this->keywordsScheduledForDeletion = null;
}

it should be $pks[] = $remotePk;
and in that case, it works fine.

So, either I did something wrong in my schema, or this is a big bug...

_bertrand

Member

havvg commented Nov 27, 2012

Can't verify this; test below passes.

<?php
    public function testCrossRef()
    {
        $finance = new IndustrySector();
        $finance->setName('Finance')->save();

        $surgery = new IndustrySector();
        $surgery->setName('Surgery')->save();

        $insurance = new IndustrySector();
        $insurance->setName('Insurance')->save();

        $product = new Product();
        $product
            ->addIndustrySector($surgery)
            ->addIndustrySector($finance)
            ->setName('A sample Product')
            ->save()
        ;

        $this->assertEquals(2, ProductIndustrySectorQuery::create()->find()->count());

        $collection = new PropelObjectCollection();
        $collection->append($insurance);
        $product->setIndustrySectors($collection)->save();

        $crossRef = ProductIndustrySectorQuery::create()->find();
        $this->assertEquals(1, $crossRef->count());
        $this->assertEquals($product->getId(), $crossRef->getFirst()->getProductId());
        $this->assertEquals($insurance->getId(), $crossRef->getFirst()->getIndustrySectorId());

        $product->addIndustrySector($surgery)->save();
        $crossRef = ProductIndustrySectorQuery::create()->find();
        $this->assertEquals(2, $crossRef->count());

        $product->addIndustrySector($finance)->save();
        $crossRef = ProductIndustrySectorQuery::create()->find();
        $this->assertEquals(3, $crossRef->count());

        $product->removeIndustrySector($finance)->save();
        $crossRef = ProductIndustrySectorQuery::create()->find();
        $this->assertEquals(2, $crossRef->count());
    }

Simplified schema:

<?xml version="1.0" encoding="UTF-8"?>
<database baseClass="Ormigo\Model\BaseObject" name="default" namespace="Ormigo\Model\Product" defaultIdMethod="native" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="http://xsd.propelorm.org/1.6/database.xsd">
    <table name="product" phpName="Product"
           description="A list of Products available for Lead trading.">
        <column name="id" type="integer" autoIncrement="true" primaryKey="true" />
        <column name="name" type="varchar" size="255" required="true" primaryString="true" />

        <behavior name="timestampable" />
        <behavior name="soft_delete" />
        <behavior name="i18n">
            <parameter name="i18n_columns" value="name, description" />
            <parameter name="default_locale" value="de_DE" />
        </behavior>
        <behavior name="versionable">
            <parameter name="log_created_at" value="true" />
            <parameter name="log_created_by" value="true" />
        </behavior>
    </table>

    <table name="industry_sector" phpName="IndustrySector"
           description="A list of industry sectors where Products live in.">
        <column name="id" type="integer" autoIncrement="true" primaryKey="true" />
        <column name="name" type="varchar" size="255" required="true" primaryString="true" />

        <behavior name="i18n">
            <parameter name="i18n_columns" value="name" />
            <parameter name="default_locale" value="de_DE" />
        </behavior>
    </table>

    <table name="product_industry_sector" isCrossRef="true"
           description="A relation which Product is part of which IndustrySector.">
        <column name="product_id" type="integer" primaryKey="true" />
        <column name="industry_sector_id" type="integer" primaryKey="true" />

        <foreign-key foreignTable="product" onDelete="CASCADE" onUpdate="CASCADE">
            <reference local="product_id" foreign="id" />
        </foreign-key>
        <foreign-key foreignTable="industry_sector" onDelete="CASCADE" onUpdate="CASCADE">
            <reference local="industry_sector_id" foreign="id" />
        </foreign-key>
    </table>
</database>

iBeb commented Nov 27, 2012

I've found the reason why it works or not depending on the way you design your table.
In my case, the two foreign keys are not primary keys in the association table which has a primary key with auto increment. Therefore, this : $pks[] = array($pk, $remotePk); can't work.

Without changing my database, I've made some tries in my schema.xml

Case 1 (fails)

  • table_one
    • one_id as primary key, auto increment
  • table_two
    • two_id as primary key, auto increment
  • association_table, isCrossRef=true
    • id as primary key, auto increment
    • one_id (not primary)
    • two_id (not primary)
    • foreign keys table_one.one_id=association_table.one_id, table_two.two_id=association_table.two_id

Case 2 (success!)

  • table_one
    • one_id as primary key
  • table_two
    • two_id as primary key
  • association_table, isCrossRef=true
    • id (not primary, just auto increment)
    • one_id primary key
    • __two_id primary key __
    • foreign keys table_one.one_id=association_table.one_id, table_two.two_id=association_table.two_id

This is not really a bug then. But I think I'm not the only one to have a single primary key in association table, and the generator should make the difference and adjust the code.

Question: how did you add the code syntax for your tables?

Contributor

rozwell commented Nov 28, 2012

http://propelorm.org/documentation/04-relationships.html#manytomany_relationships

Use only isCrossRef with a middle table that is design the same way as the example (user_group), two foreign keys that are also primary keys. If you use a different schema for the middle table, use it at your own risk

Member

havvg commented Nov 28, 2012

Why do you need a isCrossRef=true with its own id, what's the purpose?

iBeb commented Nov 28, 2012

It's just an old habit to keep an eye on the activity of the table (few records but high auto increment: large activity...) and also because in some cases in my association tables I add other fields (order, type...) and it's easier to fetch a record by a single primary key, at least when I was not using Propel...
I need to update my schemas, I forgot Propel does the job for you...

iBeb commented Dec 1, 2012

I reopen the discussion because this time this is really a bug...

I've updated my schema and database.
Now I have these 3 tables

  • service
    • service_id (primary, auto increment)
    • columns...
  • marketing
    • marketing_id (primary, auto increment)
    • columns...
  • service_marketing
    • service_id (primary) => first column in my schema.xml AND database
    • marketing_id (primary) => second column in my schema.xml AND database

When I want to update service_marketing from service using the method setMarketings(PropelCollection $marketings), in BaseService, $marketingsScheduledForDeletion is normally filled with items that are no longer desired.

But, on the doSave method:

$pks = array();
$pk = $this->getPrimaryKey(); => service_id
foreach ($this->marketingsScheduledForDeletion->getPrimaryKeys(false) as $remotePk) {
$pks[] = array($remotePk, $pk); marketing_id, service_id => wrong order!
}

And in BaseMarketing, the doSave method does this:

$pks = array();
$pk = $this->getPrimaryKey(); => marketing_id
foreach ($this->servicesScheduledForDeletion->getPrimaryKeys(false) as $remotePk) {
$pks[] = array($pk, $remotePk); marketing_id, service_id => wrong order!
}

What did I do wrong?

_bertrand

Member

havvg commented Dec 1, 2012

Can you paste the complete schema for those tables?

iBeb commented Dec 1, 2012

I don't know how to past code on GitHub.
Here's the code:

On Dec 1, 2012, at 12:22 PM, Toni Uebernickel notifications@github.com wrote:

Can you paste the complete schema for those tables?


Reply to this email directly or view it on GitHub.

Member

havvg commented Dec 1, 2012

You can highlight it with three backticks, see http://github.github.com/github-flavored-markdown/

Owner

willdurand commented Dec 1, 2012

@havvg doesn't work with email replies.

See:

  <table name="marketing" phpName="Marketing" idMethod="native">
    <column name="marketing_id" phpName="MarketingId" type="INTEGER" sqlType="int(11) unsigned" primaryKey="true" autoIncrement="true" required="true"/>
    <column name="brand_id" phpName="BrandId" type="INTEGER" sqlType="int(11) unsigned" required="true"/>
    <column name="region_id" phpName="RegionId" type="INTEGER" sqlType="int(11) unsigned" required="true"/>
    <column name="activity_id" phpName="ActivityId" type="INTEGER" sqlType="int(11) unsigned" required="true"/>
    <column name="type" phpName="Type" type="CHAR" sqlType="enum('c','s')" required="false" defaultValue="c"/>
    <column name="data" phpName="Data" type="CLOB" required="false"/>
    <column name="active" phpName="Active" type="BOOLEAN" size="1" sqlType="tinyint(1) unsigned" required="true" defaultValue="true"/>
    <column name="created_at" phpName="CreatedAt" type="TIMESTAMP" required="false"/>
    <column name="updated_at" phpName="UpdatedAt" type="TIMESTAMP" required="false"/>
    <foreign-key foreignTable="activity" foreignSchema="Web" name="tm_activity_id">
      <reference local="activity_id" foreign="activity_id"/>
    </foreign-key>
    <foreign-key foreignTable="region" name="tm_region_id_fk">
      <reference local="region_id" foreign="region_id"/>
    </foreign-key>
    <foreign-key foreignTable="brand" name="tm_brand_id_fk">
      <reference local="brand_id" foreign="brand_id"/>
    </foreign-key>
    <unique name="brand_region_activity">
      <unique-column name="brand_id"/>
      <unique-column name="region_id"/>
      <unique-column name="activity_id"/>
    </unique>
    <index name="tm_region_id_idx">
      <index-column name="region_id"/>
    </index>
    <index name="tm_activity_id_idx">
      <index-column name="activity_id"/>
    </index>
  </table>

  <table name="service" phpName="Service" idMethod="native">
    <column name="service_id" phpName="ServiceId" type="INTEGER" sqlType="int(11) unsigned" primaryKey="true" autoIncrement="true" required="true"/>
    <column name="provider_id" phpName="ProviderId" type="INTEGER" sqlType="int(11) unsigned" required="false" defaultValue="0"/>
    <column name="type_id" phpName="TypeId" type="INTEGER" sqlType="int(11) unsigned" required="true" defaultValue="0"/>
    <column name="title" phpName="Title" type="VARCHAR" size="50" required="false"/>
    <column name="code" phpName="Code" type="VARCHAR" size="10" required="false"/>
    <column name="data" phpName="Data" type="LONGVARCHAR" required="false"/>
    <column name="address" phpName="Address" type="VARCHAR" required="false"/>
    <column name="zip" phpName="Zip" type="VARCHAR" size="5" required="false"/>
    <column name="city" phpName="City" type="VARCHAR" size="50" required="false"/>
    <column name="country_code" phpName="CountryCode" type="CHAR" size="3" required="false"/>
    <column name="longitude" phpName="Longitude" type="VARCHAR" size="11" required="false"/>
    <column name="latitude" phpName="Latitude" type="VARCHAR" size="11" required="false"/>
    <column name="start_date" phpName="StartDate" type="DATE" required="false"/>
    <column name="end_date" phpName="EndDate" type="DATE" required="false"/>
    <column name="active" phpName="Active" type="BOOLEAN" size="1" sqlType="tinyint(1) unsigned" required="true" defaultValue="true"/>
    <column name="mandatory" phpName="Mandatory" type="BOOLEAN" size="1" sqlType="tinyint(1) unsigned" required="true" defaultValue="false"/>
    <column name="checked" phpName="Checked" type="BOOLEAN" size="1" sqlType="tinyint(1) unsigned" required="true" defaultValue="false"/>
    <column name="favorite" phpName="Favorite" type="BOOLEAN" size="1" sqlType="tinyint(1) unsigned" required="true" defaultValue="false"/>
    <column name="hidden" phpName="Hidden" type="BOOLEAN" size="1" sqlType="tinyint(1) unsigned" required="true" defaultValue="false"/>
    <column name="bookable" phpName="Bookable" type="BOOLEAN" size="1" sqlType="tinyint(1) unsigned" required="true" defaultValue="true"/>
    <column name="created_at" phpName="CreatedAt" type="TIMESTAMP" required="false"/>
    <column name="updated_at" phpName="UpdatedAt" type="TIMESTAMP" required="false"/>
    <foreign-key foreignTable="provider" name="s_provider_id_fk">
      <reference local="provider_id" foreign="provider_id"/>
    </foreign-key>
    <foreign-key foreignTable="service_type" name="s_type_id">
      <reference local="type_id" foreign="type_id"/>
    </foreign-key>
    <foreign-key foreignTable="country" foreignSchema="Library" name="s_country_code_fk">
      <reference local="country_code" foreign="country_code"/>
    </foreign-key>
    <index name="s_country_code_idx">
      <index-column name="country_code"/>
    </index>
    <index name="s_provider_id_idx">
      <index-column name="provider_id"/>
    </index>
    <index name=s_type_id_idx">
      <index-column name="type_id"/>
    </index>
    <index name="longitude">
      <index-column name="longitude"/>
    </index>
    <index name="latitude">
      <index-column name="latitude"/>
    </index>
  </table>

  <table name="service_marketing" phpName="ServiceMarketing" idMethod="native" isCrossRef="true">
    <column name="service_id" phpName="ServiceId" type="INTEGER" sqlType="int(11) unsigned" required="true" primaryKey="true"/>
    <column name="marketing_id" phpName="MarketingId" type="INTEGER" sqlType="int(11) unsigned" required="true" primaryKey="true"/>
    <column name="created_at" phpName="CreatedAt" type="TIMESTAMP" required="false"/>
    <column name="updated_at" phpName="UpdatedAt" type="TIMESTAMP" required="false"/>
    <foreign-key foreignTable="marketing" name="sm_marketing_id_fk" onDelete="CASCADE">
      <reference local="marketing_id" foreign="marketing_id"/>
    </foreign-key>
    <foreign-key foreignTable="service" name="sm_service_id_fk" onDelete="CASCADE">
      <reference local="service_id" foreign="service_id"/>
    </foreign-key>
    <index name="sm_marketing_id_idx">
      <index-column name="marketing_id"/>
    </index>
    <index name="sm_service_id_idx">
      <index-column name="service_id"/>
    </index>
  </table>
Member

havvg commented Jan 18, 2013

I just encountered this error and found the cause. By changing my schema this way, the provided test fails:

diff --git a/src/Ormigo/Bundle/OrmigoBundle/Resources/config/propel/product_schema.xml b/src/Ormigo/Bundle/OrmigoBundle/Resources/config/propel/product_schema.xml
index 96ebea8..e3ea213 100644
--- a/src/Ormigo/Bundle/OrmigoBundle/Resources/config/propel/product_schema.xml
+++ b/src/Ormigo/Bundle/OrmigoBundle/Resources/config/propel/product_schema.xml
@@ -162,12 +162,12 @@
         <column name="product_id" type="integer" primaryKey="true" />
         <column name="industry_sector_id" type="integer" primaryKey="true" />

-        <foreign-key foreignTable="product" onDelete="CASCADE" onUpdate="CASCADE">
-            <reference local="product_id" foreign="id" />
-        </foreign-key>
         <foreign-key foreignTable="industry_sector" onDelete="CASCADE" onUpdate="CASCADE">
             <reference local="industry_sector_id" foreign="id" />
         </foreign-key>
+        <foreign-key foreignTable="product" onDelete="CASCADE" onUpdate="CASCADE">
+            <reference local="product_id" foreign="id" />
+        </foreign-key>
     </table>

     <table name="product_characteristic" isCrossRef="true"

Having the foreign-key set up the same order as the primary key columns, solves the issue.

<?php

// In PHP5ObjectBuilder at line 4036

        $middelFks = $refFK->getTable()->getForeignKeys();
        $isFirstPk = ($middelFks[0]->getForeignTableCommonName() == $this->getTable()->getCommonName());
Member

jaugustin commented Jan 18, 2013

@havvg thanks for finding the work around ;)

I will try to fix it, I just need to write the test first

Contributor

auss commented Nov 5, 2013

any change on this ?

Owner

marcj commented Nov 5, 2013

Is fixed in propelorm/Propel2#476 of Propel2.

Member

jaugustin commented Nov 6, 2013

@auss no I didn't found time to work on it

@havvg havvg referenced this issue in propelorm/PropelBundle Jan 28, 2014

Closed

Foreign key order is important #262

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