3rd-party database backends: Do you need a "can_introspect_null" feature flag?

[Shai Berger]

Hi all,

Bug #22816[1] is a test failure on Oracle. It was caused by the introduction
of the "can_introspect_null" database-feature-flag, which was set to True for
all core database backends except for Oracle; and use of that flag in the test
of introspection of NullBooleanField. The easiest way to fix the tests is to
simply revert the relevant commits -- as far as core backends are concerned,
they only affect Oracle, and there, they break the tests for no good reason
(details below, if you care).

However, if there actually is some backend which has trouble introspecting
nullability, then the flag could be useful, and should be kept. In that case,
we'll fix its use with Oracle and the tests.

So -- is there a backend which needs this flag, or can I just kill it?

Details about Oracle:

The test fails because, in fact, Oracle introspects NullBooleanField's
perfectly well -- it only fails to introspect nullability correctly on
CharField's; this is due to its interpretation of empty strings as nulls,
which forces the backend to make all CharField's nullable. Thus, the commit
message on the flag addition -- "Previously this was conflated with another
Oracle-specific behavior" -- is actually wrong; the "conflation" is fully
justified.

Thanks,
Shai.

[1] https://code.djangoproject.com/ticket/22816

[Michael Manfre]

The feature was added by Aymeric for django-pymssql, which needs the feature.

Regards,

Michael Manfre

--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/201406130117.32421.shai%40platonix.com.
For more options, visit https://groups.google.com/d/optout.

[Aymeric Augustin]

Sorry. We should probably set can_introspect_null = True on Oracle and restore the previous logic in the CharField introspection test.

-- 

Aymeric.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CAGdCwBtVv4KUsdiOe9-fRxQpxQuQ1vKwg1Pc9hvp-0Suv3%3DX0w%40mail.gmail.com.

[Shai Berger]

On Friday 13 June 2014 08:52:29 Aymeric Augustin wrote:
> > Le 13 juin 2014 à 01:30, Michael Manfre <mma...@gmail.com> a écrit :
> >> On Thu, Jun 12, 2014 at 6:17 PM, Shai Berger <sh...@platonix.com> wrote:
> >>
> >> Bug #22816[1] is a test failure on Oracle. It was caused by the
> >> introduction of the "can_introspect_null" database-feature-flag,
> >

> > The feature was added by Aymeric for django-pymssql, which needs the
> > feature.
> >

> Sorry. We should probably set can_introspect_null = True on Oracle and
> restore the previous logic in the CharField introspection test.
>

I set can_introspect_null=True on Oracle; the test was corrected to pass
(rather than be skipped) on Oracle, by checking that nullability matches
interprets_empty_strings_as_null. It is still skipped unless
can_introspect_null, so django-pymssql should be ok.

Thanks,
Shai.

[Rahul]

DB2 backend can introspect the nullability correctly, but it can't introspect BooleanField.

It introspect Boolean field as SmallIntegerField but in the test case if can_introspect_boolean_field=False then it checks only for IntegerField. By including introspect_boolean_field_as_smallInteger and introspect_boolean_field_as_Integer feature flag can eliminate this problem.

[Aymeric Augustin]

2014-06-17 12:26 GMT+02:00 Rahul <rahul.pr...@in.ibm.com>:

It introspect Boolean field as SmallIntegerField but in the test case if can_introspect_boolean_field=False then it checks only for IntegerField. By including introspect_boolean_field_as_smallInteger and introspect_boolean_field_as_Integer feature flag can eliminate this problem. 

That may be going a bit too far...

Should we eliminate all the `else:` clauses of tests controlled by `if connection.features.can_introspect_xxx:`? If the backend can't introspect something properly, we don't care that much about what it introspects instead.

Backwards-compatibility concerns are minimal. I don't think it's a big deal if the output of inspectdb changes accidentally for a core backend for a field that isn't correctly inspected anyway.

--
Aymeric.

[Shai Berger]

On Tuesday 17 June 2014 13:53:25 Aymeric Augustin wrote:
> 2014-06-17 12:26 GMT+02:00 Rahul <rahul.pr...@in.ibm.com>:
> > It introspect Boolean field as SmallIntegerField but in the test case if
> > can_introspect_boolean_field=False then it checks only for IntegerField.
> > By including introspect_boolean_field_as_smallInteger and
> > introspect_boolean_field_as_Integer feature flag can eliminate this
> > problem.
>
> That may be going a bit too far...
>

I agree.

> Should we eliminate all the `else:` clauses of tests controlled by `if
> connection.features.can_introspect_xxx:`? If the backend can't introspect
> something properly, we don't care that much about what it introspects
> instead.

I think that is going too far in the other direction -- specifically, in the
case of parameters. As a backend maintainer, if I couldn't introspect
NullBooleanField, I think I'd still like to verify that I get a BooleanField.
If I can't get the right length, I still want to know it's a CharField.

In that spirit, Rahul (and Michael), I think we should just change the test to
allow for either IntegerField or SmallIntegerField when not
can_introspect_boolean_field.

>
> Backwards-compatibility concerns are minimal. I don't think it's a big deal
> if the output of inspectdb changes accidentally for a core backend for a
> field that isn't correctly inspected anyway.

Right -- within the above, IMO.

Shai.

[Michael Manfre]

On Tue, Jun 17, 2014 at 8:49 AM, Shai Berger <sh...@platonix.com> wrote:

> Should we eliminate all the `else:` clauses of tests controlled by `if

> connection.features.can_introspect_xxx:`? If the backend can't introspect
> something properly, we don't care that much about what it introspects
> instead.

I think that is going too far in the other direction -- specifically, in the
case of parameters. As a backend maintainer, if I couldn't introspect
NullBooleanField, I think I'd still like to verify that I get a BooleanField.
If I can't get the right length, I still want to know it's a CharField.

In that spirit, Rahul (and Michael), I think we should just change the test to
allow for either IntegerField or SmallIntegerField when not
can_introspect_boolean_field.

That's not much better than just skipping the assertion for those databases. My suggestion would be to allow a backend to provide its own assertFieldType [1]. This would move all of the feature checking out of the tests and allow a backend to inspect custom fields. We could probably get rid of some of these introspect features too. I'm not sure whether it makes more sense to put assertFieldType on DatabaseFeatures or DatabaseIntrospection.

[1] https://github.com/django/django/blob/master/tests/inspectdb/tests.py#L36

Regards,

Michael Manfre

 

>
> Backwards-compatibility concerns are minimal. I don't think it's a big deal
> if the output of inspectdb changes accidentally for a core backend for a
> field that isn't correctly inspected anyway.

Right -- within the above, IMO.

Shai.

--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/201406171549.08377.shai%40platonix.com.

[Aymeric Augustin]

2014-06-17 14:49 GMT+02:00 Shai Berger <sh...@platonix.com>:

In that spirit, Rahul (and Michael), I think we should just change the test to
allow for either IntegerField or SmallIntegerField when not
can_introspect_boolean_field.

I'm OK with that stop-gap solution for DB2.

It's not the end of the story, but it's an easy enough improvement

--
Aymeric.

[Maximiliano Robaina]

El martes, 17 de junio de 2014 07:26:03 UTC-3, Rahul escribió:

DB2 backend can introspect the nullability correctly, but it can't introspect BooleanField.

It introspect Boolean field as SmallIntegerField but in the test case if can_introspect_boolean_field=False then it checks only for IntegerField. By including introspect_boolean_field_as_smallInteger and introspect_boolean_field_as_Integer feature flag can eliminate this problem.

+1

Same in Firebird