From 31b6b266607c8203e1c4b39f44623fc8e8a5fd97 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Wed, 30 Sep 2026 16:20:51 +0000 Subject: [PATCH] Fix advanced search for spaced external IDs Auto-quote unquoted external_ids.all / preferred values that contain whitespace so ISNI-style IDs match as a single keyword (ror-roadmap#71). When a legacy v1 path like external_ids.FundRef.all is used, include a hint pointing at the v2 fields (ror-roadmap#70). Refs ror-community/ror-roadmap#70, ror-community/ror-roadmap#71 Co-authored-by: Joseph Rhoads --- rorapi/common/queries.py | 60 +++++++++++++++++-- .../tests_integration/tests_search_v2.py | 34 +++++++++++ rorapi/tests/tests_unit/tests_queries_v2.py | 38 +++++++++++- 3 files changed, 127 insertions(+), 5 deletions(-) diff --git a/rorapi/common/queries.py b/rorapi/common/queries.py index 1577c0e..1302b22 100644 --- a/rorapi/common/queries.py +++ b/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"(?Pexternal_ids\.(?:all|preferred)):" + r"(?P(?P\"(?:\\.|[^\"\\])*\")|" + r"(?P.*?))" + 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) diff --git a/rorapi/tests/tests_integration/tests_search_v2.py b/rorapi/tests/tests_integration/tests_search_v2.py index c8279de..6650b8d 100644 --- a/rorapi/tests/tests_integration/tests_search_v2.py +++ b/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'])) diff --git a/rorapi/tests/tests_unit/tests_queries_v2.py b/rorapi/tests/tests_unit/tests_queries_v2.py index 251b542..b8d87ea 100644 --- a/rorapi/tests/tests_unit/tests_queries_v2.py +++ b/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'