propelorm / Propel

Current stable (and outdated and unmaintained) version of Propel - Please use v2 https://github.com/propelorm/Propel2 --
http://www.propelorm.org
MIT License
841 stars 418 forks source link

isCrossRef bug for ScheduledForDeletion collection #518

Open iBeb opened 11 years ago

iBeb commented 11 years ago

I have this schema (simplified):

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

havvg commented 11 years ago

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 11 years ago

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)

Case 2 (success!)

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?

rozwell commented 11 years ago

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

havvg commented 11 years ago

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

iBeb commented 11 years ago

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 11 years ago

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

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(); _=> serviceid foreach ($this->marketingsScheduledForDeletion->getPrimaryKeys(false) as $remotePk) { $pks[] = array($remotePk, $pk); _marketing_id, serviceid => wrong order! }

And in BaseMarketing, the doSave method does this:

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

What did I do wrong?

_bertrand

havvg commented 11 years ago

Can you paste the complete schema for those tables?

iBeb commented 11 years ago

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.

havvg commented 11 years ago

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

willdurand commented 11 years ago

@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>
havvg commented 11 years ago

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());
jaugustin commented 11 years ago

@havvg thanks for finding the work around ;)

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

auss commented 10 years ago

any change on this ?

marcj commented 10 years ago

Is fixed in https://github.com/propelorm/Propel2/pull/476 of Propel2.

jaugustin commented 10 years ago

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