test_disable_constraint_checks_context_manager

[Wlodek]

Hi all,

I have doubt about mentioned test. Is it correct? 

As context manager "constraint_checks_disabled" enables 

constraint checking upon exit which is before end of transaction (rollback)

there will/should be IntegrityError raised?

As I understand this test passes for oracle/postgres because there 

is no implementation for "enable/disable_constraint_checking" for those backends?

I'm on stable/1.5.x but master looks the same.

--

Wlodek

[Shai Berger]

On Sunday 02 February 2014 23:08:05 Wlodek wrote:
> Hi all,
>
> I have doubt about mentioned test. Is it correct?
>

The test is correct for the backends in core.

> As context manager "constraint_checks_disabled" enables
> constraint checking upon exit which is before end of transaction (rollback)
> there will/should be IntegrityError raised?
>

That depends on the backend. For MySQL, you can (and the backend does) enable
the constraints without checking them on existing data, so no error is raised.

I don't know if you can force checks while enabling constraints on MySQL; you
can (and should) on MSSQL, which AFAIR doesn't have deferrable FKs -- and then
the test would fail. Although MSSQL is not in core, we should probably try
harder to be friendly towards it.

> As I understand this test passes for oracle/postgres because there
> is no implementation for "enable/disable_constraint_checking" for those
> backends?
>

Not exactly. The methods, as you say, do nothing -- so you could expect the
test to fail because the checks are never turned off. But Postgres and Oracle
have deferrable FKs -- which means the checks for FK constraints are only done
at transaction commit time; since the transaction is rolled back, nothing
fails.

There is one thing in the test (also the preceding test) which could be seen
as a little misleading -- the line "transaction.set_rollback(True)" only gets
executed when the test passes, because self.fail(...) raises an exception. You
don't see that in the code -- you would if it were in an "else" block" -- but
if you're familiar with tests, it shouldn't surprise you much.

HTH,
Shai.

[Wlodek]

Still disagree. Beign correct you must mean it passes while 

I would say it is incorrect conceptually. besides one might expect it to 

work on other backends as well, which is exactly what I try to do,

not only core backends.

I understand that in some backends you can declare default/initial constraint 

deferability which is set by django to "deferred" by default because that is 

what we need most of the time, ie. constraint checks at the commit.

But one might ask two questions

1. What are context manager "constraint_checks_disabled", and methods

"disable/enable_constraint_checking" supposed to do? If nothing than 

they can be removed.

2. What if the backend used doesn't have default/initial constraint deferrability 

but has transaction level set contraint checking?

I would expect that if the backend/database used can control constraint checking 

(defere it or enforce) then "constraint_checks_disabled" should implement it.

Not only if a backend doesn't have default/initial constraint deferrability when 

those 3 methods are the only means to control constraint but even if it has

initial deferrability those should be implemented. You may not use them 

which one will almost always do but if you do they should work as supposed.

As "constraint_checks_disabled" is expected to disable contraint checking

for a given block of statements and enforce them at the end inside 

a transation (as savepoints do for commits) and in the test

we are checking that IntegrityError is not raised then in  implemented (not empty)

constraint_checks_disabled and disable/enable_constraint_checking you should 

use it in expected way, ie. ensure that constraints are not vialated at the exit 

from context manager, ie. something like that:

try:

    with connection.constraint_checks_disabled():

        a.save()

        a.reporter_id= self.r.id 

        a.save()

except IntegrityError:

    self.fail("IntegrityError should not have occurred.")

That should work as expected in all cases.

The same goes for two other tests in this test case.

Regards

Wlodek