sdk-python icon indicating copy to clipboard operation
sdk-python copied to clipboard

cloudevents.http.from_http binary incorrect error messages

Open klueken opened this issue 4 years ago • 2 comments

Expected Behavior

When using cloudevents.http.from_http(headers, body), an event that is missing any of the required fields should result in a cloud_exceptions.MissingRequiredFields exception with a message that indicates which field is missing.

Actual Behavior

When given a Binary Cloud Event that is missing a required field ce-id, ce-source, or ce-type, it will return a MissingRequiredFields error with the incorrect error message Failed to find specversion in HTTP request.

Steps to Reproduce the Problem

from cloudevents.http import from_http
from cloudevents.exceptions import MissingRequiredFields

# Correctly does not result in an error if all required fields are present
def test_from_http():
    event = from_http({"ce-specversion": "1.0", "ce-id":"123", "ce-type": "test-type", "ce-source": "test-source"}, "{}")
    assert event["id"] == "123"

# Returns an incorrect error message
def test_from_http_missing_id_binary():
    try:
        event = from_http({"ce-specversion": "1.0", "ce-type": "test-type", "ce-source": "test-source"}, "{}")
        assert 1 == 2
    except MissingRequiredFields as e:
        assert "Failed to find specversion in HTTP request" == str(e)

# Returns the appropriate message
def test_from_http_missing_id_structured():
    try:
        event = from_http({}, "{\"specversion\": \"1.0\", \"type\": \"test-type\", \"source\": \"test-source\"}")
        assert 1 == 2
    except MissingRequiredFields as e:
        assert "Missing required attributes: {'id'}" == str(e)

The code flow is as follows:

  1. it checks is_binary(headers) https://github.com/cloudevents/sdk-python/blob/b83bfc58eb851f9b91a96f4665754d9bb82cd74e/cloudevents/http/http_methods.py#L46
  2. It calls binary_parser.can_read https://github.com/cloudevents/sdk-python/blob/b83bfc58eb851f9b91a96f4665754d9bb82cd74e/cloudevents/http/event_type.py#L6-L16
  3. it calls has_binary_headers https://github.com/cloudevents/sdk-python/blob/b83bfc58eb851f9b91a96f4665754d9bb82cd74e/cloudevents/sdk/converters/binary.py#L29-L35
  4. has_binary_headers checks for the presence of all required fields, which in this test case is false because it is missing ce-id https://github.com/cloudevents/sdk-python/blob/b83bfc58eb851f9b91a96f4665754d9bb82cd74e/cloudevents/sdk/converters/util.py#L4-L10
  5. it then falls through and tries to get the specversion here https://github.com/cloudevents/sdk-python/blob/b83bfc58eb851f9b91a96f4665754d9bb82cd74e/cloudevents/http/http_methods.py#L57
  6. specversion is never set and the error gets thrown here https://github.com/cloudevents/sdk-python/blob/b83bfc58eb851f9b91a96f4665754d9bb82cd74e/cloudevents/http/http_methods.py#L64-L67

I think that the solution might be as simple as changing all of the ands to ors in the has_binary_headers method in step 4.

Specifications

  • Platform: Mac OS
  • Python Version: 3.9.4

klueken avatar Jun 21 '21 21:06 klueken

I would also like to see this fixed. It would greatly improve the user experience. What I would also consider doing in this step is adding an attribute to the MissingRequiredFields exception that specifies which fields are actually missing. This would make it easier to handle the exception by another application.

sp-schoen avatar Jun 23 '21 12:06 sp-schoen

We are seeing this issue, and debugging was a headache because of it. It looks like the PR went stale with no outcome from a WG meeting -- is this true?

bbtong avatar Nov 24 '21 22:11 bbtong