diff --git a/.github/workflows/dev.yml b/.github/workflows/dev.yml index 7fcc925..e848a26 100644 --- a/.github/workflows/dev.yml +++ b/.github/workflows/dev.yml @@ -19,7 +19,7 @@ jobs: - name: Checkout uses: actions/checkout@v2 - name: Set up Docker Buildx - uses: docker/setup-buildx-action@v1 + uses: docker/setup-buildx-action@v4 - name: Cache Docker layers uses: actions/cache@v4 with: @@ -28,12 +28,12 @@ jobs: restore-keys: | ${{ runner.os }}-buildx- - name: Login to DockerHub - uses: docker/login-action@v1 + uses: docker/login-action@v4 with: username: ${{ secrets.DOCKERHUB_RORAPI_USERNAME }} password: ${{ secrets.DOCKERHUB_RORAPI_TOKEN }} - name: Build and push - uses: docker/build-push-action@v2 + uses: docker/build-push-action@v7 with: context: . file: ./Dockerfile @@ -84,7 +84,7 @@ jobs: git add ror/services/api/environments/dev/_ror-api-dev.auto.tfvars git commit -m "Adding ror-api git variables for commit ${{ steps.extract_variables.outputs.GIT_SHA }}" - name: Push changes - uses: ad-m/github-push-action@v0.6.0 + uses: ad-m/github-push-action@v1.3.0 with: github_token: ${{ secrets.PERSONAL_ACCESS_TOKEN }} repository: "ror-community/new-deployment" diff --git a/.github/workflows/pull-request.yml b/.github/workflows/pull-request.yml index f3bc27c..6f765f9 100644 --- a/.github/workflows/pull-request.yml +++ b/.github/workflows/pull-request.yml @@ -18,7 +18,7 @@ jobs: - name: Checkout ror-api code uses: actions/checkout@v2 - name: Set up Python environment - uses: actions/setup-python@v6 + uses: actions/setup-python@v7 with: python-version: "3.12" - name: Install ruff diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 4dd3cdd..c5781ab 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -13,7 +13,7 @@ jobs: - name: Checkout uses: actions/checkout@v2 - name: Set up Docker Buildx - uses: docker/setup-buildx-action@v1 + uses: docker/setup-buildx-action@v4 - name: Cache Docker layers uses: actions/cache@v4 with: @@ -22,7 +22,7 @@ jobs: restore-keys: | ${{ runner.os }}-buildx- - name: Login to DockerHub - uses: docker/login-action@v1 + uses: docker/login-action@v4 with: username: ${{ secrets.DOCKERHUB_RORAPI_USERNAME }} password: ${{ secrets.DOCKERHUB_RORAPI_TOKEN }} @@ -31,7 +31,7 @@ jobs: echo "GIT_TAG=$(git tag --points-at HEAD)" >> $GITHUB_OUTPUT id: set_git_vars - name: Build and push - uses: docker/build-push-action@v2 + uses: docker/build-push-action@v7 with: context: . file: ./Dockerfile @@ -80,7 +80,7 @@ jobs: git add ror/services/api/environments/prod/_ror-api.auto.tfvars git commit -m "Adding ror-api git variables for commit ${{ steps.extract_variables.outputs.GIT_SHA }}" - name: Push changes - uses: ad-m/github-push-action@v0.6.0 + uses: ad-m/github-push-action@v1.3.0 with: github_token: ${{ secrets.PERSONAL_ACCESS_TOKEN }} repository: 'ror-community/new-deployment' diff --git a/.github/workflows/run_tests.yml b/.github/workflows/run_tests.yml index 142a083..392cabd 100644 --- a/.github/workflows/run_tests.yml +++ b/.github/workflows/run_tests.yml @@ -43,7 +43,7 @@ jobs: with: path: ror-api - name: Set up Python environment - uses: actions/setup-python@v6 + uses: actions/setup-python@v7 with: python-version: "3.12" cache: "pip" diff --git a/.github/workflows/staging.yml b/.github/workflows/staging.yml index e52b0ad..80babe6 100644 --- a/.github/workflows/staging.yml +++ b/.github/workflows/staging.yml @@ -15,7 +15,7 @@ jobs: - name: Checkout uses: actions/checkout@v2 - name: Set up Docker Buildx - uses: docker/setup-buildx-action@v1 + uses: docker/setup-buildx-action@v4 - name: Cache Docker layers uses: actions/cache@v4 with: @@ -24,12 +24,12 @@ jobs: restore-keys: | ${{ runner.os }}-buildx- - name: Login to DockerHub - uses: docker/login-action@v1 + uses: docker/login-action@v4 with: username: ${{ secrets.DOCKERHUB_RORAPI_USERNAME }} password: ${{ secrets.DOCKERHUB_RORAPI_TOKEN }} - name: Build and push - uses: docker/build-push-action@v2 + uses: docker/build-push-action@v7 with: context: . file: ./Dockerfile @@ -80,7 +80,7 @@ jobs: git add ror/services/api/environments/staging/_ror-api-staging.auto.tfvars git commit -m "Adding ror-api git variables for commit ${{ steps.extract_variables.outputs.GIT_SHA }}" - name: Push changes - uses: ad-m/github-push-action@v0.6.0 + uses: ad-m/github-push-action@v1.3.0 with: github_token: ${{ secrets.PERSONAL_ACCESS_TOKEN }} repository: 'ror-community/new-deployment' diff --git a/rorapi/common/create_update.py b/rorapi/common/create_update.py index f965fba..05bc337 100644 --- a/rorapi/common/create_update.py +++ b/rorapi/common/create_update.py @@ -1,3 +1,7 @@ +import logging + +logger = logging.getLogger(__name__) + import copy import functools import json @@ -60,10 +64,10 @@ def update_locations(locations): for location in locations: if 'geonames_id' in location: try: - print(location['geonames_id']) + logger.info(location['geonames_id']) updated_location = ua.new_geonames_v2(str(location['geonames_id'])) updated_locations.append(updated_location['location']) - except: + except Exception: error = "Error retrieving Geonames data for ID {}. Please check that this is a valid Geonames ID".format(location['geonames_id']) return error, updated_locations @@ -90,7 +94,7 @@ def new_record_from_json(json_input, version): new_record['locations'] = updated_locations new_record = add_created_last_mod(new_record) new_ror_id = check_ror_id() - print("new ror id: " + new_ror_id) + logger.info("new ror id: " + new_ror_id) new_record['id'] = new_ror_id error, valid_data = validate_record(sort_list_fields(new_record), get_v2_schema()) return error, valid_data diff --git a/rorapi/common/csv_bulk.py b/rorapi/common/csv_bulk.py index c6dbe8f..d5ee30b 100644 --- a/rorapi/common/csv_bulk.py +++ b/rorapi/common/csv_bulk.py @@ -1,3 +1,7 @@ +import logging + +logger = logging.getLogger(__name__) + import csv import json import io @@ -43,7 +47,7 @@ def save_report_file(report, report_fields, csv_file, dir_name, validate_only): f.write(chunk) def process_csv(csv_file, version, validate_only): - print("Processing CSV") + logger.info("Processing CSV") dir_name = datetime.now().strftime("%Y-%m-%d_%H_%M_%S") + "-ror-records" success_msg = None error = None @@ -53,15 +57,15 @@ def process_csv(csv_file, version, validate_only): updated_count = 0 new_count = 0 read_file = csv_file.read().decode('utf-8') - print(read_file) + logger.info(read_file) reader = csv.DictReader(io.StringIO(read_file)) row_num = 2 for row in reader: html_url = None ror_id = None updated = False - print("Row data") - print(row) + logger.info("Row data") + logger.info(row) if row['html_url']: html_url = row['html_url'] if row['id']: @@ -80,7 +84,7 @@ def process_csv(csv_file, version, validate_only): ror_id = v2_record['id'] serializer = OrganizationSerializerV2(v2_record) json_obj = json.loads(JSONRenderer().render(serializer.data)) - print(json_obj) + logger.info(json_obj) if not validate_only: #create file file = save_record_file(ror_id, updated, json_obj, dir_name) diff --git a/rorapi/common/csv_create.py b/rorapi/common/csv_create.py index 10ff03b..154b90b 100644 --- a/rorapi/common/csv_create.py +++ b/rorapi/common/csv_create.py @@ -1,3 +1,7 @@ +import logging + +logger = logging.getLogger(__name__) + import copy from rorapi.common.record_utils import * from rorapi.common.csv_utils import * @@ -74,8 +78,8 @@ def new_record_from_csv(csv_data, version): "lang": lang_code } temp_names.append(name_obj) - print("temp names 1:") - print(temp_names) + logger.info("temp names 1:") + logger.info(temp_names) name_vals = [n['value'] for n in temp_names] dup_names = [] for n in name_vals: @@ -99,8 +103,8 @@ def new_record_from_csv(csv_data, version): if name_obj not in temp_names: temp_names = [t for t in temp_names if t not in name_lang_dups] temp_names.append(name_obj) - print("temp names 2:") - print(temp_names) + logger.info("temp names 2:") + logger.info(temp_names) v2_data['names'] = temp_names #status diff --git a/rorapi/common/csv_update.py b/rorapi/common/csv_update.py index 2299e2a..16cb78d 100644 --- a/rorapi/common/csv_update.py +++ b/rorapi/common/csv_update.py @@ -1,3 +1,7 @@ +import logging + +logger = logging.getLogger(__name__) + import copy from rorapi.common.record_utils import * from rorapi.v2.record_constants import * @@ -12,29 +16,29 @@ def update_record_from_csv(csv_data, version): errors = [] updated_record = None - print("updating record from csv") + logger.info("updating record from csv") existing_org_errors, existing_org = retrieve_organization(csv_data['id']) - print(existing_org) + logger.info(existing_org) if existing_org is None: errors.append("No existing record found for ROR ID '{}'".format(csv_data['id'])) else: row_validation_errors = validate_csv_row_update_syntax(csv_data) if row_validation_errors: errors.extend(row_validation_errors) - print("row validation errors:") - print(errors) + logger.info("row validation errors:") + logger.info(errors) else: serializer = OrganizationSerializerV2(existing_org) existing_record = serializer.data - print(existing_record) + logger.info(existing_record) update_data = {} #domains if csv_data['domains']: actions_values = get_actions_values(csv_data['domains']) temp_domains = copy.deepcopy(existing_record['domains']) - print("initial temp domains:") - print(temp_domains) + logger.info("initial temp domains:") + logger.info(temp_domains) if UPDATE_ACTIONS['DELETE'] in actions_values: delete_values = actions_values[UPDATE_ACTIONS['DELETE']] if delete_values is None: @@ -45,23 +49,23 @@ def update_record_from_csv(csv_data, version): errors.append("Attempting to delete domain(s) that don't exist: {}".format(d)) temp_domains = [d for d in temp_domains if d not in delete_values] - print("temp domains delete") - print(temp_domains) + logger.info("temp domains delete") + logger.info(temp_domains) if UPDATE_ACTIONS['ADD'] in actions_values: add_values = actions_values[UPDATE_ACTIONS['ADD']] for a in add_values: if a in temp_domains: errors.append("Attempting to add domain(s) that already exist: {}".format(a)) - print(add_values) + logger.info(add_values) temp_domains.extend(add_values) - print("temp domains add") - print(temp_domains) + logger.info("temp domains add") + logger.info(temp_domains) if UPDATE_ACTIONS['REPLACE'] in actions_values: temp_domains = actions_values[UPDATE_ACTIONS['REPLACE']] - print("temp domains replace") - print(temp_domains) - print("final temp domains:") - print(temp_domains) + logger.info("temp domains replace") + logger.info(temp_domains) + logger.info("final temp domains:") + logger.info(temp_domains) update_data['domains'] = temp_domains #established @@ -180,18 +184,18 @@ def update_record_from_csv(csv_data, version): "value": r } temp_links.append(link_obj) - print("final temp links:") - print(temp_links) + logger.info("final temp links:") + logger.info(temp_links) update_data['links'] = temp_links #locations if csv_data['locations.geonames_id']: actions_values = get_actions_values(csv_data['locations.geonames_id']) temp_locations = copy.deepcopy(existing_record['locations']) - print("initial temp locations:") - print(temp_locations) + logger.info("initial temp locations:") + logger.info(temp_locations) existing_geonames_ids = [tl['geonames_id'] for tl in temp_locations] - print(existing_geonames_ids) + logger.info(existing_geonames_ids) if UPDATE_ACTIONS['DELETE'] in actions_values: delete_values = [int(d) for d in actions_values[UPDATE_ACTIONS['DELETE']]] for d in delete_values: @@ -219,8 +223,8 @@ def update_record_from_csv(csv_data, version): "geonames_details": {} } temp_locations.append(location_obj) - print("final temp locations:") - print(temp_locations) + logger.info("final temp locations:") + logger.info(temp_locations) update_data['locations'] = temp_locations #names @@ -228,20 +232,20 @@ def update_record_from_csv(csv_data, version): for k,v in V2_NAME_TYPES.items(): if csv_data['names.types.' + v]: updated_name_types.append(v) - print("updated name types") - print(updated_name_types) + logger.info("updated name types") + logger.info(updated_name_types) if updated_name_types: temp_names = copy.deepcopy(existing_record['names']) for t in updated_name_types: - print("updating name type " + t) + logger.info("updating name type " + t) if csv_data['names.types.' + t]: actions_values = get_actions_values(csv_data['names.types.' + t]) for k, v in actions_values.items(): if v: vals_obj_list = [] for val in v: - print("val is") - print(val) + logger.info("val is") + logger.info(val) vals_obj = { "value": None, "lang": None @@ -263,12 +267,12 @@ def update_record_from_csv(csv_data, version): if vals_obj["value"]: vals_obj_list.append(vals_obj) actions_values[k] = vals_obj_list - print("updated actions values") - print(actions_values) + logger.info("updated actions values") + logger.info(actions_values) if UPDATE_ACTIONS['DELETE'] in actions_values: - print("delete in actions") + logger.info("delete in actions") delete_values = actions_values[UPDATE_ACTIONS['DELETE']] - print(delete_values) + logger.info(delete_values) if delete_values is None: temp_names = [tn for tn in temp_names if t not in tn['types']] else: @@ -299,8 +303,8 @@ def update_record_from_csv(csv_data, version): else: name_vals_match = [tn for tn in temp_names if (tn['value'] == a['value'] and tn['lang'] == a['lang'])] if name_vals_match: - print("name vals match") - print(name_vals_match) + logger.info("name vals match") + logger.info(name_vals_match) for nvm in name_vals_match: # if value and lang exist but not type, add type only if len(nvm['types']) > 0: @@ -346,8 +350,8 @@ def update_record_from_csv(csv_data, version): } temp_names.append(name_obj) - print("final temp names:") - print(temp_names) + logger.info("final temp names:") + logger.info(temp_names) update_data['names'] = temp_names #status @@ -362,8 +366,8 @@ def update_record_from_csv(csv_data, version): if csv_data['types']: actions_values = get_actions_values(csv_data['types']) temp_types = copy.deepcopy(existing_record['types']) - print("initial temp types:") - print(temp_types) + logger.info("initial temp types:") + logger.info(temp_types) if UPDATE_ACTIONS['DELETE'] in actions_values: delete_values = [av.lower() for av in actions_values[UPDATE_ACTIONS['DELETE']]] for d in delete_values: @@ -380,8 +384,8 @@ def update_record_from_csv(csv_data, version): temp_types.extend(add_values) if UPDATE_ACTIONS['REPLACE'] in actions_values: temp_types = [av.lower() for av in actions_values[UPDATE_ACTIONS['REPLACE']]] - print("final temp types:") - print(temp_types) + logger.info("final temp types:") + logger.info(temp_types) update_data['types'] = temp_types if not errors: diff --git a/rorapi/common/csv_utils.py b/rorapi/common/csv_utils.py index 8c4d706..d0e8e1f 100644 --- a/rorapi/common/csv_utils.py +++ b/rorapi/common/csv_utils.py @@ -1,3 +1,7 @@ +import logging + +logger = logging.getLogger(__name__) + import csv import io import re @@ -43,27 +47,27 @@ def get_actions_values(csv_field): - print("getting actions values:") + logger.info("getting actions values:") actions_values = {} if csv_field.lower() == UPDATE_ACTIONS["DELETE"]: actions_values[UPDATE_ACTIONS["DELETE"]] = None elif UPDATE_DELIMITER in csv_field: for ua in list(UPDATE_ACTIONS.values()): - print(ua) + logger.info(ua) if ua + UPDATE_DELIMITER in csv_field: - print("doing regex:") + logger.info("doing regex:") regex = r"(" + re.escape( ua + UPDATE_DELIMITER) + r")(.*?)(?=$|(add|delete|replace)==)" result = re.search(regex, csv_field) - print(result[0]) + logger.info(result[0]) temp_val = result[0].replace(ua + UPDATE_DELIMITER, '') - print("temp val:") - print(temp_val) + logger.info("temp val:") + logger.info(temp_val) actions_values[ua] = [v.strip() for v in temp_val.split(';') if v] else: actions_values[UPDATE_ACTIONS["REPLACE"]] = [v.strip() for v in csv_field.split(';') if v] - print(actions_values) + logger.info(actions_values) return actions_values def validate_csv(csv_file): @@ -80,28 +84,28 @@ def validate_csv(csv_file): for field in CSV_REQUIRED_FIELDS_ACTIONS.keys(): if field not in csv_fields: missing_fields.append(field) - print(missing_fields) + logger.info(missing_fields) if missing_fields: errors.append(f'CSV file is missing columns: {", ".join(missing_fields)}') else: errors.append("CSV file contains no data rows") except IOError as e: errors.append(f"Error parsing CSV file: {e}") - print(errors) + logger.info(errors) return errors def validate_csv_row_update_syntax(csv_data): - print("validating row") + logger.info("validating row") errors = [] for k, v in csv_data.items(): if UPDATE_DELIMITER in v: - print("field:") - print(k) - print("value:") - print(v) + logger.info("field:") + logger.info(k) + logger.info("value:") + logger.info(v) actions_values = get_actions_values(v) - print("actions values:") - print(actions_values) + logger.info("actions values:") + logger.info(actions_values) update_actions = list(actions_values.keys()) if not update_actions: errors.append("Update delimiter '{}' found in '{}' field but no valid update action found in value {}".format(UPDATE_DELIMITER, k, v)) @@ -111,10 +115,10 @@ def validate_csv_row_update_syntax(csv_data): if not (UPDATE_ACTIONS['ADD'] and UPDATE_ACTIONS['DELETE']) in update_actions: errors.append("Invalid combination of update actions '{}' found in '{}' field.".format(", ".join(update_actions), k)) disallowed_actions = [ua for ua in update_actions if ua not in CSV_REQUIRED_FIELDS_ACTIONS[k]] - print("allowed actions:") - print(CSV_REQUIRED_FIELDS_ACTIONS[k]) - print("disallowed actions:") - print(disallowed_actions) + logger.info("allowed actions:") + logger.info(CSV_REQUIRED_FIELDS_ACTIONS[k]) + logger.info("disallowed actions:") + logger.info(disallowed_actions) if disallowed_actions: errors.append("Invalid update action(s) '{}' found in {} field. Allowed actions for this field are '{}'".format(", ".join(disallowed_actions), k, ", ".join(CSV_REQUIRED_FIELDS_ACTIONS[k]))) if v.strip() == UPDATE_ACTIONS['DELETE'].lower() and k in NO_DELETE_FIELDS: 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/common/record_utils.py b/rorapi/common/record_utils.py index 2332785..a8e2fee 100644 --- a/rorapi/common/record_utils.py +++ b/rorapi/common/record_utils.py @@ -1,3 +1,7 @@ +import logging + +logger = logging.getLogger(__name__) + import jsonschema import requests from iso639 import Lang @@ -24,8 +28,8 @@ def get_file_from_url(url): def validate_record(data, schema): try: - print("validating data:") - print(data) + logger.info("validating data:") + logger.info(data) jsonschema.validate(data, schema) except jsonschema.ValidationError as e: return "Validation error: " + e.message, None diff --git a/rorapi/common/views.py b/rorapi/common/views.py index 68aa99d..0bbc8c9 100644 --- a/rorapi/common/views.py +++ b/rorapi/common/views.py @@ -1,3 +1,7 @@ +import logging + +logger = logging.getLogger(__name__) + import csv from rest_framework import viewsets, routers, status from rest_framework.response import Response @@ -160,7 +164,7 @@ class OrganizationViewSet(viewsets.ViewSet): def list(self, request, version=REST_FRAMEWORK["DEFAULT_VERSION"]): params = request.GET.dict() if "query.name" in params or "query.names" in params: - print("redirecting") + logger.info("redirecting") param_name = "query.name" if "query.name" in params else "query.names" params["query"] = params[param_name] del params[param_name] @@ -276,7 +280,7 @@ class GenerateId(APIView): def get(self, request, version=REST_FRAMEWORK["DEFAULT_VERSION"]): id = check_ror_id() - print("Generated ID: {}".format(id)) + logger.info("Generated ID: {}".format(id)) return Response({"id": id}) class IndexData(APIView): @@ -319,7 +323,7 @@ def post(self, request, version=REST_FRAMEWORK["DEFAULT_VERSION"]): errors = Errors(["File upload required. 'file' field is missing."]) else: mime_type = magic.from_buffer(file_object.read(2048)) - print(mime_type) + logger.info(mime_type) if "ASCII text" in mime_type or "UTF-8 text" in mime_type or "UTF-8 Unicode text" in mime_type or "CSV text" in mime_type: file_object.seek(0) csv_validation_errors = validate_csv(file_object) @@ -338,7 +342,7 @@ def post(self, request, version=REST_FRAMEWORK["DEFAULT_VERSION"]): else: errors = Errors(["Could not process request. No data included in request."]) if errors is not None: - print(errors.__dict__) + logger.info(errors.__dict__) return Response( ErrorsSerializer(errors).data, status=status.HTTP_400_BAD_REQUEST ) diff --git a/rorapi/management/commands/getrordump.py b/rorapi/management/commands/getrordump.py index cb75d74..2e2d570 100644 --- a/rorapi/management/commands/getrordump.py +++ b/rorapi/management/commands/getrordump.py @@ -27,7 +27,7 @@ def get_ror_dump_sha(filename, use_test_data, github_headers): if filename in file['name']: sha = file['sha'] return sha - except: + except Exception: return None def get_ror_dump_zip(self, filename, use_test_data, github_headers): @@ -52,7 +52,7 @@ def get_ror_dump_zip(self, filename, use_test_data, github_headers): if dir_names: raise SystemExit(f"Dump zip has extra directory and cannot be indexed") return zip_file.name - except: + except Exception: raise SystemExit(f"Something went wrong saving zip file") class Command(BaseCommand): diff --git a/rorapi/management/commands/setup.py b/rorapi/management/commands/setup.py index fc9f8bf..018a933 100644 --- a/rorapi/management/commands/setup.py +++ b/rorapi/management/commands/setup.py @@ -111,9 +111,9 @@ def handle(self, *args, **options): filename = options['filename'] use_test_data = options['testdata'] if use_test_data: - print("Using ror-data-test repo") + logger.info("Using ror-data-test repo") else: - print("Using ror-data repo") + logger.info("Using ror-data repo") try: sha = get_ror_dump_sha(filename, use_test_data) diff --git a/rorapi/settings.py b/rorapi/settings.py index dea38ed..8931bbb 100644 --- a/rorapi/settings.py +++ b/rorapi/settings.py @@ -10,6 +10,7 @@ https://docs.djangoproject.com/en/5.2/ref/settings/ """ +import logging import os import sys import json @@ -203,7 +204,8 @@ #DATA['CLIENT'] = localboto3.client('s3') #DATA['OBJECT'] = DATA['CLIENT'].list_objects_v2(Bucket = DATA['DATA_STORE']) else: - print("Please set the DATA_STORE environment variable or run this codebase through docker compose") + logging.getLogger(__name__).warning( + "Please set the DATA_STORE environment variable or run this codebase through docker compose") DATA['DIR'] = os.path.join(BASE_DIR, 'rorapi', 'data') ROR_API = {'PAGE_SIZE': 20, 'ID_PREFIX': 'https://ror.org/'} @@ -220,3 +222,19 @@ AWS_ACCESS_KEY_ID = os.environ.get('AWS_ACCESS_KEY_ID') AWS_SECRET_ACCESS_KEY = os.environ.get('AWS_SECRET_ACCESS_KEY') AWS_SES_REGION_NAME = os.environ.get('AWS_REGION', 'eu-west-1') + +LOGGING = { + 'version': 1, + 'disable_existing_loggers': False, + 'handlers': { + 'console': { + 'class': 'logging.StreamHandler', + }, + }, + 'loggers': { + 'rorapi': { + 'handlers': ['console'], + 'level': 'INFO', + }, + }, +} 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'