From d12325e6b26791f498e7e1bbecfe1602645909de Mon Sep 17 00:00:00 2001 From: Tim DiLauro Date: Thu, 27 Jul 2023 12:47:35 -0400 Subject: [PATCH 1/3] Correctly handle "All" special case for "distributor" / "collectionName" lane search facets. (#1299) * Handle "All" case for `distributor` and `collectionName`. Also, add some context to the respective problem details for each. * Return the ProblemDetail, if we got one. * Add test for bug and a few other cases. --- api/controller.py | 2 +- core/lane.py | 24 +++++++---- tests/api/test_controller_opdsfeed.py | 57 ++++++++++++++++++++++++++- 3 files changed, 74 insertions(+), 9 deletions(-) diff --git a/api/controller.py b/api/controller.py index 914860ad5a..9955907043 100644 --- a/api/controller.py +++ b/api/controller.py @@ -1136,7 +1136,7 @@ def search(self, lane_identifier, feed_class=AcquisitionFeed): facets = self._load_search_facets(lane) if isinstance(facets, ProblemDetail): - return lane + return facets search_engine = self.search_engine if isinstance(search_engine, ProblemDetail): diff --git a/core/lane.py b/core/lane.py index 481620be33..8b719241a0 100644 --- a/core/lane.py +++ b/core/lane.py @@ -4,7 +4,7 @@ import logging import time from collections import defaultdict -from typing import TYPE_CHECKING, List, Optional +from typing import TYPE_CHECKING, Any, List, Optional from urllib.parse import quote_plus from flask_babel import lazy_gettext as _ @@ -422,7 +422,7 @@ def available_facets(cls, config, facet_group_name): that might not be enabled in library configuration, but you can't make up totally new facets. - TODO: This sytem would make more sense if you _could_ make up + TODO: This system would make more sense if you _could_ make up totally new facets, maybe because each facet was represented as a policy object rather than a key to code implemented elsewhere in this class. Right now this method implies more @@ -448,7 +448,9 @@ def default_facet(cls, config, facet_group_name): return config.default_facet(facet_group_name) @classmethod - def _values_from_request(cls, config, get_argument, get_header): + def _values_from_request( + cls, config, get_argument, get_header + ) -> dict[str, Any] | ProblemDetail: g = Facets.ORDER_FACET_GROUP_NAME order = get_argument(g, cls.default_facet(config, g)) order_facets = cls.available_facets(config, g) @@ -484,10 +486,14 @@ def _values_from_request(cls, config, get_argument, get_header): g = Facets.DISTRIBUTOR_FACETS_GROUP_NAME distributor = get_argument(g, cls.default_facet(config, g)) distributor_facets = cls.available_facets(config, g) - if distributor and distributor not in distributor_facets: + if ( + distributor + and distributor != "All" + and distributor not in distributor_facets + ): return INVALID_INPUT.detailed( _( - "I don't understand what '%(distributor)s' refers to.", + "I don't understand which distributor '%(distributor)s' refers to.", distributor=distributor, ), 400, @@ -496,10 +502,14 @@ def _values_from_request(cls, config, get_argument, get_header): g = Facets.COLLECTION_NAME_FACETS_GROUP_NAME collection_name = get_argument(g, cls.default_facet(config, g)) collection_name_facets = cls.available_facets(config, g) - if collection_name and collection_name not in collection_name_facets: + if ( + collection_name + and collection_name != "All" + and collection_name not in collection_name_facets + ): return INVALID_INPUT.detailed( _( - "I don't understand what '%(collection_name)s' refers to.", + "I don't understand which collection '%(collection_name)s' refers to.", collection_name=collection_name, ), 400, diff --git a/tests/api/test_controller_opdsfeed.py b/tests/api/test_controller_opdsfeed.py index 4b0a1ab806..4a9c2fb6f3 100644 --- a/tests/api/test_controller_opdsfeed.py +++ b/tests/api/test_controller_opdsfeed.py @@ -567,7 +567,7 @@ def search(cls, **kwargs): library_short_name=library.short_name, **dict(list(facets.items())), q=query, - _external=True + _external=True, ) assert expect_url == kwargs.pop("url") @@ -606,6 +606,61 @@ def search(cls, **kwargs): ) assert self.called_with["facets"].search_type == "json" + def test_lane_search_params( + self, + circulation_fixture: CirculationControllerFixture, + ): + # Tests some of the lane search parameters. + # TODO: Add test for valid `distributor`. + + valid_lane_id = circulation_fixture.english_adult_fiction.id + valid_collection_name = circulation_fixture.collection.name + invalid_collection_name = "__non-existent-collection__" + invalid_distributor = "__non-existent-distributor__" + + with circulation_fixture.request_context_with_library("/?collectionName=All"): + response = circulation_fixture.manager.opds_feeds.search(valid_lane_id) + assert 200 == response.status_code + assert "application/opensearchdescription+xml" == response.headers.get( + "content-type" + ) + + with circulation_fixture.request_context_with_library( + f"/?collectionName={valid_collection_name}" + ): + response = circulation_fixture.manager.opds_feeds.search(valid_lane_id) + assert 200 == response.status_code + assert "application/opensearchdescription+xml" == response.headers.get( + "content-type" + ) + + with circulation_fixture.request_context_with_library( + f"/?collectionName={invalid_collection_name}" + ): + response = circulation_fixture.manager.opds_feeds.search(valid_lane_id) + assert 400 == response.status_code + assert ( + f"I don't understand which collection '{invalid_collection_name}' refers to." + == response.detail + ) + + with circulation_fixture.request_context_with_library("/?distributor=All"): + response = circulation_fixture.manager.opds_feeds.search(valid_lane_id) + assert 200 == response.status_code + assert "application/opensearchdescription+xml" == response.headers.get( + "content-type" + ) + + with circulation_fixture.request_context_with_library( + f"/?distributor={invalid_distributor}" + ): + response = circulation_fixture.manager.opds_feeds.search(valid_lane_id) + assert 400 == response.status_code + assert ( + f"I don't understand which distributor '{invalid_distributor}' refers to." + == response.detail + ) + def test_misconfigured_search( self, circulation_fixture: CirculationControllerFixture ): From 8228bfde3f0747461a38c4ff9e2e202d85c47249 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 31 Jul 2023 16:32:57 +0000 Subject: [PATCH 2/3] Bump uwsgi from 2.0.21 to 2.0.22 (#1300) --- poetry.lock | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/poetry.lock b/poetry.lock index 1a75f4198f..596f1cc794 100644 --- a/poetry.lock +++ b/poetry.lock @@ -4005,12 +4005,12 @@ socks = ["PySocks (>=1.5.6,!=1.5.7,<2.0)"] [[package]] name = "uwsgi" -version = "2.0.21" +version = "2.0.22" description = "The uWSGI server" optional = false python-versions = "*" files = [ - {file = "uwsgi-2.0.21.tar.gz", hash = "sha256:35a30d83791329429bc04fe44183ce4ab512fcf6968070a7bfba42fc5a0552a9"}, + {file = "uwsgi-2.0.22.tar.gz", hash = "sha256:4cc4727258671ac5fa17ab422155e9aaef8a2008ebb86e4404b66deaae965db2"}, ] [[package]] From 13a0515883802ddbd7e36e60957514922644950a Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Tue, 1 Aug 2023 15:49:07 +0000 Subject: [PATCH 3/3] Bump pyparsing from 3.1.0 to 3.1.1 (#1301) --- poetry.lock | 8 ++++---- pyproject.toml | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/poetry.lock b/poetry.lock index 596f1cc794..2182d696e8 100644 --- a/poetry.lock +++ b/poetry.lock @@ -2993,13 +2993,13 @@ test = ["flaky", "pretend", "pytest (>=3.0.1)"] [[package]] name = "pyparsing" -version = "3.1.0" +version = "3.1.1" description = "pyparsing module - Classes and methods to define and execute parsing grammars" optional = false python-versions = ">=3.6.8" files = [ - {file = "pyparsing-3.1.0-py3-none-any.whl", hash = "sha256:d554a96d1a7d3ddaf7183104485bc19fd80543ad6ac5bdb6426719d766fb06c1"}, - {file = "pyparsing-3.1.0.tar.gz", hash = "sha256:edb662d6fe322d6e990b1594b5feaeadf806803359e3d4d42f11e295e588f0ea"}, + {file = "pyparsing-3.1.1-py3-none-any.whl", hash = "sha256:32c7c0b711493c72ff18a981d24f28aaf9c1fb7ed5e9667c9e84e3db623bdbfb"}, + {file = "pyparsing-3.1.1.tar.gz", hash = "sha256:ede28a1a32462f5a9705e07aea48001a08f7cf81a021585011deba701581a0db"}, ] [package.extras] @@ -4207,4 +4207,4 @@ testing = ["flake8 (<5)", "func-timeout", "jaraco.functools", "jaraco.itertools" [metadata] lock-version = "2.0" python-versions = ">=3.8,<4" -content-hash = "31f3da1d714f3330146f9423d35b0712fa9099b84e1b991fff439956f529cd04" +content-hash = "7600087ac13c7c72ff6fdbec8364c7e04780d06e80a9115c4483cb47ff393d7f" diff --git a/pyproject.toml b/pyproject.toml index fe461586f0..6e4c991d25 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -178,7 +178,7 @@ PyJWT = "2.6.0" PyLD = "2.0.3" pymarc = "5.1.0" pyOpenSSL = "^23.1.0" -pyparsing = "3.1.0" +pyparsing = "3.1.1" pyspellchecker = "0.7.2" python = ">=3.8,<4" python-dateutil = "2.8.2"