Opened 9 years ago

Closed 9 years ago

#25400 closed Bug (fixed)

Attributes are always set on `connection.features` (gis)

Reported by: Daniel Hahler Owned by: nobody
Component: Database layer (models, ORM) Version: dev
Severity: Release blocker Keywords: regression
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

With commit d9ff5ef36d3f714736d633435d45f03eac9c17b5 the behaviour of hasattr
on connection.features seems to have changed, e.g. the following will be True
now always, and was previously so only for Django < 1.5.

hasattr(connection.features, 'confirm')

pytest-django is using the following code:

# confirm() is not needed/available in Django >= 1.5
# See https://code.djangoproject.com/ticket/17760
if hasattr(self.connection.features, 'confirm'):
    self.connection.features.confirm()

The problem seems to be that __getattr__ should raise an AttributeError in this case.
(Source: https://github.com/django/django/commit/d9ff5ef36d3f714736d633435d45f03eac9c17b5#diff-fbce17c4cb97f431da78cbdf3ac4ff16R90)

Change History (6)

comment:1 by Claude Paroz, 9 years ago

Has patch: set
Needs tests: set
Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

Thanks for noticing that regression. Could you please add a test to the patch?

comment:2 by Daniel Hahler, 9 years ago

Where should the test go?

Last edited 9 years ago by Daniel Hahler (previous) (diff)

comment:3 by Tim Graham, 9 years ago

A new file called tests/gis_tests/test_backends.py seems reasonable to me.

comment:4 by Daniel Hahler, 9 years ago

It might make sense to add a test for this for all backends?
(with pytest I could imagine using a parametrized test for this - which would access an existing and non-existing attribute).
Can this be done in Django's testing framework?

Apart from that, I think that it would be better/faster if somebody who is more used to Django's tests would add it.

(I am more used to tests based on pytest, and to be honest: the lack of proper attribution (in terms of Git authorship) for (several, but not all) previous patches from me makes me less motivated to go the last mile here, although that seems to get handled better by now - just saying).

comment:5 by Tim Graham, 9 years ago

I think writing the test for the current backend being tested (e.g. checking hasattr(connection.features, 'nonexistent') is enough.

comment:6 by Tim Graham <timograham@…>, 9 years ago

Resolution: fixed
Status: newclosed

In da5747f8:

Fixed #25400 -- Fixed regression in nonexistent features on gis backends.

Note: See TracTickets for help on using tickets.
Back to Top