Repository: h2non/pook

Issue:
`param_exists` fails when requested query param has no value
The following should work but does not:

```
pook.get("https://example.com").param_exists("x")
urlopen("https://example.com/?x")
```

This is because the query param matcher returns `false` here:

https://github.com/h2non/pook/blob/fac40e9f571152ba09bc16954548ce51d590ccea/src/pook/matchers/query.py#L14-L15

It should check `key in req_query` in addition to None. Falling back to an empty string in that method when `key in req_query and req_query[key] is None` _might_ be okay, but it could interfere with other expectations. It would be safer to augment `param_exists` to correctly handle the present-but-None case, potentially by using a simpler matcher class altogether instead of overloading the existing `QueryMatcher` and abusing the regex matching.

Discussion:

Patch:
diff --git a/src/pook/matchers/api.py b/src/pook/matchers/api.py
index 7bc0fb2..c1cfcf2 100644
--- a/src/pook/matchers/api.py
+++ b/src/pook/matchers/api.py
@@ -1,9 +1,9 @@
 from .base import BaseMatcher
 from .url import URLMatcher
 from .body import BodyMatcher
-from .query import QueryMatcher
+from .query import QueryMatcher, QueryParameterExistsMatcher
 from .method import MethodMatcher
-from .headers import HeadersMatcher
+from .headers import HeadersMatcher, HeaderExistsMatcher
 from .path import PathMatcher
 from .xml import XMLMatcher
 from .json import JSONMatcher
@@ -34,6 +34,7 @@ matchers = [
     MethodMatcher,
     URLMatcher,
     HeadersMatcher,
+    HeaderExistsMatcher,
     QueryMatcher,
     PathMatcher,
     BodyMatcher,
@@ -41,6 +42,7 @@ matchers = [
     JSONMatcher,
     JSONSchemaMatcher,
     QueryMatcher,
+    QueryParameterExistsMatcher,
 ]
 
 
diff --git a/src/pook/matchers/base.py b/src/pook/matchers/base.py
index 427304b..e32ad95 100644
--- a/src/pook/matchers/base.py
+++ b/src/pook/matchers/base.py
@@ -4,7 +4,7 @@ from abc import abstractmethod, ABCMeta
 from ..compare import compare
 
 
-class BaseMatcher(object):
+class BaseMatcher:
     """
     BaseMatcher implements the basic HTTP request matching interface.
     """
@@ -81,3 +81,39 @@ class BaseMatcher(object):
             return not result if self.negate else result
 
         return wrapper
+
+
+class ExistsMatcher(BaseMatcher, metaclass=ABCMeta):
+    """
+    Base class for matchers that only check for existence.
+    """
+
+    @property
+    @abstractmethod
+    def request_attr(self):
+        """
+        The attribute from the request in which to check for existence of the expectation.
+        """
+        ...
+
+    def get_request_attribute(self, request):
+        """
+        Retrieve attribute from the request in which existence should be checked.
+        """
+        if self.request_attr is None:
+            raise ValueError("`request_attr` must not be None")
+
+        return getattr(request, self.request_attr)
+
+    @BaseMatcher.matcher
+    def match(self, request):
+        attribute = self.get_request_attribute(request)
+        assert (
+            attribute is not None
+        ), f"Expected request to have {self.request_attr} with {self.expectation}, but no {self.request_attr} found on the request"
+
+        assert (
+            self.expectation in attribute
+        ), f"{self.expectation} not found in request's {self.request_attr}"
+
+        return True
diff --git a/src/pook/matchers/headers.py b/src/pook/matchers/headers.py
index 21ae487..ad03cf9 100644
--- a/src/pook/matchers/headers.py
+++ b/src/pook/matchers/headers.py
@@ -1,4 +1,4 @@
-from .base import BaseMatcher
+from .base import BaseMatcher, ExistsMatcher
 from ..headers import to_string_value
 from ..regex import Pattern
 
@@ -53,3 +53,7 @@ class HeadersMatcher(BaseMatcher):
             return value
 
         return to_string_value(value)
+
+
+class HeaderExistsMatcher(ExistsMatcher):
+    request_attr = "headers"
diff --git a/src/pook/matchers/query.py b/src/pook/matchers/query.py
index c762033..f97f664 100644
--- a/src/pook/matchers/query.py
+++ b/src/pook/matchers/query.py
@@ -1,4 +1,4 @@
-from .base import BaseMatcher
+from .base import BaseMatcher, ExistsMatcher
 
 from urllib.parse import parse_qs
 
@@ -41,3 +41,31 @@ class QueryMatcher(BaseMatcher):
 
         # Match query params
         return self.match_query(query, req_query)
+
+
+class QueryParameterExistsMatcher(ExistsMatcher):
+    request_attr = "query"
+
+    def __init__(self, expectation, allow_empty, negate=False):
+        super().__init__(expectation, negate)
+        self.allow_empty = allow_empty
+
+    def match(self, request):
+        if not super().match(request):
+            return False
+
+        if not self.allow_empty:
+            attribute = self.get_request_attribute(request)
+            assert not self.is_empty(
+                attribute[self.expectation]
+            ), f"The request's {self.expectation} query parameter was unexpectedly empty."
+
+        return True
+
+    def is_empty(self, value):
+        """
+        Check for empty query parameter values.
+
+        `urllib.parse.parse_qs` returns a value of `['']` for parameters that are present but without value.
+        """
+        return not value or value == [""]
diff --git a/src/pook/mock.py b/src/pook/mock.py
index dd6fed6..246aeb3 100644
--- a/src/pook/mock.py
+++ b/src/pook/mock.py
@@ -1,4 +1,3 @@
-import re
 import functools
 from furl import furl
 from inspect import isfunction, ismethod
@@ -276,10 +275,7 @@ class Mock(object):
             (pook.get('server.com/api')
                 .header_present('content-type'))
         """
-        for name in names:
-            headers = {name: re.compile("(.*)")}
-            self.add_matcher(matcher("HeadersMatcher", headers))
-        return self
+        return self.headers_present(names)
 
     def headers_present(self, headers):
         """
@@ -300,8 +296,11 @@ class Mock(object):
             (pook.get('server.com/api')
                 .headers_present(['content-type', 'Authorization']))
         """
-        headers = {name: re.compile("(.*)") for name in headers}
-        self.add_matcher(matcher("HeadersMatcher", headers))
+        if not headers:
+            raise ValueError("`headers` must not be empty")
+
+        for header in headers:
+            self.add_matcher(matcher("HeaderExistsMatcher", header))
         return self
 
     def type(self, value):
@@ -368,17 +367,18 @@ class Mock(object):
         self.params({name: value})
         return self
 
-    def param_exists(self, name):
+    def param_exists(self, name, allow_empty=False):
         """
         Checks if a given URL param name is present in the URL.
 
         Arguments:
             name (str): param name to check existence.
+            allow_empty (bool): whether to allow an empty value of the param
 
         Returns:
             self: current Mock instance.
         """
-        self.params({name: re.compile("(.*)")})
+        self.add_matcher(matcher("QueryParameterExistsMatcher", name, allow_empty))
         return self
 
     def params(self, params):
diff --git a/src/pook/request.py b/src/pook/request.py
index abd8733..e4f7279 100644
--- a/src/pook/request.py
+++ b/src/pook/request.py
@@ -90,10 +90,15 @@ class Request(object):
             if not protoregex.match(url):
                 url = "http://{}".format(url)
             self._url = urlparse(url)
-            self._query = parse_qs(self._url.query) if self._url.query else self._query
+            # keep_blank_values necessary for `param_exists` when a parameter has no value but is present
+            self._query = (
+                parse_qs(self._url.query, keep_blank_values=True)
+                if self._url.query
+                else self._query
+            )
 
     @property
-    def query(self, url):
+    def query(self):
         return self._query
 
     @query.setter


Test patch:
diff --git a/tests/unit/matchers/headers_test.py b/tests/unit/matchers/headers_test.py
index 277391a..e4de845 100644
--- a/tests/unit/matchers/headers_test.py
+++ b/tests/unit/matchers/headers_test.py
@@ -236,6 +236,6 @@ def test_headers_present(required_headers, requested_headers, should_match):
     assert matched == should_match, explanation
 
 
-def test_headers_present_empty_headers():
+def test_headers_present_empty_argument():
     with pytest.raises(ValueError):
         pook.get("https://example.com").headers_present([])
diff --git a/tests/unit/matchers/query_test.py b/tests/unit/matchers/query_test.py
index e69de29..c2856d4 100644
--- a/tests/unit/matchers/query_test.py
+++ b/tests/unit/matchers/query_test.py
@@ -0,0 +1,21 @@
+import pytest
+from urllib.request import urlopen
+
+import pook
+from pook.exceptions import PookNoMatches
+
+
+@pytest.mark.pook(allow_pending_mocks=True)
+def test_param_exists_empty_disallowed():
+    pook.get("https://httpbin.org/404").param_exists("x").reply(200)
+
+    with pytest.raises(PookNoMatches):
+        urlopen("https://httpbin.org/404?x")
+
+
+@pytest.mark.pook
+def test_param_exists_empty_allowed():
+    pook.get("https://httpbin.org/404").param_exists("x", allow_empty=True).reply(200)
+
+    res = urlopen("https://httpbin.org/404?x")
+    assert res.status == 200
diff --git a/tests/unit/mock_engine_test.py b/tests/unit/mock_engine_test.py
index afc62d3..5b83476 100644
--- a/tests/unit/mock_engine_test.py
+++ b/tests/unit/mock_engine_test.py
@@ -1,4 +1,7 @@
 import pytest
+from urllib.request import urlopen, Request
+
+import pook
 from pook import MockEngine, Engine
 from pook.interceptors import BaseInterceptor
 
@@ -49,3 +52,20 @@ def test_mock_engine_status(engine):
 
     engine.disable()
     assert not interceptor.active
+
+
+@pytest.mark.xfail(
+    reason="Pook cannot disambiguate the two mocks. Ideally it would try to find the most specific mock that matches, but that's not possible yet."
+)
+@pytest.mark.pook(allow_pending_mocks=True)
+def test_mock_specificity():
+    pook.get("https://httpbin.org/404").header_present("authorization").reply(201)
+    pook.get("https://httpbin.org/404").headers({"Authorization": "Bearer pook"}).reply(
+        200
+    )
+
+    res_with_headers = urlopen(
+        Request("https://httpbin.org/404", headers={"Authorization": "Bearer pook"})
+    )
+
+    assert res_with_headers.status == 200
diff --git a/tests/unit/mock_test.py b/tests/unit/mock_test.py
index 4be7b79..35d342d 100644
--- a/tests/unit/mock_test.py
+++ b/tests/unit/mock_test.py
@@ -1,12 +1,10 @@
 import pytest
 import json
-import re
 
 import pook
 from pook.mock import Mock
 from pook.request import Request
 from urllib.request import urlopen
-from urllib.parse import urlencode
 
 
 @pytest.fixture
@@ -38,14 +36,21 @@ def test_mock_url(mock):
         ),
         pytest.param(
             {"param_exists": "z"},
-            # This complexity is needed until https://github.com/h2non/pook/issues/110
-            # is resolved
-            f'?{urlencode({"z": re.compile("(.*)")})}',
-            id="param_exists",
+            "?z",
+            marks=pytest.mark.xfail(
+                condition=True,
+                reason="Constructor does not have a method for passing `allow_empty` to `param_exists`",
+            ),
+            id="param_exists_empty_on_request",
+        ),
+        pytest.param(
+            {"param_exists": "z"},
+            "?z=123",
+            id="param_exists_has_value",
         ),
     ),
 )
-def test_constructor(param_kwargs, query_string):
+def test_mock_constructor(param_kwargs, query_string):
     # Should not raise
     mock = Mock(
         url="https://httpbin.org/404",
@@ -54,12 +59,9 @@ def test_constructor(param_kwargs, query_string):
         **param_kwargs,
     )
 
-    expected_url = f"https://httpbin.org/404{query_string}"
-    assert mock._request.rawurl == expected_url
-
     with pook.use():
         pook.engine().add_mock(mock)
-        res = urlopen(expected_url)
+        res = urlopen(f"https://httpbin.org/404{query_string}")
         assert res.status == 200
         assert json.loads(res.read()) == {"hello": "from pook"}