Send requested properties beside a resource key, with or without a licence key

pipeline/fiftyone_pipeline_cloudrequestengine/readme.md

@@ -29,10 +29,11 @@ property names to ask for, is also needed:
 | `resource_key` | `license_key` | `requested_properties` | Accepted |
 | --- | --- | --- | --- |
 | Yes | No | No | Yes. The resource key states which properties it carries. |
-| Yes | Yes | No | Yes. The licence key adds the products it grants. |
+| Yes | Yes | No | Yes. The answer holds what the resource key carries. |
+| Yes | Yes | Yes | Yes. The service adds the listed properties the licence key grants. |
+| Yes | No | Yes | Yes. The service adds the listed properties, as null with a reason where no licence grants one. |
 | No | Yes | Yes | Yes. A licence key names no properties, so the list says which are wanted. |
 | No | Yes | No | No. The service answers 400 to every such request. |
-| Yes | Either | Yes | No. The service ignores the list while a resource key is present. |
 | No | No | Either | No. There is nothing to authenticate with. |
 
 A combination that is not accepted raises `ValueError` when the engine is
@@ -47,8 +48,11 @@ key and name the properties it wants, so the credential stays on the
 server and each answer carries only what that caller needs. A licence key
 is sent in the body of every request, never in a URL. Where a request
 named properties and the answer left some out, the engine warns once that
-they are not covered by the licence key; the properties that came back
-are usable as normal.
+they are not covered by the keys in use, and the properties that came back
+are usable as normal. With a resource key and no licence key, a property no
+licence grants comes back as null with a reason, so the warning says instead
+that every property is answered and these names are not properties, and
+asks the caller to check the spelling of each one.
 
 See the [Credentials section of the cloud request engine specification](https://github.com/51Degrees/specifications/blob/main/pipeline-specification/pipeline-elements/cloud-request-engine.md#credentials)
 and the rules in

pipeline/fiftyone_pipeline_cloudrequestengine/src/fiftyone_pipeline_cloudrequestengine/cloudrequestengine.py

@@ -60,9 +60,8 @@ def __init__(self, settings = {}):
         license_key, and optionally
         1) requested_properties, the fully qualified names of the
         properties to ask for, as a list or a comma separated string.
-        Required with a license_key alone and refused with a resource_key,
-        because the cloud service ignores the list whenever a resource key
-        is present
+        Required with a license_key alone. Beside a resource_key the cloud
+        service adds them to the properties the resource key carries
         2) a cloud_endpoint to overwrite the default baseurl
         3) an cloud_request_origin to use when making requests
 
@@ -181,9 +180,7 @@ def add_requested_properties(self, names):
         Add to the properties each request asks for, after the engine is
         built. The names are trimmed and lower-cased as the constructor
         does, so a name given in the case the documentation uses still
-        matches the answer, and the credential rules are checked again,
-        because a list only has meaning on a licence key alone and the
-        constructor's check cannot see a list added later.
+        matches the answer.
 
         @type names: list or string
         @param names: the names to add, as a list or one comma separated
@@ -192,7 +189,6 @@ def add_requested_properties(self, names):
 
         self.requested_properties.extend(
             self.setting_names({"names": names}, "names"))
-        self.check_credentials()
 
     def check_credentials(self):
 
@@ -213,10 +209,6 @@ def check_credentials(self):
         if has_resource_key == False and has_requested_properties == False:
             raise ValueError(Constants.MESSAGE_LICENSE_KEY_NEEDS_PROPERTIES)
 
-        if has_resource_key == True and has_requested_properties == True:
-            raise ValueError(
-                Constants.MESSAGE_PROPERTIES_IGNORED_WITH_RESOURCE_KEY)
-
     def get_credentials(self):
 
         """!
@@ -277,12 +269,10 @@ def get_engine_properties(self):
         """
 
         # Get properties for all engines. A resource key alone goes in the
-        # query string as it always has. A licence key identifies an
-        # account, so it is posted in the body instead, where it stays out
-        # of request logs. The cloud service reads both keys from either
-        # place, answers a licence key alone with everything the licence
-        # entitles, and widens a resource key's answer by the products a
-        # licence key sent beside it grants.
+        # query string. A licence key identifies an account, so it is
+        # posted in the body instead, where it stays out of request logs.
+        # The cloud service reads both keys from either place, and answers
+        # a licence key alone with every property the licence entitles.
 
         propertiesURL = self.baseURL + "accessibleProperties"
 
@@ -430,7 +420,7 @@ def process_internal(self, flowdata):
 
         """
 
-        # A resource key names the route, as it always has. A licence key
+        # A resource key names the route. A licence key
         # alone has no resource key to put there, so it uses the data
         # endpoint that takes none and travels in the body with the list
         # of properties wanted.
@@ -488,13 +478,16 @@ def report_properties_not_covered(self, result):
         Compare the properties a request asked for with the ones its answer
         carries, and warn once about any it left out.
 
-        The cloud service drops a property the licence key does not cover
-        without naming it, whenever the request also asked for a property
-        the key does cover, so the engine has to notice the difference
-        itself. A property that is present but null counts as answered,
+        With a licence key, the cloud service drops a property the licence
+        key does not cover without naming it, whenever the request also
+        asked for a property the key does cover, so the engine has to
+        notice the difference itself. With a resource key and no licence
+        key, a property no licence grants comes back as null with a reason,
+        so a name left out is not a property, and the warning says that
+        instead. A property that is present but null counts as answered,
         because it carries a nullreason beside it saying why it has no
         value on this request, which is a different matter from
-        entitlement. The answer cannot change while the key and the list
+        entitlement. The answer cannot change while the keys and the list
         stay the same, so each distinct list is reported once.
 
         @type result: string
@@ -507,11 +500,17 @@ def report_properties_not_covered(self, result):
 
         try:
             answer = json.loads(result)
-        except JSONDecodeError:
+        except (AttributeError, JSONDecodeError, TypeError, ValueError):
             # validate_response has already dealt with an answer that is
             # not JSON, so there is nothing to compare here.
             return
 
+        # An answer that parses but is not an object, such as an array,
+        # has no sections to look in. Reporting must not fail the request,
+        # which succeeded, so there is nothing to say.
+        if isinstance(answer, dict) == False:
+            return
+
         not_covered = []
         for name in sorted(requested):
             section, _, property_name = name.partition(
@@ -524,9 +523,10 @@ def report_properties_not_covered(self, result):
         self.reported_not_covered.add(requested)
 
         if len(not_covered) > 0:
-            warnings.warn(
-                Constants.MESSAGE_PROPERTIES_NOT_COVERED.format(
-                    ", ".join(not_covered)))
+            message = (Constants.MESSAGE_PROPERTIES_NOT_COVERED
+                       if self.license_key != ""
+                       else Constants.MESSAGE_PROPERTIES_NOT_KNOWN)
+            warnings.warn(message.format(", ".join(not_covered)))
 
     def get_content(self, flowData):
 

pipeline/fiftyone_pipeline_cloudrequestengine/src/fiftyone_pipeline_cloudrequestengine/constants.py

@@ -115,18 +115,23 @@ class Constants:
         "properties it wants. Set requested_properties to the fully "
         "qualified names wanted, for example "
         "['device.ismobile', 'device.iscrawler'].")
-    MESSAGE_PROPERTIES_IGNORED_WITH_RESOURCE_KEY = (
-        "CloudRequestEngine has requested_properties and a resource key. "
-        "The cloud service ignores the list whenever a resource key is "
-        "present and answers with everything the resource key carries, so "
-        "the caller would believe the response had narrowed when it had "
-        "not. Either leave out requested_properties, or authenticate with "
-        "a licence key alone.")
 
-    # Logged once when a request named properties and the response left
-    # some of them out without saying so.
+    # Logged once when a request with a licence key named properties and
+    # the response left some of them out without saying so. Both the
+    # licence behind a resource key and a licence key beside it decide
+    # what comes back, so the message names the keys rather than one key.
     MESSAGE_PROPERTIES_NOT_COVERED = (
         "The cloud service did not return the requested properties {} in "
-        "its answer. They are not covered by the licence key in use, so "
-        "they will be missing from every result. The other requested "
+        "its answer. They are not covered by the keys in use, so they "
+        "will be missing from every result. The other requested "
         "properties came back and are usable.")
+    # Logged once when a request with a resource key and no licence key
+    # named properties and the response left some of them out. Without a
+    # licence key a property no licence grants comes back as null with a
+    # reason, so a name left out is not a property.
+    MESSAGE_PROPERTIES_NOT_KNOWN = (
+        "The cloud service did not return the requested properties {} in "
+        "its answer. Every property is answered, with a reason when it has "
+        "no value, so these names are not properties. Check the spelling "
+        "of each one. The other requested properties came back and are "
+        "usable.")

pipeline/fiftyone_pipeline_cloudrequestengine/tests/test_credentials.py

@@ -90,9 +90,10 @@ def test_resource_key_alone_is_accepted(self):
 
     def test_resource_key_with_licence_key_is_accepted(self):
         """
-        Row two of the table. The licence key adds the products it grants,
-        so it travels in the body of the data request and of the accessible
-        properties request, and no list of properties is sent.
+        Row two of the table. With no list the answer holds what the
+        resource key carries and the licence key adds no property, so the
+        licence key travels in the body of the data request and of the
+        accessible properties request, and no list of properties is sent.
         """
 
         engine = CloudRequestEngine({
@@ -182,23 +183,39 @@ def test_licence_key_without_properties_is_refused(self):
         self.assertIn("400", str(context.exception))
         self.assertIn("requested_properties", str(context.exception))
 
-    def test_properties_with_resource_key_are_refused(self):
+    def test_properties_with_resource_key_are_sent(self):
         """
-        Row five of the table, with and without a licence key beside the
-        resource key. The service ignores the list, so the engine refuses
-        to build and says the response would not narrow.
+        A list beside a resource key, with and without a licence key. The
+        service adds the listed properties to those the resource key
+        carries, so the engine builds and sends the list as values, with
+        the licence key in the body where there is one.
         """
 
         for license_key in ("", LICENSE_KEY):
-            with self.assertRaises(ValueError) as context:
-                CloudRequestEngine({
-                    "resource_key": RESOURCE_KEY,
-                    "license_key": license_key,
-                    "requested_properties": REQUESTED,
-                    "http_client": self.mock_http(),
-                })
+            engine = CloudRequestEngine({
+                "resource_key": RESOURCE_KEY,
+                "license_key": license_key,
+                "requested_properties": REQUESTED,
+                "http_client": self.mock_http(),
+            })
 
-            self.assertIn("ignores the list", str(context.exception))
+            requests = self.process(engine)
+
+            for request_type, url, content in requests:
+                self.assertNotIn(LICENSE_KEY, url)
+
+            request_type, url, content = self.data_request(requests)
+            self.assertTrue(url.endswith(RESOURCE_KEY + ".json?"), url)
+            self.assertEqual(
+                RESOURCE_KEY, content[Constants.RESOURCE_PARAMETER])
+            self.assertEqual(
+                "device.ismobile,device.istablet",
+                content[Constants.VALUES_PARAMETER])
+            if license_key == "":
+                self.assertNotIn(Constants.LICENSE_PARAMETER, content)
+            else:
+                self.assertEqual(
+                    LICENSE_KEY, content[Constants.LICENSE_PARAMETER])
 
     def test_no_credential_is_refused(self):
         """
@@ -413,14 +430,13 @@ def test_evidence_no_element_accepts_is_sent(self):
         self.assertEqual("a cookie", content["other"])
         self.assertEqual("iPhone", content["User-Agent"])
 
-    def test_properties_added_later_are_normalised_and_checked(self):
+    def test_properties_added_later_are_normalised_and_sent(self):
         """
         A caller may add to the list after the engine is built, once the
         accessible properties say which client-side properties the key
         carries. The names added are trimmed and lower-cased as the
-        constructor's are, so they match the answer, and the rule that a
-        list only goes with a licence key alone is checked again, because
-        the constructor cannot see a list added after it ran.
+        constructor's are, so they match the answer, and they are sent
+        beside a resource key as well as beside a licence key alone.
         """
 
         engine = CloudRequestEngine({
@@ -447,10 +463,13 @@ def test_properties_added_later_are_normalised_and_checked(self):
             "http_client": self.mock_http(),
         })
 
-        with self.assertRaises(ValueError) as context:
-            engine.add_requested_properties(REQUESTED)
+        engine.add_requested_properties(REQUESTED)
 
-        self.assertIn("ignores the list", str(context.exception))
+        request_type, url, content = self.data_request(
+            self.process(engine))
+        self.assertEqual(
+            "device.ismobile,device.istablet",
+            content[Constants.VALUES_PARAMETER])
 
     def test_properties_the_answer_leaves_out_are_reported_once(self):
         """
@@ -483,10 +502,73 @@ def test_properties_the_answer_leaves_out_are_reported_once(self):
             str(warning.message) for warning in caught
             if "not covered" in str(warning.message)]
         self.assertEqual(1, len(messages), messages)
+        self.assertIn("not covered by the keys in use", messages[0])
         self.assertIn("device.iscrawler", messages[0])
         self.assertNotIn("device.istablet", messages[0])
         self.assertNotIn("device.ismobile", messages[0])
 
+    def test_a_name_left_out_beside_a_resource_key_alone_is_not_a_property(
+            self):
+        """
+        With a resource key, a list and no licence key, a property no
+        licence grants comes back as null with its nullreason, so a name the
+        answer leaves out is not a property at all. The engine says so,
+        once, and does not name a licence key that was never given.
+        """
+
+        engine = CloudRequestEngine({
+            "resource_key": RESOURCE_KEY,
+            "requested_properties": ["device.value", "device.notaproperty"],
+            "http_client": self.mock_http(),
+        })
+
+        pipeline = PipelineBuilder().add(engine).build()
+
+        with warnings.catch_warnings(record=True) as caught:
+            warnings.simplefilter("always")
+            for _ in range(2):
+                data = pipeline.create_flowdata()
+                data.evidence.add("query.User-Agent", "iPhone")
+                data.process()
+
+        messages = [
+            str(warning.message) for warning in caught
+            if "device.notaproperty" in str(warning.message)]
+        self.assertEqual(1, len(messages), messages)
+        self.assertIn(
+            "Every property is answered, with a reason when it has no "
+            "value, so these names are not properties", messages[0])
+        self.assertIn("Check the spelling of each one", messages[0])
+        self.assertNotIn("licence key", messages[0])
+        self.assertNotIn("device.value", messages[0])
+
+    def test_an_answer_that_is_not_an_object_does_not_fail_the_request(self):
+        """
+        The report compares the answer with the list, and an answer that
+        parses as JSON but is not an object has nothing to compare. It
+        must not fail the request, because the request itself succeeded,
+        so the engine leaves the answer alone and says nothing.
+        """
+
+        engine = CloudRequestEngine({
+            "license_key": LICENSE_KEY,
+            "requested_properties": REQUESTED,
+            "http_client": self.mock_http(
+                license_json_response='["device", "ismobile"]'),
+        })
+
+        pipeline = PipelineBuilder().add(engine).build()
+
+        with warnings.catch_warnings(record=True) as caught:
+            warnings.simplefilter("always")
+            data = pipeline.create_flowdata()
+            data.evidence.add("query.User-Agent", "iPhone")
+            data.process()
+
+        self.assertEqual(
+            [], [str(warning.message) for warning in caught
+                 if "did not return" in str(warning.message)])
+
     def test_no_report_when_every_property_came_back(self):
         """
         An answer carrying every requested property raises no warning, and