Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

NOT NULL not accounted when deleting referenced object in 1:n #636

Closed
havvg opened this Issue Mar 26, 2013 · 8 comments

Comments

Projects
None yet
3 participants
Member

havvg commented Mar 26, 2013

The fix for #603 is a major BC Break right now (288e91d#L0L4097).

This change breaks by generating the following code:

            if ($this->bankAccountsScheduledForDeletion !== null) {
                if (!$this->bankAccountsScheduledForDeletion->isEmpty()) {
                    foreach ($this->bankAccountsScheduledForDeletion as $bankAccount) {
                        // need to save related object because we set the relation to null
                        $bankAccount->save($con);
                    }
                    $this->bankAccountsScheduledForDeletion = null;
                }
            }

The snippet from the schema:

    <table name="bank_account">
        <column name="id" type="integer" autoIncrement="true" primaryKey="true" />
        <column name="customer_id" type="integer" required="true" />
        <foreign-key foreignTable="customer">
            <reference local="customer_id" foreign="id" />
        </foreign-key>
    </table>

The previous code read (correctly):

                    BankAccountQuery::create()
                        ->filterByPrimaryKeys($this->bankAccountsScheduledForDeletion->getPrimaryKeys(false))
                        ->delete($con);

The current code cannot be fulfilled by the database, as the customer_id column is required (NOT NULL).

The following test fails:

    public function testDeleteCustomerCascades()
    {
        $customer = new Customer();
        $customer
            ->setUser(
                (new User())
                ->setFirstname('Max')
                ->setSurname('Mustermann')
            )
            ->addBankAccount(
                (new BankAccount())
                ->setAccountHolder('Max Mustermann')
                ->setAccountNumber(1234567)
                ->setBank(
                    (new Bank())
                    ->setName('Bundebank')
                    ->setCode('10000000')
                    ->setBic('MARKDEF1100')
                )
            )
            ->save()
        ;

        $customer->delete();

        $this->assertEquals(0, BankAccountQuery::create()->count());
    }
Owner

marcj commented Mar 26, 2013

You mean actually onDelete="CASCADE" is for foreign-key the default? I guess, if you add that to your foreign-key then it is fixed, isn't it?

Member

havvg commented Mar 26, 2013

Adding the onDelete fixes it, yes.

CASCADE is not default for onDelete, it's RESTRICT, however not defining it and having a NOT NULL column in the foreign key should (and did prior the change) alter the default behavior to CASCADE, which makes the most sense.

For now I added every single onDelete and onUpdate to my schema files, just to be sure :)

Owner

marcj commented Mar 27, 2013

The problem I see here is that RESTRICT is only for MySQL the default. In PostgreSQL and SQLite it's NO ACTION [1]. Also in the Propel's documentation it's not mentioned which behavior is the default one, so it looks like Propel does not use a default value if the attribute is omitted, thus it's completely database based which is actually not that good. However, my suggestion is to define now a default value if its omitted and implement that correctly as well as mention it in the documentation.

@willdurand, @staabm, @jaugustin, RFC.

[1] SQLite If an action is not explicitly specified, it defaults to "NO ACTION". - http://www.sqlite.org/foreignkeys.html#fk_actions
[1] PostreSQL NO ACTION means that [...]; this is the default behavior if you do not specify anything - http://www.postgresql.org/docs/9.0/static/ddl-constraints.html

Member

havvg commented Mar 27, 2013

RESTRICT and NO ACTION are basically the same. The only difference is the actual timing (deferred or directly, if supported by RDBMS) when the alteration of the tables in a relationship is checked against foreign key violations. From a users perspective the outcome is the same. Either of those values (depending on support) is the default in every SQL based RDBMS I know; simply because it's the most sane one.

However, it doesn't make sense, to have a clear database related setup for this sort of things. Those constraints are mainly set to ensure a valid state on the database itself and it's ensuring it by itself; speaking: The generated SQL is the main concern when adding those in your schema.

The actual behavior of Propel should aid the user to never actually violate those constraints. If you would follow the given constraints in the generated code, you would have to do everything in user-land (e.g. deleting the BankAccounts prior deleting the Customer) which doesn't help. If my code deletes a Customer, I want to delete it - and I mean it. My example is actually pretty good for this: I didn't specify the ON DELETE on the BankAccount, which results in the default of RESTRICT on the database level on the relation to Customer. Propel used to make sure that the this constraints was not violated by clearing related models (which would actually be CASCADE). I discovered this yesterday while updating my schema and reverted the changes immediately because the database would change, too. I don't want to have the database delete certain entries when deleting related entries. This makes sure, that applications not being the main application have the same understanding of what they are deleting (for this the given relation is a bad example :D). An (even bad) example would be a user directly connected to the server, e.g. phpMyAdmin or a command line client. If this user tries to delete a Customer, he should be damn sure, what he is about to delete with it maybe on accident.

I'm with you on having a documented default value (or behavior) for both values. But as I said, currently the change is a BC break - a major one, imho; everything mentioned above was working perfectly fine!

Owner

marcj commented Mar 27, 2013

Ok, so I believe beside $refFK->getOnDelete() == ForeignKey::CASCADE should be another check like || $refFK->isAtLeastOneLocalNotNull(). Can you provide a PR for that?

Owner

marcj commented Mar 28, 2013

Strange that we don't have a test for that basic behavior. o.O

Owner

willdurand commented Mar 30, 2013

I agree on choosing a default behavior. Anyway, fixing the BC break must be the priority.

Member

havvg commented Apr 9, 2013

I'm on this today+, we need to regenerate our models and this is the missing piece :)

@havvg havvg referenced this issue Apr 11, 2013

Merged

deleting objects #656

@havvg havvg closed this Apr 11, 2013

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