diff --git a/pipeline/fiftyone_pipeline_cloudrequestengine/readme.md b/pipeline/fiftyone_pipeline_cloudrequestengine/readme.md index 022d2c05..de740e3b 100644 --- a/pipeline/fiftyone_pipeline_cloudrequestengine/readme.md +++ b/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 diff --git a/pipeline/fiftyone_pipeline_cloudrequestengine/src/fiftyone_pipeline_cloudrequestengine/cloudrequestengine.py b/pipeline/fiftyone_pipeline_cloudrequestengine/src/fiftyone_pipeline_cloudrequestengine/cloudrequestengine.py index ff598ef1..5dfc6bc0 100644 --- a/pipeline/fiftyone_pipeline_cloudrequestengine/src/fiftyone_pipeline_cloudrequestengine/cloudrequestengine.py +++ b/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): diff --git a/pipeline/fiftyone_pipeline_cloudrequestengine/src/fiftyone_pipeline_cloudrequestengine/constants.py b/pipeline/fiftyone_pipeline_cloudrequestengine/src/fiftyone_pipeline_cloudrequestengine/constants.py index 4ba8535f..60bd4d24 100644 --- a/pipeline/fiftyone_pipeline_cloudrequestengine/src/fiftyone_pipeline_cloudrequestengine/constants.py +++ b/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.") diff --git a/pipeline/fiftyone_pipeline_cloudrequestengine/tests/test_credentials.py b/pipeline/fiftyone_pipeline_cloudrequestengine/tests/test_credentials.py index 75b08646..e29c2e64 100644 --- a/pipeline/fiftyone_pipeline_cloudrequestengine/tests/test_credentials.py +++ b/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