v1.2.0-stable (#122)

* from_http bug None and non dict data bug fixes (#119) * resolving from_http bugs Signed-off-by: Curtis Mason <cumason@google.com> * resolved from_http bugs Signed-off-by: Curtis Mason <cumason@google.com> * nit fix Signed-off-by: Curtis Mason <cumason@google.com> * Exceptions general class (#120) * More edgecase testing Signed-off-by: Curtis Mason <cumason@google.com> * Tested empty object edge cases Signed-off-by: Curtis Mason <cumason@google.com> * test-coverage Signed-off-by: Curtis Mason <cumason@google.com> * Changelog update (#121) Signed-off-by: Curtis Mason <cumason@google.com>
2020-08-19 17:41:22 -04:00 · 2020-08-19 17:41:22 -04:00 · 390f5944c0
parent 14c76188d1
commit 390f5944c0
10 changed files with 253 additions and 45 deletions
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@ -4,11 +4,18 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

+## [1.2.0]
+### Added
+- Added GenericException, DataMarshallingError and DataUnmarshallingError ([#120])
+
 ## [1.1.0]
 ### Changed
 - Changed from_http to now expect headers argument before data ([#110])
 - Renamed exception names ([#111])

+### Fixed
+- Fixed from_http bugs with data of type None, or not dict-like ([#119])
+
 ### Deprecated
 - Renamed to_binary_http and to_structured_http. ([#108])

@ -105,3 +112,5 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 [#108]: https://github.com/cloudevents/sdk-python/pull/108
 [#110]: https://github.com/cloudevents/sdk-python/pull/110
 [#111]: https://github.com/cloudevents/sdk-python/pull/111
+[#119]: https://github.com/cloudevents/sdk-python/pull/119
+[#120]: https://github.com/cloudevents/sdk-python/pull/120
--- a/cloudevents/init.py
+++ b/cloudevents/init.py
@ -1 +1 @@
-__version__ = "1.1.0"
+__version__ = "1.2.0"
--- a/cloudevents/exceptions.py
+++ b/cloudevents/exceptions.py
@ -11,17 +11,29 @@
 #    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
 #    License for the specific language governing permissions and limitations
 #    under the License.
-class MissingRequiredFields(Exception):
+class GenericException(Exception):
    pass


-class InvalidRequiredFields(Exception):
+class MissingRequiredFields(GenericException):
    pass


-class InvalidStructuredJSON(Exception):
+class InvalidRequiredFields(GenericException):
    pass


-class InvalidHeadersFormat(Exception):
+class InvalidStructuredJSON(GenericException):
+    pass
+
+
+class InvalidHeadersFormat(GenericException):
+    pass
+
+
+class DataMarshallerError(GenericException):
+    pass
+
+
+class DataUnmarshallerError(GenericException):
    pass
--- a/cloudevents/http/event.py
+++ b/cloudevents/http/event.py
@ -59,14 +59,14 @@ class CloudEvent:

        if self._attributes["specversion"] not in _required_by_version:
            raise cloud_exceptions.MissingRequiredFields(
-                f"Invalid specversion: {self._attributes['specversion']}. "
+                f"Invalid specversion: {self._attributes['specversion']}"
            )
        # There is no good way to default 'source' and 'type', so this
        # checks for those (or any new required attributes).
        required_set = _required_by_version[self._attributes["specversion"]]
        if not required_set <= self._attributes.keys():
            raise cloud_exceptions.MissingRequiredFields(
-                f"Missing required keys: {required_set - self._attributes.keys()}. "
+                f"Missing required keys: {required_set - self._attributes.keys()}"
            )

    def __eq__(self, other):
--- a/cloudevents/http/http_methods.py
+++ b/cloudevents/http/http_methods.py
@ -20,19 +20,21 @@ def from_http(
    Unwrap a CloudEvent (binary or structured) from an HTTP request.
    :param headers: the HTTP headers
    :type headers: typing.Dict[str, str]
-    :param data: the HTTP request body
+    :param data: the HTTP request body. If set to None, "" or b'', the returned
+        event's data field will be set to None
    :type data: typing.IO
    :param data_unmarshaller: Callable function to map data to a python object
        e.g. lambda x: x or lambda x: json.loads(x)
    :type data_unmarshaller: types.UnmarshallerType
    """
-    if data is None:
+    if data is None or data == b"":
+        # Empty string will cause data to be marshalled into None
        data = ""

    if not isinstance(data, (str, bytes, bytearray)):
        raise cloud_exceptions.InvalidStructuredJSON(
            "Expected json of type (str, bytes, bytearray), "
-            f"but instead found {type(data)}. "
+            f"but instead found type {type(data)}"
        )

    headers = {key.lower(): value for key, value in headers.items()}
@ -47,22 +49,28 @@ def from_http(
        try:
            raw_ce = json.loads(data)
        except json.decoder.JSONDecodeError:
-            raise cloud_exceptions.InvalidStructuredJSON(
-                "Failed to read fields from structured event. "
-                f"The following can not be parsed as json: {data}. "
+            raise cloud_exceptions.MissingRequiredFields(
+                "Failed to read specversion from both headers and data. "
+                f"The following can not be parsed as json: {data}"
+            )
+        if hasattr(raw_ce, "get"):
+            specversion = raw_ce.get("specversion", None)
+        else:
+            raise cloud_exceptions.MissingRequiredFields(
+                "Failed to read specversion from both headers and data. "
+                f"The following deserialized data has no 'get' method: {raw_ce}"
            )
-        specversion = raw_ce.get("specversion", None)

    if specversion is None:
        raise cloud_exceptions.MissingRequiredFields(
-            "Failed to find specversion in HTTP request. "
+            "Failed to find specversion in HTTP request"
        )

    event_handler = _obj_by_version.get(specversion, None)

    if event_handler is None:
        raise cloud_exceptions.InvalidRequiredFields(
-            f"Found invalid specversion {specversion}. "
+            f"Found invalid specversion {specversion}"
        )

    event = marshall.FromRequest(
@ -73,7 +81,13 @@ def from_http(
    attrs.pop("extensions", None)
    attrs.update(**event.extensions)

-    return CloudEvent(attrs, event.data)
+    if event.data == "" or event.data == b"":
+        # TODO: Check binary unmarshallers to debug why setting data to ""
+        # returns an event with data set to None, but structured will return ""
+        data = None
+    else:
+        data = event.data
+    return CloudEvent(attrs, data)


 def _to_http(
@ -96,7 +110,7 @@ def _to_http(

    if event._attributes["specversion"] not in _obj_by_version:
        raise cloud_exceptions.InvalidRequiredFields(
-            f"Unsupported specversion: {event._attributes['specversion']}. "
+            f"Unsupported specversion: {event._attributes['specversion']}"
        )

    event_handler = _obj_by_version[event._attributes["specversion"]]()
--- a/cloudevents/http/util.py
+++ b/cloudevents/http/util.py
@ -3,7 +3,7 @@ import typing


 def default_marshaller(content: any):
-    if content is None or len(content) == 0:
+    if content is None:
        return None
    try:
        return json.dumps(content)
@ -12,7 +12,7 @@ def default_marshaller(content: any):


 def _json_or_string(content: typing.Union[str, bytes]):
-    if content is None or len(content) == 0:
+    if content is None:
        return None
    try:
        return json.loads(content)
--- a/cloudevents/sdk/event/base.py
+++ b/cloudevents/sdk/event/base.py
@ -201,7 +201,14 @@ class BaseEvent(EventGetterSetter):
            data_marshaller = lambda x: x  # noqa: E731
        props = self.Properties()
        if "data" in props:
-            data = data_marshaller(props.pop("data"))
+            data = props.pop("data")
+            try:
+                data = data_marshaller(data)
+            except Exception as e:
+                raise cloud_exceptions.DataMarshallerError(
+                    "Failed to marshall data with error: "
+                    f"{type(e).__name__}('{e}')"
+                )
            if isinstance(data, (bytes, bytes, memoryview)):
                props["data_base64"] = base64.b64encode(data).decode("ascii")
            else:
@ -225,14 +232,23 @@ class BaseEvent(EventGetterSetter):
            )

        for name, value in raw_ce.items():
+            decoder = lambda x: x
            if name == "data":
                # Use the user-provided serializer, which may have customized
                # JSON decoding
-                value = data_unmarshaller(json.dumps(value))
+                decoder = lambda v: data_unmarshaller(json.dumps(v))
            if name == "data_base64":
-                value = data_unmarshaller(base64.b64decode(value))
+                decoder = lambda v: data_unmarshaller(base64.b64decode(v))
                name = "data"
-            self.Set(name, value)
+
+            try:
+                set_value = decoder(value)
+            except Exception as e:
+                raise cloud_exceptions.DataUnmarshallerError(
+                    "Failed to unmarshall data with error: "
+                    f"{type(e).__name__}('{e}')"
+                )
+            self.Set(name, set_value)

    def UnmarshalBinary(
        self,
@ -256,7 +272,15 @@ class BaseEvent(EventGetterSetter):
                self.SetContentType(value)
            elif header.startswith("ce-"):
                self.Set(header[3:], value)
-        self.Set("data", data_unmarshaller(body))
+
+        try:
+            raw_ce = data_unmarshaller(body)
+        except Exception as e:
+            raise cloud_exceptions.DataUnmarshallerError(
+                "Failed to unmarshall data with error: "
+                f"{type(e).__name__}('{e}')"
+            )
+        self.Set("data", raw_ce)

    def MarshalBinary(
        self, data_marshaller: types.MarshallerType
@ -276,7 +300,13 @@ class BaseEvent(EventGetterSetter):
            headers["ce-{0}".format(key)] = value

        data, _ = self.Get("data")
-        data = data_marshaller(data)
+        try:
+            data = data_marshaller(data)
+        except Exception as e:
+            raise cloud_exceptions.DataMarshallerError(
+                "Failed to marshall data with error: "
+                f"{type(e).__name__}('{e}')"
+            )
        if isinstance(data, str):  # Convenience method for json.dumps
            data = data.encode("utf-8")
        return headers, data
--- a/cloudevents/tests/test_http_cloudevent.py
+++ b/cloudevents/tests/test_http_cloudevent.py
@ -2,6 +2,7 @@ import pytest

 import cloudevents.exceptions as cloud_exceptions
 from cloudevents.http import CloudEvent
+from cloudevents.http.util import _json_or_string


@pytest.mark.parametrize("specversion", ["0.3", "1.0"])
@ -75,19 +76,19 @@ def test_http_cloudevent_mutates_equality(specversion):
 def test_cloudevent_missing_specversion():
    attributes = {"specversion": "0.2", "source": "s", "type": "t"}
    with pytest.raises(cloud_exceptions.MissingRequiredFields) as e:
-        event = CloudEvent(attributes, None)
+        _ = CloudEvent(attributes, None)
    assert "Invalid specversion: 0.2" in str(e.value)


 def test_cloudevent_missing_minimal_required_fields():
    attributes = {"type": "t"}
    with pytest.raises(cloud_exceptions.MissingRequiredFields) as e:
-        event = CloudEvent(attributes, None)
+        _ = CloudEvent(attributes, None)
    assert f"Missing required keys: {set(['source'])}" in str(e.value)

    attributes = {"source": "s"}
    with pytest.raises(cloud_exceptions.MissingRequiredFields) as e:
-        event = CloudEvent(attributes, None)
+        _ = CloudEvent(attributes, None)
    assert f"Missing required keys: {set(['type'])}" in str(e.value)


@ -114,3 +115,7 @@ def test_cloudevent_general_overrides():
        assert attribute in event
        del event[attribute]
    assert len(event) == 0
+
+
+def test_none_json_or_string():
+    assert _json_or_string(None) is None
--- a/cloudevents/tests/test_http_events.py
+++ b/cloudevents/tests/test_http_events.py
@ -90,11 +90,8 @@ async def echo(request):

@pytest.mark.parametrize("body", invalid_cloudevent_request_body)
 def test_missing_required_fields_structured(body):
-    with pytest.raises(cloud_exceptions.MissingRequiredFields):
-        # CloudEvent constructor throws TypeError if missing required field
-        # and NotImplementedError because structured calls aren't
-        # implemented. In this instance one of the required keys should have
-        # prefix e-id instead of ce-id therefore it should throw
+    with pytest.raises(cloud_exceptions.MissingRequiredFields) as e:
+
        _ = from_http(
            {"Content-Type": "application/cloudevents+json"}, json.dumps(body),
        )
@ -103,13 +100,16 @@ def test_missing_required_fields_structured(body):
@pytest.mark.parametrize("headers", invalid_test_headers)
 def test_missing_required_fields_binary(headers):
    with pytest.raises(cloud_exceptions.MissingRequiredFields):
-        # CloudEvent constructor throws TypeError if missing required field
-        # and NotImplementedError because structured calls aren't
-        # implemented. In this instance one of the required keys should have
-        # prefix e-id instead of ce-id therefore it should throw
        _ = from_http(headers, json.dumps(test_data))


+@pytest.mark.parametrize("headers", invalid_test_headers)
+def test_missing_required_fields_empty_data_binary(headers):
+    # Test for issue #115
+    with pytest.raises(cloud_exceptions.MissingRequiredFields):
+        _ = from_http(headers, None)
+
+
@pytest.mark.parametrize("specversion", ["1.0", "0.3"])
 def test_emit_binary_event(specversion):
    headers = {
@ -286,9 +286,17 @@ def test_empty_data_structured_event(specversion):
        "source": "<source-url>",
    }

-    _ = from_http(
+    event = from_http(
        {"content-type": "application/cloudevents+json"}, json.dumps(attributes)
    )
+    assert event.data == None
+
+    attributes["data"] = ""
+    # Data of empty string will be marshalled into None
+    event = from_http(
+        {"content-type": "application/cloudevents+json"}, json.dumps(attributes)
+    )
+    assert event.data == None


@pytest.mark.parametrize("specversion", ["1.0", "0.3"])
@ -302,7 +310,13 @@ def test_empty_data_binary_event(specversion):
        "ce-time": "2018-10-23T12:28:22.4579346Z",
        "ce-source": "<source-url>",
    }
-    _ = from_http(headers, "")
+    event = from_http(headers, None)
+    assert event.data == None
+
+    data = ""
+    # Data of empty string will be marshalled into None
+    event = from_http(headers, data)
+    assert event.data == None


@pytest.mark.parametrize("specversion", ["1.0", "0.3"])
@ -450,11 +464,13 @@ def test_is_structured():
 def test_empty_json_structured():
    headers = {"Content-Type": "application/cloudevents+json"}
    data = ""
-    with pytest.raises(cloud_exceptions.InvalidStructuredJSON) as e:
+    with pytest.raises(cloud_exceptions.MissingRequiredFields) as e:
        from_http(
            headers, data,
        )
-    assert "Failed to read fields from structured event. " in str(e.value)
+    assert "Failed to read specversion from both headers and data" in str(
+        e.value
+    )


 def test_uppercase_headers_with_none_data_binary():
@ -472,3 +488,46 @@ def test_uppercase_headers_with_none_data_binary():

    _, new_data = to_binary(event)
    assert new_data == None
+
+
+def test_generic_exception():
+    headers = {"Content-Type": "application/cloudevents+json"}
+    data = json.dumps(
+        {
+            "specversion": "1.0",
+            "source": "s",
+            "type": "t",
+            "id": "1234-1234-1234",
+            "data": "",
+        }
+    )
+    with pytest.raises(cloud_exceptions.GenericException) as e:
+        from_http({}, None)
+    e.errisinstance(cloud_exceptions.MissingRequiredFields)
+
+    with pytest.raises(cloud_exceptions.GenericException) as e:
+        from_http({}, 123)
+    e.errisinstance(cloud_exceptions.InvalidStructuredJSON)
+
+    with pytest.raises(cloud_exceptions.GenericException) as e:
+        from_http(headers, data, data_unmarshaller=lambda x: 1 / 0)
+    e.errisinstance(cloud_exceptions.DataUnmarshallerError)
+
+    with pytest.raises(cloud_exceptions.GenericException) as e:
+        event = from_http(headers, data)
+        to_binary(event, data_marshaller=lambda x: 1 / 0)
+    e.errisinstance(cloud_exceptions.DataMarshallerError)
+
+
+def test_non_dict_data_no_headers_bug():
+    # Test for issue #116
+    headers = {"Content-Type": "application/cloudevents+json"}
+    data = "123"
+    with pytest.raises(cloud_exceptions.MissingRequiredFields) as e:
+        from_http(
+            headers, data,
+        )
+    assert "Failed to read specversion from both headers and data" in str(
+        e.value
+    )
+    assert "The following deserialized data has no 'get' method" in str(e.value)
--- a/cloudevents/tests/test_marshaller.py
+++ b/cloudevents/tests/test_marshaller.py
@ -12,15 +12,19 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.

+import json
+
 import pytest

+import cloudevents.exceptions as cloud_exceptions
+from cloudevents.http import CloudEvent, from_http, to_binary, to_structured
 from cloudevents.sdk import converters, exceptions, marshaller
 from cloudevents.sdk.converters import binary, structured
 from cloudevents.sdk.event import v1


@pytest.fixture
-def headers():
+def binary_headers():
    return {
        "ce-specversion": "1.0",
        "ce-source": "1.0",
@ -29,6 +33,19 @@ def headers():
    }


+@pytest.fixture
+def structured_data():
+    return json.dumps(
+        {
+            "specversion": "1.0",
+            "source": "pytest",
+            "type": "com.pytest.test",
+            "id": "1234-1234-1234",
+            "data": "test",
+        }
+    )
+
+
 def test_from_request_wrong_unmarshaller():
    with pytest.raises(exceptions.InvalidDataUnmarshaller):
        m = marshaller.NewDefaultHTTPMarshaller()
@ -41,7 +58,7 @@ def test_to_request_wrong_marshaller():
        _ = m.ToRequest(v1.Event(), data_marshaller="")


-def test_from_request_cannot_read(headers):
+def test_from_request_cannot_read(binary_headers):
    with pytest.raises(exceptions.UnsupportedEventConverter):
        m = marshaller.HTTPMarshaller(
            [binary.NewBinaryHTTPCloudEventConverter(),]
@ -52,7 +69,7 @@ def test_from_request_cannot_read(headers):
        m = marshaller.HTTPMarshaller(
            [structured.NewJSONHTTPCloudEventConverter()]
        )
-        m.FromRequest(v1.Event(), headers, "")
+        m.FromRequest(v1.Event(), binary_headers, "")


 def test_to_request_invalid_converter():
@ -61,3 +78,65 @@ def test_to_request_invalid_converter():
            [structured.NewJSONHTTPCloudEventConverter()]
        )
        m.ToRequest(v1.Event(), "")
+
+
+def test_http_data_unmarshaller_exceptions(binary_headers, structured_data):
+    # binary
+    with pytest.raises(cloud_exceptions.DataUnmarshallerError) as e:
+        from_http(binary_headers, None, data_unmarshaller=lambda x: 1 / 0)
+    assert (
+        "Failed to unmarshall data with error: "
+        "ZeroDivisionError('division by zero')" in str(e.value)
+    )
+
+    # structured
+    headers = {"Content-Type": "application/cloudevents+json"}
+    with pytest.raises(cloud_exceptions.DataUnmarshallerError) as e:
+        from_http(headers, structured_data, data_unmarshaller=lambda x: 1 / 0)
+    assert (
+        "Failed to unmarshall data with error: "
+        "ZeroDivisionError('division by zero')" in str(e.value)
+    )
+
+
+def test_http_data_marshaller_exception(binary_headers, structured_data):
+    # binary
+    event = from_http(binary_headers, None)
+    with pytest.raises(cloud_exceptions.DataMarshallerError) as e:
+        to_binary(event, data_marshaller=lambda x: 1 / 0)
+    assert (
+        "Failed to marshall data with error: "
+        "ZeroDivisionError('division by zero')" in str(e.value)
+    )
+
+    # structured
+    headers = {"Content-Type": "application/cloudevents+json"}
+
+    event = from_http(headers, structured_data)
+    with pytest.raises(cloud_exceptions.DataMarshallerError) as e:
+        to_structured(event, data_marshaller=lambda x: 1 / 0)
+    assert (
+        "Failed to marshall data with error: "
+        "ZeroDivisionError('division by zero')" in str(e.value)
+    )
+
+
+@pytest.mark.parametrize("test_data", [[], {}, (), "", b"", None])
+def test_known_empty_edge_cases(binary_headers, test_data):
+    expect_data = test_data
+    if test_data in ["", b""]:
+        expect_data = None
+    elif test_data == ():
+        # json.dumps(()) outputs '[]' hence list not tuple check
+        expect_data = []
+
+    # Remove ce- prefix
+    headers = {key[3:]: value for key, value in binary_headers.items()}
+
+    # binary
+    event = from_http(*to_binary(CloudEvent(headers, test_data)))
+    assert event.data == expect_data
+
+    # structured
+    event = from_http(*to_structured(CloudEvent(headers, test_data)))
+    assert event.data == expect_data