Skip to content
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

Filtering not applied on related object list views? #113

Closed
jonprindiville opened this issue Sep 20, 2019 · 5 comments · Fixed by #123
Closed

Filtering not applied on related object list views? #113

jonprindiville opened this issue Sep 20, 2019 · 5 comments · Fixed by #123
Assignees

Comments

@jonprindiville
Copy link
Member

Here's a bit of odd behaviour that I've encountered. I feel like this is in error, but will have to do a bit more digging to be sure.

In the following examples, g is a gapipy client with the only options passed in being an application key (the g.com key, in this case).


Reproducing

First off, if we fetch a tour_dossier and then access the related departures listing, it can tell us what the count is (from the count attr on the JSON responses):

In [3]: g.tour_dossiers.get(24821).departures.count()
Out[3]: 1247

If we apply some filter, the count changes appropriately:

In [4]: g.tour_dossiers.get(24821).departures.filter(start_date__gte='2019-09-20').count()
Out[4]: 672

If -- instead of calling .count on it -- we iterate the result of the filter call, we seem to get the full unfiltered set of departures:

In [5]: len([d for d in g.tour_dossiers.get(24821).departures.filter(start_date__gte='2019-09-20')])
Out[5]: 1247

😢 What happened to my filters? I would expect to see 672 items here, same as where we used .count. This is the source of my confusion.


Contrast with...

... what happens when we query the departures list view directly, rather than reaching through a tour_dossier:

Filter for all the departures associated with that tour_dossier and we get the same 1247 value:

In [6]: g.departures.filter(**{'tour_dossier.id': '24821'}).count()
Out[6]: 1247

If we add on our date filter and use .count, again we get 672:

In [7]: g.departures.filter(**{'tour_dossier.id': '24821', 'start_date__gte': '2019-09-20'}).count()
Out[7]: 672

And here, the expected behaviour -- iterating that filter expression and we encounter the same number of items as when .counting it -- seems like the filtering persists in this case:

In [8]: len([d for d in g.departures.filter(**{'tour_dossier.id': '24821', 'start_date__gte': '2019-09-20'})])
Out[8]: 672
@marz619
Copy link
Contributor

marz619 commented Apr 14, 2020

Fixed by #121 Not entirely fixed in #121

@marz619 marz619 closed this as completed Apr 14, 2020
@marz619 marz619 reopened this Apr 14, 2020
marz619 added a commit that referenced this issue Apr 20, 2020
* Fix for `2.26.0 (2020-04-14)`_ and `Issue #113`_
  * Calls to ``Query.all`` will use initialised its parameters, unless the URI
    provides its own.
  * See `PR 123 <https://github.com/gadventures/gapipy/pull/123>`_
* Add the ability to define the ``max_retries`` values on the requestor.
  * New ``env`` value ``GAPI_CLIENT_MAX_RETRIES``
  * The default value will be ``0``, and if provided will override the ``retry``
    value on the ``requests.Session``.
  * This change will also always initialize a ``requests.Session`` value on
    initialisation of the ``gapipy.Client``.
  * See `PR 124 <https://github.com/gadventures/gapipy/pull/124>`_
* Add ``variation_id`` field to the ``Image`` resource.
  * See `Commit edc8d9b <https://github.com/gadventures/gapipy/commit/edc8d9b>`_
* Update the ``ActivityDossier`` and ``AccommodationDOssier`` resources.
  * Remove the ``is_prepaid`` field
  * Adds the ``has_costs`` field
  * See `Commit bd35531 <https://github.com/gadventures/gapipy/commit/bd35531>`_

.. _`Issue #113`: #113
marz619 added a commit that referenced this issue Apr 20, 2020
* Fix for `2.26.0 (2020-04-14)`_ and `Issue #113`_.
  * Calls to ``Query.all`` will use initialised its parameters, unless the URI
  provides its own.
  * See `PR 123 <https://github.com/gadventures/gapipy/pull/123>`_.
* Add the ability to define the ``max_retries`` values on the requestor.
  * New ``env`` value ``GAPI_CLIENT_MAX_RETRIES``.
  * The default value will be ``0``, and if provided will override the ``retry``
  value on the ``requests.Session``.
  * This change will also always initialize a ``requests.Session`` value on
  initialisation of the ``gapipy.Client``.
  * See `PR 124 <https://github.com/gadventures/gapipy/pull/124>`_.
* Add ``variation_id`` field to the ``Image`` resource.
  * See `Commit edc8d9b <https://github.com/gadventures/gapipy/commit/edc8d9b>`_.
* Update the ``ActivityDossier`` and ``AccommodationDOssier`` resources.
  * Remove the ``is_prepaid`` field.
  * Adds the ``has_costs`` field.
  * See `Commit bd35531 <https://github.com/gadventures/gapipy/commit/bd35531>`_.

.. _`Issue #113`: #113
@marz619 marz619 self-assigned this Apr 20, 2020
@jonprindiville
Copy link
Member Author

👋 @marz619 I see you closed this -- thanks for following up, but I think that some problems remain.

I just wanted to follow up to confirm that the issue had been fixed, but I can't get through the reproduction instructions at all. Can you?

I've got gapipy==2.26.1 installed (and the latest version of the private repo as well) and then I try the following...

from gapipy_private import Client
g = Client(application_key='probably should not matter, but i am using a g.com key')
g.tour_dossiers.get(24821).departures.count()

... but I get an AttributeError: 'tuple' object has no attribute 'uri' at line 184 of gapipy.request:

self.parent.uri,


That g.tour_dossiers.get(24821).departures.count() call wasn't meant to indicate whether this issue was fixed or not, it was just meant to establish context for the other filtering calls done later on in the reproduction steps... so, hard for me to say whether the issue has been fixed or not.

@jonprindiville
Copy link
Member Author

I see in your PR you created a namedtuple for that parent arg, but in my stacktrace I get a plain old tuple ... maybe related to this parent= stuff?

parent = ('tours', self.id, None)
query = Query(self._client, resource_cls, parent=parent, raw_data=value)

@jonprindiville
Copy link
Member Author

I should add: I see the same failure when I do from gapipy import Client as when I do from gapipy_private import Client -- not sure why I used private in my example.

@marz619
Copy link
Contributor

marz619 commented Apr 20, 2020

@jonprindiville yup, adding the namedtuple broke the way the code functions. 🤦

We can remove that overriding behaviour in tour_dossier.py as we no longer use Tours as the base for querying against departures. My guess is this code was for a pre-Sieve era.

@marz619 marz619 reopened this Apr 20, 2020
marz619 added a commit that referenced this issue Apr 20, 2020
1. Adding the _Parent namedtuple in models.base broke this existing
   behaviour when Querying for departures under a tour-dossier

We can remove this, as 1) Tours are deprecated & 2) We use Sieve to
paginate through the departures resources under a tour-dossier.
marz619 added a commit that referenced this issue Apr 20, 2020
Fix buggy behaviour from 2.26.1 & Issue #113

* Remove the _set_resource_collection_field method from the TourDossier
  class. It is no longer needed, and was broken by introducing the
  _Parent namedtuple in #123
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants