Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix incorrect error messages for from_http() #143

Closed
wants to merge 1 commit into from
Closed

fix incorrect error messages for from_http() #143

wants to merge 1 commit into from

Conversation

xinydev
Copy link
Contributor

@xinydev xinydev commented Aug 24, 2021

Fixes #139

Align the logic of has_binary_headers() with sdk-go

Using ce-specversion is enough.

func NewMessage(header nethttp.Header, body io.ReadCloser) *Message {
	m := Message{Header: header}
	if body != nil {
		m.BodyReader = body
	}
	if m.format = format.Lookup(header.Get(ContentType)); m.format == nil {
		m.version = specs.Version(m.Header.Get(specs.PrefixedSpecVersionName()))
	}
	return &m
}

func (m *Message) ReadEncoding() binding.Encoding {
	if m.version != nil {
		return binding.EncodingBinary
	}
	if m.format != nil {
		return binding.EncodingStructured
	}
	return binding.EncodingUnknown
}

Changes

One line description for the changelog

  • Tests pass
  • Appropriate changes to README are included in PR

@di di requested a review from grant August 24, 2021 13:58
@cloudevents cloudevents deleted a comment from xinydev Aug 24, 2021
@GrahamCampbell
Copy link
Contributor

This PR is missing tests covering the actual issue.

grant
grant suggested changes Sep 2, 2021
View reviewed changes
Copy link
Member

@grant grant left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR.

The spec states that binary CEs have required attributes in the headers.

There are 4 required attributes:

https://github.com/cloudevents/spec/blob/v1.0/spec.md#required-attributes

I'm not seeing how this fixes an issue with the function.

@grant
Copy link
Member

grant commented Sep 2, 2021

Align the logic of has_binary_headers() with sdk-go

The goal of the SDKs aren't to align to each other, but rather the CE spec. I'm a bit confused why this is mentioned here.

@GrahamCampbell
Copy link
Contributor

I think the complaint here is that the error message when it fails to parse is not very helpful? By removing the extra conditions on trying to process the message as a binary encoding, we are able to provide a more helpful error about missing required attributes, rather than instead trying to process it as structured.

@GrahamCampbell
Copy link
Contributor

Had there been a test included with this, this would have been clearer. :trollface: :P

@grant
Copy link
Member

grant commented Sep 2, 2021

Making the error message better sounds good. However, I don't see this PR fixing #139, so I'm inclined we modify the PR or close it. :)

@xinydev xinydev force-pushed the fix-binary-error branch 2 times, most recently from 5c066c2 to c0ac91f Compare September 3, 2021 03:14
@xinydev
fix incorrect error messages for from_http()
9cbb539 
add tests

fix lint

Signed-off-by: XinYang <xinydev@gmail.com>
@xinydev xinydev requested a review from grant September 3, 2021 03:21
@xinydev
Copy link
Contributor Author

xinydev commented Sep 6, 2021

PTAL, I added a test case

I don't see this PR fixing

Before modification, all http requests with missing necessary headers will be judged as structured content mode(but in fact is binary mode),and than raise an error in L65-L67(Failed to find specversion in HTTP request)

After modification, the http requests with missing necessary headers will throw an error through L76-L78

sdk-python/cloudevents/http/http_methods.py

Lines 46 to 78 in 705e8b4

if is_binary(headers):
specversion = headers.get("ce-specversion", None)
else:
try:
raw_ce = json.loads(data)
except json.decoder.JSONDecodeError:
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}"
)
if specversion is None:
raise cloud_exceptions.MissingRequiredFields(
"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}"
)
event = marshall.FromRequest(
event_handler(), headers, data, data_unmarshaller=data_unmarshaller
)

grant
grant suggested changes Sep 17, 2021
View reviewed changes
Copy link
Member

@grant grant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of the 4 required CE fields should be check and required.

cloudevents/sdk/converters/util.py
and "ce-source" in headers
and "ce-type" in headers
and "ce-id" in headers
or "ce-source" in headers
Copy link
Member

@grant grant Sep 17, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of these headers are required for binary CEs.

@grant
Copy link
Member

grant commented Sep 17, 2021

Perhaps come to CloudEvent WG to talk about the PR?

https://github.com/cloudevents/spec#meeting-time

@grant
Copy link
Member

grant commented Dec 10, 2021

Hi @xinydev, lmk if you're still working on this. I left some comments from before.

@xinydev
Copy link
Contributor Author

xinydev commented Dec 10, 2021 •
edited

Hi @xinydev, lmk if you're still working on this. I left some comments from before.

I'm very sorry, I don't have enough time to continue doing this recently.

@xinydev xinydev closed this Dec 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grant grant grant requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

cloudevents.http.from_http binary incorrect error messages
3 participants
@xinydev @GrahamCampbell @grant