New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Amend model_attr resolution #1482

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
1 participant
@wylee

wylee commented Feb 7, 2017

This amends the model_attr resolution changes made in 2a450d8 so they are backward compatible. Further details can be found in 12ee434.

wylee added some commits Feb 7, 2017

Amend SearchField model_attr resolution
Commit 2a450d8 introduced some changes
to how `model_attr` resolution works. It appears that the main intent of
those changes was to support model relationships. The implementation,
though, is backward-incompatible in at least a couple of ways.

I created issue #1411 to address the issue of not being able to retrieve
a named attribute from an iterable object. It looks like others have run
into the same issue (e.g., see #1441).

2a450d8 introduced another issue, which is noted in an inline comment on
the commit on GitHub. The issue is that when a related field is nullable
a list is returned instead of None as would be expected.

IMO, the implementation in 2a450d8 is also hard to understand because it
uses recursion unnecessarily.

This patch addresses these issues by:

- Prioritizing a named attribute if the current object is an iterable
- Only descending into an iterable if the named attribute doesn't exist
- Simplifying the implementation by disusing recursion, which also fixes
  the issue with lists being returned for nullable fields

In addition, this patch attempts to make detection of relationships more
robust by using `isinstance(obj, BaseManager)` instead of
`hasattr(obj, 'all')` and `ismethod(obj.all)`.

This patch has been tested against:

- Elasticsearch 1.7
- All supported versions of Python except PyPy
- All supported versions of Django
- A couple of Django projects I'm currently working on

As an example of the issue with iterable fields, consider the following
Model and SearchIndex:

    class Point(models.Model):

        name = models.CharField()

        # geom has x and y attributes AND is iterable
        # iterating over geom yields x and y
        geom = models.PointField()

    class PointIndex(indexes.SearchIndex, indexes.Indexable):

        name = indexex.CharField(model_attr='name')
        x = indexes.FloatField(model_attr='geom__x', indexed=False)
        y = indexes.FloatField(model_attr='geom__y', indexed=False)

The implementation in 2a450d8 errors out when attempting to index the
`x` and `y` fields, because accessing `geom` yields `(x, y)` instead of
the `geom` object.

As an example of the issue with nullable related fields, consider the
following Models and SearchIndex:

    class Classification(models.Model):

        name = models.CharField()

    class Article(models.Model):

        title = models.CharField()
        classification = models.ForeignKey(Classification, null=True, related_name='articles')

    class ArticleIndex(indexes.SearchIndex, indexes.Indexable):

        title = indexes.CharField()
        classification = indexes.IntegerField(model_attr='classification__id', indexed=False, null=True)

The implementation in 2a450d8 errors out when attempting to index the
`classification` field, because accessing `classification ` yields `[]`
instead of `None`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment