fix #333 - TagBase._default_manager for existant tag check #334

Merged
merged 1 commit into from Aug 8, 2015

Projects

None yet

3 participants

@groovecoder
Contributor

No description provided.

@frewsxcv
Collaborator
frewsxcv commented Aug 6, 2015

Looks good. I'll merge this later today and ship a new version when I get home. How difficult would it be to create a regression test?

@groovecoder
Contributor

I tried for a bit, but I'm not comfortable enough with the code-base to write it. 😢 I couldn't tell how to assert the behavior inside TagBase.save() was going thru the right flow.

@frewsxcv
Collaborator
frewsxcv commented Aug 6, 2015

That's alright, I understand. In case you missed it, flake8 is not happy about the style, stangely

@groovecoder
Contributor

tumblr_m8x5o6nzc71rdkg05o1_500

@jezdez jezdez commented on the diff Aug 6, 2015
taggit/models.py
@@ -68,8 +68,11 @@ def save(self, *args, **kwargs):
except IntegrityError:
pass
# Now try to find existing slugs with similar names
- slugs = set(Tag.objects.filter(slug__startswith=self.slug)
- .values_list('slug', flat=True))
+ slugs = set(
+ self._default_manager
@jezdez
jezdez Aug 6, 2015 Contributor

If I may suggest _default_manager should be accessed with the class not the instance as demonstrated in the majority of cases in Django's code base. If I understand correctly it has something to do how the default manager adds itself to the model class during import time and not the instance on creation time which in theory could be prevented if a Tag subclass were to fiddle with its class attributes during class or instance creation.

@groovecoder
groovecoder Aug 6, 2015 Contributor

Can you link to one of those? I'm not as familiar with django internals, and most of the ones I see with a simple search show model._default_manager.

@frewsxcv
frewsxcv Aug 8, 2015 Collaborator

If a decision here ever gets established, feel free to open another PR with the fix

@jezdez
jezdez Aug 8, 2015 Contributor

Ough I missed @groovecoder's comment. I'm away from the keyboard right now so can't link to all the cases in django's source code easily, but the model he refers to is a model class, not an instance of a model class. I'm not sure what else to say other than that if you want to use a private API like this (as it makes sense) I'd strongly suggest to follow the precedence to reduce the chance for surprises.

On 08 Aug 2015, at 16:32, Corey Farwell notifications@github.com wrote:

In taggit/models.py:

@@ -68,8 +68,11 @@ def save(self, _args, *_kwargs):
except IntegrityError:
pass
# Now try to find existing slugs with similar names

  •        slugs = set(Tag.objects.filter(slug__startswith=self.slug)
    
  •                               .values_list('slug', flat=True))
    
  •        slugs = set(
    
  •            self._default_manager
    
    If a decision here ever gets established, feel free to open another PR with the fix


Reply to this email directly or view it on GitHub.

@frewsxcv
Collaborator
frewsxcv commented Aug 8, 2015

Thanks for the PR @groovecoder !

@frewsxcv frewsxcv merged commit 7c44f90 into alex:develop Aug 8, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@frewsxcv
Collaborator
frewsxcv commented Aug 8, 2015

Released 0.16.3 that includes this change. Thanks again!

https://pypi.python.org/pypi/django-taggit/0.16.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment