Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#28105 closed Bug (fixed)

BaseGeometryWidget.get_context() crashes if attrs contains the name of an existing key

Reported by: Dylan Verheul Owned by: Dylan Verheul
Component: GIS Version: 1.11
Severity: Release blocker Keywords: gis, forms, widgets
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description (last modified by Tim Graham)

Incontrib.gis.forms.widgets there is this part (line 67 in current master):

context = self.build_attrs(self.attrs, dict(
    name=name,
    module='geodjango_%s' % name.replace('-', '_'),  # JS-safe
    serialized=self.serialize(value),
    geom_type=gdal.OGRGeomType(self.attrs['geom_type']),
    STATIC_URL=settings.STATIC_URL,
    LANGUAGE_BIDI=translation.get_language_bidi(),
    **attrs
))

If attrs also contains a key 'geom_type' this leads to an inevitable crash.

This should probaly be something like:

context_kwargs = attrs.copy()
context_kwargs.update(dict(
    name=name,
    module='geodjango_%s' % name.replace('-', '_'),  # JS-safe
    serialized=self.serialize(value),
    geom_type=gdal.OGRGeomType(self.attrs['geom_type']),
    STATIC_URL=settings.STATIC_URL,
    LANGUAGE_BIDI=translation.get_language_bidi(),
))

Currently this causes django-bootstrap3 to fail for Django 1.11.

Change History (12)

comment:1 Changed 3 years ago by Tim Graham

Component: FormsGIS
Description: modified (diff)
Severity: NormalRelease blocker
Summary: Missing check in django.contrib.forms.widgetsBaseGeometryWidget.get_context() crashes if attrs contains the name of an existing key
Triage Stage: UnreviewedAccepted

Could you explain more about the use case that causes the crash? If you could write a test for tests/gis_tests/test_geoforms.py, that would be ideal.

comment:2 Changed 3 years ago by Dylan Verheul

Owner: changed from nobody to Dylan Verheul
Status: newassigned

I'll see what I can do to fix it myself.

comment:3 Changed 3 years ago by Dylan Verheul

Easy pickings: set
Owner: Dylan Verheul deleted
Status: assignednew

The test suite keeps getting stuck on my Mac. Seems like this is easy pickings so I'll hope someone else picks it up.

comment:4 Changed 3 years ago by Tim Graham

You likely wouldn't need to run the entire test suite (see Running only some of the tests). Running gis_tests should be enough -- the pull request tester runs the entire suite.

On the mailing list you wrote, "My patch and tests are about done". If you don't continue, can you provide what you did so far?

Last edited 3 years ago by Tim Graham (previous) (diff)

comment:5 Changed 3 years ago by Dylan Verheul

I'm no stranger to pyenv, virtualenv, git and unit tests. I have followed https://docs.djangoproject.com/en/dev/internals/contributing/writing-code/working-with-git/ and https://docs.djangoproject.com/en/dev/internals/contributing/new-contributors/ and https://docs.djangoproject.com/en/dev/intro/contributing/.

Unfortunately the instructions for running the test suite end up in either a stuck test (have to abort it after several hours), essential tests skipped (because of sqlite instead of postgis I presume). I've sent quite a few hours because I really want to contribute, but it seems that it just does not work on my MacBook. A co-worker did get the test suite to run on his ubuntu machine. So, it's probably me.

I'm giving it a final try now. It's bothering me because the error is blatantly obvious.

comment:6 Changed 3 years ago by Tim Graham

If you copy tests/test_sqlite.py to tests/test_spatialite.py and change the 'ENGINE' in that file to 'django.contrib.gis.db.backends.spatialite', you should be able to run ./runtests.py --settings=test_spatialite gis_tests.test_geoforms. Ping me (timograham) in the #django-dev IRC channel if I can help further.

comment:7 Changed 3 years ago by Dylan Verheul

Perseverance paid of. Got the tests to work as well as I could, enough to make a testable PR

comment:8 Changed 3 years ago by Dylan Verheul

Owner: set to Dylan Verheul
Status: newassigned

comment:9 Changed 3 years ago by Dylan Verheul

comment:10 Changed 3 years ago by Dylan Verheul

Is there anything else for me to do now that the Pull Request has been committed?

comment:11 Changed 3 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In 75aeebeb:

Fixed #28105 -- Fixed crash in BaseGeometryWidget.get_context() when overriding existing attrs.

comment:13 Changed 3 years ago by Tim Graham <timograham@…>

In b1aea89:

[1.11.x] Fixed #28105 -- Fixed crash in BaseGeometryWidget.get_context() when overriding existing attrs.

Backport of 75aeebebfe3df3ea46b8e12dd5e7719f98664d3a from master

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