Filtering on the deleted field is done on evaluation.

[AndreasBackx]

See #64 for extra information (this PR is branched from #64), but here's a summary:

• SafeDeleteMixin.refresh_from_db works if visibility is set to DELETED_VISIBLE_BY_FIELD.
• SafeDeleteMixin.objects.filter(pk={pk}).get() does not give a DoesNotExist if visibility is set to DELETED_VISIBLE_BY_FIELD. This is why refresh_from_db works now. If you have any idea on how to make this work for the other ones, feel free to leave some feedback. I cannot come up with a good solution as they would have race conditions.
• I initially thought that SafeDeleteMixin.objects.select_related('something').get(pk={pk}) would ignore the select_related because get would create a new queryset. It apparently didn't in my initial tests which still boggles me. Anyhow, added tests assures it doesn't now.
• All QuerySet specific methods moved to SafeDeleteQueryset.

So the filter(deleted__isnull=True/False) is added when the QuerySet is evaluated. See __getattribute__ which intercepts those calls to add it. Would love some feedback, the added code has got documentation so check those too.

[AndreasBackx]

Might be better if we not deprecate this, but add helper methods for all visibility modifiers. We should perhaps add a note with rst stating that this should only be used for related fields.

[AndreasBackx]

This needs to be documented in the docs for when using custom QuerySets. When working with the QuerySet's query directly that hasn't had the deleted filter applied yet, filter_visibility needs to be called first.

[coveralls]

Coverage increased (+0.1%) to 97.974% when pulling 5d15cef on AndreasBackx:enhancement/queryset-reuse into ec0f547 on makinacorpus:master.

[coveralls]

Coverage increased (+0.1%) to 97.974% when pulling 5d15cef on AndreasBackx:enhancement/queryset-reuse into ec0f547 on makinacorpus:master.

[AndreasBackx]

This and #64 are ready to be merged if everything looks good to you. I moved the SafeDeleteQueryset to queryset.py and added docs there too. It remains backwards compatible and can still be imported with from safedelete.managers import SafeDeleteQueryset.

[Gagaro]

Thanks a lot 👍 . If you could just add a changelog in the CHANGES file it would be perfect!

[Gagaro]

So I should close #64 and merge #65, right?

[AndreasBackx]

@Gagaro I don't really mind how you merge it. Might be easier to close the first one and refer to this one saying that this one is merged. But if you still want to merge both, I guess you should be fine if you don't create a merge commit.

I'll add the changes to the changelog tonight. Do you want a version bump then...?

[Gagaro]

Don't worry about the version, I'll take care of it if I release it.

[glemmaPaul]

Hey guys!

It seems that this code change gave me an issue, as following:

User.objects.filter(email="paul@email.nl").first()

Gives me:

File "/Users/pauloostenrijk/webprojects/app/app/users/import_export/resources.py", line 57, in before_import
user = User.objects.all().filter(email=row['email']).first()
File "/Users/pauloostenrijk/.virtualenvs/app/lib/python2.7/site-packages/django/db/models/query.py", line 550, in first
objects = list((self if self.ordered else self.order_by('pk'))[:1])
File "/Users/pauloostenrijk/.virtualenvs/app/lib/python2.7/site-packages/django/db/models/query.py", line 258, in __iter__
self._fetch_all()
File "/Users/pauloostenrijk/.virtualenvs/app/lib/python2.7/site-packages/safedelete/queryset.py", line 121, in decorator
self._filter_visibility()
File "/Users/pauloostenrijk/.virtualenvs/app/lib/python2.7/site-packages/safedelete/queryset.py", line 98, in _filter_visibility
"Cannot filter a query once a slice has been taken."
AssertionError: Cannot filter a query once a slice has been taken.

[glemmaPaul]

Looking at the source code:

        """Methods that do not return a QuerySet should call ``_filter_visibility`` first."""
        attr = object.__getattribute__(self, name)
        # These methods evaluate the queryset and therefore need to filter the
        # visiblity set.
        evaluation_methods = (
            '_fetch_all', 'count', 'exists', 'aggregate', 'update', '_update',
            'delete', 'undelete',
        )
        if hasattr(attr, '__call__') and name in evaluation_methods:
            def decorator(*args, **kwargs):
                self._filter_visibility()
                return attr(*args, **kwargs)
            return decorator
        return attr

Removing _fetch_all from evaluation_methods fixes the issue, but not sure if that breaks anything else.

[AndreasBackx]

Could first be added here? I might have overlooked something when _fetch_all gets called. I used _fetch_all because it gets called in a lot of functions, but I'd have to look at it again. I think it's trying to filter the visibility after you call first when it should filter it before.

[glemmaPaul]

Let me try that right away @AndreasBackx

[glemmaPaul]

tried to add first evaluation_methods, but that didn't help :/

[glemmaPaul]

Wow 🎉
Looked into the code of django.models.query and saw it hit _fetch_all, upon looking at the method it looks like this:

def _fetch_all(self):
        if self._result_cache is None:
            self._result_cache = list(self.iterator())
        if self._prefetch_related_lookups and not self._prefetch_done:
            self._prefetch_related_objects()

So, fetching all if cached just by the pervious iterator, otherwise prefetch related objects. Prefetching usually means that it's a sliced queryset (correct me if I'm wrong). Changing the evaluation_methods _fetch_all to _prefetch_related_objects worked for me.

[glemmaPaul]

Wait that didn't do it, but there is something else that happened. Our UserManager is extending get_queryset to add some prefetch_related and select_related, removing that together with removing _fetch_all replacing it with _prefetch_related_objects works.

Is there a way around that issue?

[AndreasBackx]

Please create a PR with a test case that clearly shows your problem. We can work on the PR and fix the problem without breaking any of the other tests.

[AndreasBackx]

@glemmaPaul could you perhaps make a PR with a test that fails?