Fix advanced search for spaced external IDs and clarify v1 FundRef paths

rorapi/common/queries.py

@@ -58,6 +58,25 @@
 # _exists_: check if field has non-null value, ex _exists_:wikipedia_url
 ALLOWED_ENDINGS = ("_exists_", "\\", "\\*")
 
+# Keyword fields whose values often contain spaces (e.g. ISNI). Unquoted
+# whitespace is split by Elasticsearch query_string (default_operator AND),
+# so we auto-quote those values before building the query.
+_EXTERNAL_ID_VALUE_PATTERN = re.compile(
+    r"(?P<field>external_ids\.(?:all|preferred)):"
+    r"(?P<value>(?P<quoted>\"(?:\\.|[^\"\\])*\")|"
+    r"(?P<unquoted>.*?))"
+    r"(?=(?:\s+(?:AND|OR|NOT)\b|\s*\)|$))",
+    re.IGNORECASE | re.DOTALL,
+)
+_LEGACY_EXTERNAL_ID_FIELD = re.compile(
+    r"^external_ids\.[^.]+\.(all|preferred)$", re.IGNORECASE
+)
+_V2_EXTERNAL_ID_HINT = (
+    "for schema v2 use external_ids.type, external_ids.all, and/or "
+    "external_ids.preferred "
+    "(e.g. external_ids.type:fundref AND external_ids.all:100000908)"
+)
+
 
 def get_ror_id(string):
     """Extracts ROR id from a string and transforms it into canonical form"""
@@ -81,6 +100,40 @@ def adv_query_string_to_list(query_string):
     return field_list
 
 
+def quote_spaced_external_id_values(query_string):
+    """Quote unquoted external_ids.all / .preferred values that contain spaces.
+
+    Elasticsearch query_string treats unquoted whitespace as term separators.
+    ISNI (and similar) IDs are stored as single keyword tokens with spaces, so
+    ``external_ids.all:0000 0001 2375 2908`` must become
+    ``external_ids.all:"0000 0001 2375 2908"``. Already-quoted values and
+    values without whitespace are left unchanged.
+    """
+    if not isinstance(query_string, str) or not query_string:
+        return query_string
+
+    def repl(match):
+        field = match.group("field")
+        if match.group("quoted") is not None:
+            return "{}:{}".format(field, match.group("quoted"))
+        value = match.group("unquoted") or ""
+        core = value.rstrip()
+        trailing_ws = value[len(core) :]
+        if not core or not any(c.isspace() for c in core):
+            return "{}:{}{}".format(field, core, trailing_ws)
+        return '{}:"{}"{}'.format(field, core, trailing_ws)
+
+    return _EXTERNAL_ID_VALUE_PATTERN.sub(repl, query_string)
+
+
+def illegal_field_error_message(field):
+    """Build the illegal-field error, with a v2 hint for legacy ID paths."""
+    message = "string '{}' contains an illegal field name".format(field)
+    if _LEGACY_EXTERNAL_ID_FIELD.match(field):
+        message = "{}; {}".format(message, _V2_EXTERNAL_ID_HINT)
+    return message
+
+
 def check_status_adv_q(adv_q_string):
     status_in_q = False
     adv_query_fields = adv_query_string_to_list(adv_q_string)
@@ -158,9 +211,7 @@ def validate(params):
             and not f.endswith(tuple(ALLOWED_ENDINGS))
         )
     ]
-    errors.extend(
-        ["string '{}' contains an illegal field name".format(f) for f in illegal_fields]
-    )
+    errors.extend([illegal_field_error_message(f) for f in illegal_fields])
 
     filters = filter_string_to_list(params.get("filter", ""))
     invalid_filters = [f for f in filters if ":" not in f]
@@ -207,7 +258,8 @@ def build_search_query(params):
             del params["all_status"]
 
     if "query.advanced" in params:
-        qb.add_string_query_advanced(nfc(params.get("query.advanced")))
+        advanced = quote_spaced_external_id_values(nfc(params.get("query.advanced")))
+        qb.add_string_query_advanced(advanced)
     elif "query" in params:
         query = nfc(params.get("query"))
         ror_id = get_ror_id(query)

rorapi/tests/tests_integration/tests_search_v2.py

@@ -211,3 +211,37 @@ def test_aggregation_keys_keep_original_casing(self):
         self.assertEqual(countries.get('us'), 'United States')
         continents = {b['id']: b['title'] for b in meta['continents']}
         self.assertEqual(continents.get('na'), 'North America')
+
+
+class ExternalIdAdvancedSearchTestCase(SimpleTestCase):
+    """External ID advanced search (ror-roadmap#70, #71)."""
+
+    def test_spaced_isni_without_quotes(self):
+        # query_string would otherwise split on spaces (roadmap#71)
+        unquoted = requests.get(BASE_URL, {
+            'query.advanced': 'external_ids.all:0000 0001 2375 2908'
+        }).json()
+        quoted = requests.get(BASE_URL, {
+            'query.advanced': 'external_ids.all:"0000 0001 2375 2908"'
+        }).json()
+        self.assertTrue(quoted['number_of_results'] > 0)
+        self.assertEqual(unquoted['number_of_results'],
+                         quoted['number_of_results'])
+        self.assertEqual(unquoted['items'][0]['id'],
+                         'https://ror.org/019496w77')
+
+    def test_fundref_via_v2_fields(self):
+        items = requests.get(BASE_URL, {
+            'query.advanced':
+            'external_ids.type:fundref AND external_ids.all:100000908'
+        }).json()
+        self.assertEqual(items['number_of_results'], 1)
+        self.assertEqual(items['items'][0]['id'], 'https://ror.org/02g8xhs57')
+
+    def test_legacy_fundref_path_error_hint(self):
+        items = requests.get(BASE_URL, {
+            'query.advanced': 'external_ids.FundRef.all:100000908'
+        }).json()
+        self.assertIn('errors', items)
+        self.assertTrue(any('external_ids.all' in e for e in items['errors']))
+        self.assertTrue(any('schema v2' in e for e in items['errors']))

rorapi/tests/tests_unit/tests_queries_v2.py

@@ -4,7 +4,8 @@
 
 from django.test import SimpleTestCase
 from rorapi.common.queries import get_ror_id, validate, build_search_query, \
-    build_retrieve_query, search_organizations, retrieve_organization
+    build_retrieve_query, search_organizations, retrieve_organization, \
+    quote_spaced_external_id_values
 from rorapi.settings import ES_VARS
 from .utils import IterableAttrDict
 
@@ -66,6 +67,16 @@ def test_illegal_field(self):
         self.assertEqual(len(error.errors), 1)
         self.assertTrue(any(['illegal' in e for e in error.errors]))
 
+    def test_legacy_external_id_field_hint(self):
+        error = validate({
+            'query.advanced': 'external_ids.FundRef.all:100000908'
+        })
+        self.assertEqual(len(error.errors), 1)
+        self.assertIn('illegal field name', error.errors[0])
+        self.assertIn('external_ids.type', error.errors[0])
+        self.assertIn('external_ids.all', error.errors[0])
+        self.assertIn('schema v2', error.errors[0])
+
 
     def test_invalid_filter(self):
         error = validate({
@@ -284,6 +295,31 @@ def test_query_advanced(self):
         query = build_search_query({'query.advanced': 'query terms'})
         self.assertEqual(query.to_dict(), expected)
 
+    def test_quote_spaced_external_id_values_helper(self):
+        self.assertEqual(
+            quote_spaced_external_id_values(
+                'external_ids.all:0000 0001 2375 2908'),
+            'external_ids.all:"0000 0001 2375 2908"')
+        self.assertEqual(
+            quote_spaced_external_id_values(
+                'external_ids.all:"0000 0001 2375 2908"'),
+            'external_ids.all:"0000 0001 2375 2908"')
+        self.assertEqual(
+            quote_spaced_external_id_values('external_ids.all:100000908'),
+            'external_ids.all:100000908')
+        self.assertEqual(
+            quote_spaced_external_id_values(
+                'external_ids.all:0000 0001 2375 2908 AND status:active'),
+            'external_ids.all:"0000 0001 2375 2908" AND status:active')
+
+    def test_query_advanced_quotes_spaced_external_ids(self):
+        query = build_search_query({
+            'query.advanced': 'external_ids.all:0000 0001 2375 2908'
+        }).to_dict()
+        self.assertEqual(
+            query['query']['bool']['must'][0]['query_string']['query'],
+            'external_ids.all:"0000 0001 2375 2908"')
+
     def test_query_advanced_nfc_normalization(self):
         nfd = 'locations.geonames_details.name:Hu\u0065\u0302\u0301'
         nfc_form = 'locations.geonames_details.name:Hu\u1ebf'