Remove dependency on fcntl (improves Windows support)

.github/workflows/lint.yml

@@ -16,7 +16,7 @@ jobs:
       run: pip install black
 
     - name: Isort
-      run: isort --check-only --recursive *.py */
+      run: isort --check-only *.py */
 
     - name: Flake8
       run: flake8

libcove/lib/common.py

@@ -1,13 +1,13 @@
 import collections
 import csv
 import datetime
-import fcntl
 import functools
 import json
 import logging
 import os
 import re
 from collections import OrderedDict
+from tempfile import NamedTemporaryFile
 from urllib.parse import urljoin, urlparse
 
 import jsonref
@@ -1154,51 +1154,45 @@ def get_spreadsheet_meta_data(
     return metatab_json
 
 
-def get_orgids_prefixes(orgids_url=None):
-    """Get org-ids.json file from file system (or fetch upstream if it doesn't exist)
+def org_id_file_fresh(org_id_file_contents, check_date):
+    """Unless the file was downloaded on greater than or equal to 'check_date' it is considered stale."""
+    org_id_file_date_downloaded_date = datetime.datetime.strptime(
+        org_id_file_contents.get("downloaded", "2000-1-1"), "%Y-%m-%d"
+    ).date()
+    return org_id_file_date_downloaded_date >= check_date
 
-    A lock file is needed to avoid different processes trying to access the file
-    trampling each other. If a process has the exclusive lock, a different process
-    will wait until it is released.
-    """
-    local_org_ids_dir = os.path.dirname(os.path.realpath(__file__))
-    local_org_ids_file = os.path.join(local_org_ids_dir, "org-ids.json")
-    lock_file = os.path.join(local_org_ids_dir, "org-ids.json.lock")
-    today = datetime.date.today()
-    get_remote_file = False
-    first_request = False
 
-    if not orgids_url:
+def get_orgids_prefixes(orgids_url=None):
+    """Get org-ids.json file from file system (or fetch remotely if it doesn't exist)"""
+    local_org_ids_file = os.path.join(
+        os.path.dirname(os.path.realpath(__file__)), "org-ids.json"
+    )
+    today = datetime.date.today()
+    if orgids_url is None:
         orgids_url = "http://org-id.guide/download.json"
-
-    if os.path.exists(local_org_ids_file):
-        with open(lock_file, "w") as lock:
-            fcntl.flock(lock, fcntl.LOCK_EX)
-            fp = open(local_org_ids_file)
-            org_ids = json.load(fp)
-            fp.close()
-            fcntl.flock(lock, fcntl.LOCK_UN)
-        date_str = org_ids.get("downloaded", "2000-1-1")
-        date_downloaded = datetime.datetime.strptime(date_str, "%Y-%m-%d").date()
-        if date_downloaded != today:
-            get_remote_file = True
-    else:
-        get_remote_file = True
-        first_request = True
-
-    if get_remote_file:
+    org_id_file_contents = None
+
+    # Try to grab the data from the local filesystem
+    try:
+        with open(local_org_ids_file) as fp:
+            org_id_file_contents = json.load(fp)
+    except FileNotFoundError:
+        pass
+
+    if org_id_file_contents is None or not org_id_file_fresh(
+        org_id_file_contents, today
+    ):
+        # Refresh the file
         try:
-            org_ids = requests.get(orgids_url).json()
-            org_ids["downloaded"] = "%s" % today
-            with open(lock_file, "w") as lock:
-                fcntl.flock(lock, fcntl.LOCK_EX)
-                fp = open(local_org_ids_file, "w")
-                json.dump(org_ids, fp, indent=2)
-                fp.close()
-                fcntl.flock(lock, fcntl.LOCK_UN)
-        except requests.exceptions.RequestException:
-            if first_request:
-                raise  # First time ever request fails
-            pass  # Update fails
-
-    return [org_list["code"] for org_list in org_ids["lists"]]
+            org_id_file_contents = requests.get(orgids_url).json()
+        except requests.exceptions.RequestException as e:
+            # We have tried locally and remotely with no luck. We have to raise.
+            raise e
+
+        org_id_file_contents["downloaded"] = "%s" % today
+        # Use a tempfile and move to create new file here for atomicity
+        with NamedTemporaryFile(mode="w", delete=False) as tmp:
+            json.dump(org_id_file_contents, tmp, indent=2)
+        os.rename(tmp.name, local_org_ids_file)
+    # Return either the original file data, if it was found to be fresh, or the new data, if we were able to retrieve it.
+    return [org_list["code"] for org_list in org_id_file_contents["lists"]]

requirements_dev.txt

@@ -1,4 +1,6 @@
 -e .
 pytest
 flake8
+freezegun
 isort
+requests-mock

tests/lib/test_common.py

@@ -1,8 +1,11 @@
 import json
 import os
 from collections import OrderedDict
+from datetime import datetime
+from unittest import mock
 
 import pytest
+from freezegun import freeze_time
 
 from libcove.lib.common import (
     SchemaJsonMixin,
@@ -14,6 +17,7 @@
     get_json_data_generic_paths,
     get_orgids_prefixes,
     get_schema_validation_errors,
+    org_id_file_fresh,
     schema_dict_fields_generator,
 )
 
@@ -386,11 +390,17 @@ def test_get_additional_fields_info():
     )
 
 
-def test_get_orgids_prefixes_live():
-    data = get_orgids_prefixes()
+@freeze_time("2020-01-02")
+def test_get_orgids_prefixes_live(requests_mock):
+    file_contents = mock.mock_open(
+        read_data='{"downloaded": "2020-01-01", "lists": [{"code": "001"}, {"code": "002"}]}'
+    )
+    text = {"lists": [{"code": str(i)} for i in range(150)]}
+    requests_mock.get("http://org-id.guide/download.json", text=json.dumps(text))
 
-    # There is not much we can really test here, as the results will depend on the live data!
-    assert len(data) > 150
+    with mock.patch("builtins.open", file_contents):
+        data = get_orgids_prefixes()
+        assert len(data) == 150
 
 
 class DummyReleaseSchemaObj:
@@ -581,3 +591,131 @@ def test_validation_release_or_record_package(
         validation_error_jsons.append(validation_error_json)
 
     assert validation_error_jsons == validation_error_jsons_expected
+
+
+@pytest.mark.parametrize(
+    ("filedate", "checkdate", "result"),
+    (
+        ("2020-01-15", "2020-01-14", True),
+        ("2020-01-15", "2020-01-16", False),
+        ("2020-01-15", "2020-01-15", True),
+        ("1998-01-01", "1999-01-01", False),
+        ("1999-01-01", "1998-01-01", True),
+        ("2000-01-01", "2020-01-01", False),
+    ),
+)
+def test_org_id_file_fresh_dates(filedate, checkdate, result):
+    """Check that the date in the file data is greater than or equal to check date."""
+    assert (
+        org_id_file_fresh(
+            {"downloaded": filedate}, datetime.strptime(checkdate, "%Y-%m-%d").date()
+        )
+        is result
+    )
+
+
+@freeze_time("1955-11-12")
+def test_get_orgids_prefixes_does_not_make_request_when_in_date_file_found(
+    requests_mock,
+):
+    file_data = {
+        "downloaded": "2020-01-01",
+        "lists": [{"code": "001"}, {"code": "002"}],
+    }
+
+    with mock.patch("builtins.open", mock.mock_open(read_data=json.dumps(file_data))):
+        get_orgids_prefixes()
+        assert not requests_mock.called
+
+
+@freeze_time("2020-01-02")
+def test_get_orgids_prefixes_makes_request_when_file_out_of_date(requests_mock):
+    file_data = {
+        "downloaded": "2020-01-01",
+        "lists": [{"code": "001"}, {"code": "002"}],
+    }
+    request_data = {"lists": [{"code": "001"}, {"code": "002"}]}
+
+    requests_mock.get(
+        "http://org-id.guide/download.json", text=json.dumps(request_data)
+    )
+
+    with mock.patch("builtins.open", mock.mock_open(read_data=json.dumps(file_data))):
+        get_orgids_prefixes()
+        assert requests_mock.called
+
+
+@freeze_time("2020-01-02")
+def test_get_orgids_prefixes_returns_file_ids_when_file_in_date(requests_mock):
+    file_data = {
+        "downloaded": "2020-01-02",
+        "lists": [{"code": "file-id-1"}, {"code": "file-id-2"}],
+    }
+
+    requests_mock.get("http://org-id.guide/download.json")
+
+    with mock.patch("builtins.open", mock.mock_open(read_data=json.dumps(file_data))):
+        assert sorted(get_orgids_prefixes()) == ["file-id-1", "file-id-2"]
+
+
+@freeze_time("2020-01-02")
+def test_get_orgids_prefixes_returns_downloaded_ids_when_file_out_of_date(
+    requests_mock,
+):
+    file_data = {
+        "downloaded": "2020-01-01",
+        "lists": [{"code": "file-id-1"}, {"code": "file-id-2"}],
+    }
+    request_data = {
+        "lists": [{"code": "dl-id-001"}, {"code": "dl-id-002"}, {"code": "dl-id-003"}]
+    }
+
+    requests_mock.get(
+        "http://org-id.guide/download.json", text=json.dumps(request_data)
+    )
+
+    with mock.patch("builtins.open", mock.mock_open(read_data=json.dumps(file_data))):
+        assert sorted(get_orgids_prefixes()) == ["dl-id-001", "dl-id-002", "dl-id-003"]
+
+
+@freeze_time("2020-01-02")
+def test_get_orgids_prefixes_opens_file_once_when_in_date(requests_mock):
+    file_data = {
+        "downloaded": "2020-01-02",
+        "lists": [{"code": "file-id-1"}, {"code": "file-id-2"}],
+    }
+
+    with mock.patch(
+        "builtins.open", mock.mock_open(read_data=json.dumps(file_data))
+    ) as file_mock:
+        get_orgids_prefixes()
+        assert file_mock.call_count == 1
+
+
+@freeze_time("2020-01-02")
+@mock.patch("libcove.lib.common.NamedTemporaryFile")
+@mock.patch("libcove.lib.common.os.rename")
+def test_get_orgids_prefixes_opens_and_moves_file_when_updating(
+    rename_mock, tmp_mock, requests_mock
+):
+    tmp_mock.return_value.__enter__.return_value.name = "/path/to/tmp"
+    file_data = {
+        "downloaded": "2020-01-01",
+        "lists": [{"code": "file-id-1"}, {"code": "file-id-2"}],
+    }
+    request_data = {
+        "lists": [{"code": "dl-id-001"}, {"code": "dl-id-002"}, {"code": "dl-id-003"}]
+    }
+
+    requests_mock.get(
+        "http://org-id.guide/download.json", text=json.dumps(request_data)
+    )
+
+    with mock.patch(
+        "builtins.open", mock.mock_open(read_data=json.dumps(file_data))
+    ) as file_mock:
+        get_orgids_prefixes()
+        assert file_mock.call_count == 1
+        assert rename_mock.call_count == 1
+        assert rename_mock.call_args_list[0][0][0] == "/path/to/tmp"
+        assert rename_mock.call_args_list[0][0][1].endswith("org-ids.json")