Skip to content

Commit

Permalink
upgrade HStore
Browse files Browse the repository at this point in the history
Overview
--------

The point of this commit is to switch from
`django_hstore.fields.DictionaryField` to
`django.contrib.postgres.fields.HStoreField`.

The former is incompatible with later releases of django,
and OpenTreeMap needs to migrate from the current 1.8 to 1.11
for stability.

The latter is the sanctioned django interface to PostgreSQL's hstore
going forward.

The latter is also less functional than the former, prompting a series
of other changes.
Changes were also made for the sake of maintenance simplicity.

The key changes here are in `treemap.udf`, `treemap.audit`, `*.models`, and
`treemap.search`.

**treemap.udf**

Entry points:
-   `UDFModel`, a base class for models that need user defined fields
-   `UserDefinedFieldDefinition`, defines the name, data type, and model type
    for a user defined field, for a given treemap instance, as a
    scalar or a collection
-   `UserDefinedCollectionValue`, an HStore collection value for a
    specific `UserDefinedFieldDefinition` on a specific model instance

`UDFModel` user guide:

Classes that want custom fields subclass `UDFModel`.
The `UDFModel` `HStoreField` is named `hstore_udfs`, but unless you are
getting deep into udf code, you should use `udfs`, which is an ordinary
attribute on the model instance, not directly associated with any
django `Field`.

To assign a custom field,
    `my_model_instance.udfs['custom field name'] = value`

There are three ways to retrieve it:
    `my_model_instance.udfs['custom field name']`
    `getattr(my_model_instance, 'custom field name')`
    `getattr(my_model_instance, 'udf:custom field name'`)

All of the above work for both scalar and collection custom fields.

Filter using the `hstore_udf` transform.

In addition to the transforms and lookups described at
https://docs.djangoproject.com/en/1.8/ref/contrib/postgres/fields/,
`UDFModel` implements `__int` and `__float` transforms, for use
before magnitude comparisons (`__gt`, `__gte`, `__lt`, `__lte`).

`udfs` perpetually syncs to `hstore_udfs`.
Values in `udfs` are always typed for the app, and
values in `hstore_udfs` are always typed for the db.
Since they are perpetually clean, code that assigns to `udfs`
needs to be prepared for a `ValidationError` at the time of the
assignment, not just at `save`.

In practice, this is not a problem, because the web frontend
takes care to send valid data in PUTs and POSTs.

**treemap.audit and \*.models**

- Models can use a plain `GeoManager` now
- In order to reduce code coupling and assure that initialization happens in a
  rational order, `populate_previous_state` is now the responsibility of the
  leaf subclass of `UserTrackable`.
- Species is no longer a `UDFModel` because it never employed udfs,
  and removing unused code makes maintenance simpler.

Before this change, `populate_previous_state` was getting called between
initialization of different levels of model class. It calls `as_dict`, which
raises exceptions if not all fields have been initialized,
so now `populate_previous_state` is the responsibility of the leaf subclass.

This reduces the coupling between auditing and custom fields.

That tight coupling was why previous versions of `udfs.py` had to
inject a descriptor into the django model instantiation procedure.
Descriptors are fine as long as they work, but when they don't, they
are a headache to debug. Better to reduce the amount of injection
into django's procedure.

**treemap.search**

- Use the new `hstore_udf` filter transform
- Additional parser steps to add the udf `__float` filter transform

Specific Changes
----------------
- Replace UDF specific GeoHStoreUDFManager with generic
  GeoManager in treemap and stormwater models
- Add django.contrib.postgres to settings
- Migration to give the `udfs` column a default
- Migration to demote Species from UDFModel
- `treemap.udf`
  - Eliminate
    - UDF specific manager and descriptor
    - `quotesingle`
    - `UDF_ORDER_PATTERN`
    - `UDFBaseContains(Lookup)`
    - `UDFContains` and `UDFIcontains`
    - `UDFQuery(Query)`
    - `UDFQuerySet(HStoreGeoQuerySet)`
    - `GeoHStoreUDFManager(HStoreGeoManager)`
    - unused `'user'` datatype
  - Alias `UDFField` to `HStoreField` for the sake of earlier migrations
  - Rename the `HStoreField` to `hstore_udfs` with a column param
    to point it at the `udfs` column in `treemap_mapfeature` table
  - Dynamically make a `udfs` model instance attribute in `UDFModel.__init__`
  - Migration to support the above
    - The auto-generated migration used `RemoveField` and `AddField`,
      which lost all the data so the migration here has been hacked to use
      `RenameField` and `AlterField`, and tested for data retention.
  - `UserDefinedCollectionValue.data` now based on
    `django.contrib.postgres.fields.HStoreField`
  - If a collection field choice is removed,
    delete all UDCVs that name that choice,
    not just the choice field in the UDCV
  - Use `feature_type` attribute if it exists, because the only classname
    available might be the generic `MapFeature`.
  - Have a stackoverflow reference in case ordering
    by an hstore key ever becomes important
  - Perhaps it's a design flaw that `udfs` perpetually cleans and
    `hstore_udfs` perpetually reverse cleans, and both should be
    delayed to `save_with_user`, but there are disadvantages to
    allowing dirty values, too.
  - Transforms based on
    `django.contrib.postgres.fields.hstore.KeyTransform`,
    which is registered on `HStoreField`
- Search now works for both collection and scalar UDFs
- `treemap.search`
  - Removed 'IN' and 'ISNULL' predicates from collection search
    because they are never used in production
  - Removed 'WITHIN_RADIUS' predicate from scalar search
    because it is never used in production
- Removed tests that no longer apply, or that test code that
  is never used in production
- Modified integer and float udf tests so that they fail if the emitted
  SQL does a lexical comparison to demonstrate the problem
- Add a `'multichoice'` test to `test_udfs`
- Fix form_extras to tolerate a missing UDF key
- Test for `is None` wherever a falsey value could be legitimate
- Adapt `treemap.views.map_feature.update_map_feature` to include
  `ValidationError` `message_dict`s from setting a udf to an invalid value
  in its `errors`.

Testing Notes
-------------
- Combines well with other searches
- ... unless you are bitten by #3080

--

Connects to #1968
  • Loading branch information
RobinIsTheBird committed May 15, 2017
1 parent 87b715d commit b9ea43a
Show file tree
Hide file tree
Showing 21 changed files with 1,074 additions and 1,249 deletions.
2 changes: 1 addition & 1 deletion opentreemap/api/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -1072,7 +1072,7 @@ def _create_multichoice(self):
def test_update_multichoice_v3(self):
self._create_multichoice()
test_plot = mkPlot(self.instance, self.user)
test_plot.udfs['multi'] = None
test_plot.udfs['multi'] = []
test_plot.save_with_user(self.user)

updated_values = {'plot': {
Expand Down
13 changes: 7 additions & 6 deletions opentreemap/importer/models/trees.py
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,6 @@ class TreeImportRow(GenericImportRow):
'geom': fields.trees.POINT,
'width': fields.trees.PLOT_WIDTH,
'length': fields.trees.PLOT_LENGTH,
# TODO: READONLY restore when implemented
# 'readonly': fields.trees.READ_ONLY,
'owner_orig_id': fields.trees.EXTERNAL_ID_NUMBER,
'address_street': fields.trees.STREET_ADDRESS,
'address_city': fields.trees.CITY_ADDRESS,
Expand All @@ -106,8 +104,6 @@ class TreeImportRow(GenericImportRow):
'species': fields.trees.SPECIES_OBJECT,
'date_planted': fields.trees.DATE_PLANTED,
'date_removed': fields.trees.DATE_REMOVED,
# TODO: READONLY restore when implemented
# 'readonly': fields.trees.READ_ONLY
}

# plot that was created from this row
Expand Down Expand Up @@ -236,7 +232,8 @@ def _commit_tree_data(self, data, plot, tree, tree_edited):
for udf_def in tree_udf_defs:
udf_column_name = ie.get_udf_column_name(udf_def)
value = data.get(udf_column_name, None)
if value:
# Legitimate values could be falsey
if value is not None:
tree_edited = True
if tree is None:
tree = Tree(instance=plot.instance)
Expand Down Expand Up @@ -416,8 +413,12 @@ def validate_user_defined_fields(self):
udf_def.clean_value(value)
self.cleaned[column_name] = value
except ValidationError as ve:
message = str(ve)
if isinstance(ve.message_dict, dict):
message = '\n'.join(
[unicode(m) for m in ve.message_dict.values()])
self.append_error(
errors.INVALID_UDF_VALUE, column_name, str(ve))
errors.INVALID_UDF_VALUE, column_name, message)

def validate_row(self):
"""
Expand Down
7 changes: 0 additions & 7 deletions opentreemap/importer/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -1344,11 +1344,6 @@ def test_all_tree_data(self):

self.assertEqual(tree2.species.pk, s1_gsc.pk)

# TODO: READONLY restore when implemented
# csv = """
# | point x | point y | date planted | read only |
# | 25.00 | 25.00 | 2012-02-03 | true |
# """
csv = """
| point x | point y | date planted |
| 25.00 | 25.00 | 2012-02-03 |
Expand All @@ -1361,8 +1356,6 @@ def test_all_tree_data(self):
dateplanted = date(2012, 2, 3)

self.assertEqual(tree.date_planted, dateplanted)
# TODO: READONLY restore when implemented
# self.assertEqual(tree.readonly, True)

psycopg2.extras.register_hstore(connection.cursor(), globally=True)

Expand Down
3 changes: 2 additions & 1 deletion opentreemap/manage_treemap/js/src/udfs.js
Original file line number Diff line number Diff line change
Expand Up @@ -437,7 +437,8 @@ editForm.inEditModeProperty.onValue(function() {

function addNewUdf(html) {
var $created = $(html),
$model = $(dom.udfs).find('[data-model-type="' + $created.data('model-type') + '"]'),
model_type = $created.data('model-type'),
$model = $(dom.udfs).find('tbody[data-model-type="' + model_type + '"]'),
$collections = $model.find(dom.udfContainer + '[data-is-collection="yes"]');

if (0 < $collections.length) {
Expand Down
2 changes: 1 addition & 1 deletion opentreemap/manage_treemap/views/udf.py
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ def udf_context(instance, udf_def):
else:
Model = safe_get_udf_model_class(udf_def.model_type)
udf_uses = Model.objects.filter(instance=instance)\
.filter(udfs__contains=[udf_def.name])\
.filter(hstore_udfs__has_key=udf_def.name)\
.count()

return {
Expand Down
4 changes: 2 additions & 2 deletions opentreemap/opentreemap/settings/default_settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@

# List of callables that know how to import templates from various sources.
TEMPLATE_LOADERS = (
#'django.template.loaders.filesystem.Loader',
# 'django.template.loaders.filesystem.Loader', # NOQA
'django.template.loaders.app_directories.Loader',
'apptemplates.Loader'
)
Expand Down Expand Up @@ -292,7 +292,7 @@
'django.contrib.admin',
'django.contrib.gis',
'django.contrib.humanize',
'django_hstore',
'django.contrib.postgres',
'djcelery',
'url_tools',
'django_js_reverse',
Expand Down
17 changes: 11 additions & 6 deletions opentreemap/otm1_migrator/data_util.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import dateutil.parser

from treemap.models import User
from treemap.audit import model_hasattr

from treemap.util import to_model_name
from treemap.lib import udf as udf_lib
Expand Down Expand Up @@ -52,17 +51,23 @@ def dict_to_model(config, model_name, data_dict, instance):
common_fields = config[model_name].get('common_fields', set())
renamed_fields = config[model_name].get('renamed_fields', {})
dependency_fields = config[model_name].get('dependencies', {})
instance_field = None

ModelClass = config[model_name].get('model_class')

if ModelClass is None:
return PROCESS_WITHOUT_SAVE
else:
model = ModelClass()

# instance *must* be set before UDF assignment
if model_hasattr(model, 'instance'):
model.instance = instance
try:
instance_field = ModelClass._meta.get_field('instance')
except:
pass
# instance *must* be set during initialization
model = ModelClass() \
if instance_field is None or \
instance_field.many_to_many or \
instance_field.one_to_many \
else ModelClass(instance=instance)

for field in (common_fields
.union(renamed_fields)
Expand Down
11 changes: 5 additions & 6 deletions opentreemap/stormwater/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

from stormwater.benefits import PolygonalBasinBenefitCalculator
from treemap.decorators import classproperty
from treemap.models import MapFeature, GeoHStoreUDFManager, ValidationMixin
from treemap.models import MapFeature, ValidationMixin
from treemap.ecobenefits import CountOnlyBenefitCalculator


Expand All @@ -18,7 +18,7 @@ class PolygonalMapFeature(MapFeature):

polygon = models.MultiPolygonField(srid=3857)

objects = GeoHStoreUDFManager()
objects = models.GeoManager()

@classproperty
def always_writable(cls):
Expand All @@ -27,7 +27,6 @@ def always_writable(cls):
def __init__(self, *args, **kwargs):
super(PolygonalMapFeature, self).__init__(*args, **kwargs)
self._do_not_track |= self.do_not_track
self.populate_previous_state()

@classproperty
def do_not_track(cls):
Expand Down Expand Up @@ -64,7 +63,7 @@ def calculate_area(self):


class Bioswale(PolygonalMapFeature, ValidationMixin):
objects = GeoHStoreUDFManager()
objects = models.GeoManager()
drainage_area = models.FloatField(
null=True,
blank=True,
Expand Down Expand Up @@ -126,7 +125,7 @@ def clean(self):


class RainGarden(PolygonalMapFeature, ValidationMixin):
objects = GeoHStoreUDFManager()
objects = models.GeoManager()
drainage_area = models.FloatField(
null=True,
blank=True,
Expand Down Expand Up @@ -188,7 +187,7 @@ def clean(self):


class RainBarrel(MapFeature):
objects = GeoHStoreUDFManager()
objects = models.GeoManager()
capacity = models.FloatField(
verbose_name=_("Capacity"),
error_messages={'invalid': _("Please enter a number.")})
Expand Down
16 changes: 9 additions & 7 deletions opentreemap/treemap/audit.py
Original file line number Diff line number Diff line change
Expand Up @@ -541,7 +541,10 @@ def __init__(self, *args, **kwargs):
# with any fields that are added during instance initialization.
self._do_not_track = self.do_not_track
super(UserTrackable, self).__init__(*args, **kwargs)
self.populate_previous_state()

# It is the leaf class' responsibility to call
# `self.populate_previous_state()` after initialization is
# otherwise complete.

def apply_change(self, key, orig_value):
# TODO: if a field has a default value, don't
Expand Down Expand Up @@ -955,7 +958,7 @@ def save_with_user(self, user, *args, **kwargs):
for field in self._updated_fields():
if field not in self._get_writable_perms_set(user):
raise AuthorizeException("Can't edit field %s on %s" %
(field, self._model_name))
(field, self._model_name))

# If `WRITE_WITH_AUDIT` (i.e. pending write) is resurrected,
# this test will prevent `_PendingAuditable` from getting called
Expand Down Expand Up @@ -1206,8 +1209,7 @@ def save_with_user(self, user, auth_bypass=False, *args, **kwargs):

is_insert = self.pk is None

if ((self.user_can_create(user)
or not is_insert or auth_bypass)):
if ((self.user_can_create(user) or not is_insert or auth_bypass)):
# Auditable will make the audits for us (including pending audits)
return super(_PendingAuditable, self).save_with_user(
user, updates=updates, *args, **kwargs)
Expand Down Expand Up @@ -1382,8 +1384,8 @@ def _deserialize_value(self, value):
datatype = udf_def.datatype_by_field[field_name]
else:
udf_def = next((udfd for udfd in udfds
if udfd.name == field_name
and udfd.model_type == self.model), None)
if udfd.name == field_name and
udfd.model_type == self.model), None)
if udf_def is not None:
datatype = udf_def.datatype_dict
if udf_def is not None:
Expand All @@ -1395,7 +1397,7 @@ def _deserialize_value(self, value):

cls = get_auditable_class(self.model)
field_query = cls._meta.get_field_by_name(self.field)
field_cls, fk_model_cls, is_local, m2m = field_query
field_cls, __, __, __ = field_query
field_modified_value = field_cls.to_python(value)

# handle edge cases
Expand Down
18 changes: 18 additions & 0 deletions opentreemap/treemap/migrations/0043_species_not_udf_model.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# -*- coding: utf-8 -*-
from __future__ import unicode_literals

from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
('treemap', '0043_udfs_default'),
]

operations = [
migrations.RemoveField(
model_name='species',
name='udfs',
),
]
11 changes: 7 additions & 4 deletions opentreemap/treemap/migrations/0043_udfs_default.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# -*- coding: utf-8 -*-
from __future__ import unicode_literals

from django.db import migrations, models
from django.db import migrations
import treemap.udf


Expand All @@ -15,16 +15,19 @@ class Migration(migrations.Migration):
migrations.AlterField(
model_name='mapfeature',
name='udfs',
field=treemap.udf.UDFField(default={}, db_index=True, blank=True),
field=treemap.udf.UDFField(default=lambda: {},
db_index=True, blank=True),
),
migrations.AlterField(
model_name='species',
name='udfs',
field=treemap.udf.UDFField(default={}, db_index=True, blank=True),
field=treemap.udf.UDFField(default=lambda: {},
db_index=True, blank=True),
),
migrations.AlterField(
model_name='tree',
name='udfs',
field=treemap.udf.UDFField(default={}, db_index=True, blank=True),
field=treemap.udf.UDFField(default=lambda: {},
db_index=True, blank=True),
),
]
44 changes: 44 additions & 0 deletions opentreemap/treemap/migrations/0044_hstorefield.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
# -*- coding: utf-8 -*-
from __future__ import unicode_literals

from django.db import migrations
import django.contrib.postgres.fields.hstore


class Migration(migrations.Migration):

dependencies = [
('treemap', '0043_species_not_udf_model'),
]

operations = [
migrations.RenameField(
model_name='mapfeature',
old_name='udfs',
new_name='hstore_udfs',
),
migrations.AlterField(
model_name='mapfeature',
name='hstore_udfs',
field=django.contrib.postgres.fields.hstore.HStoreField(
default=lambda: {},
db_index=True, db_column='udfs', blank=True),
),
migrations.RenameField(
model_name='tree',
old_name='udfs',
new_name='hstore_udfs',
),
migrations.AlterField(
model_name='tree',
name='hstore_udfs',
field=django.contrib.postgres.fields.hstore.HStoreField(
default=lambda: {},
db_index=True, db_column='udfs', blank=True),
),
migrations.AlterField(
model_name='userdefinedcollectionvalue',
name='data',
field=django.contrib.postgres.fields.hstore.HStoreField(),
),
]

0 comments on commit b9ea43a

Please sign in to comment.