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.

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

Fix regression with json marshaller/unmarshaller #675

Merged
merged 2 commits into from
Apr 12, 2021

Conversation

slinkydeveloper
Copy link
Member

Fix the unmarshalling when there is some json in the event data, but there is no content type.

Signed-off-by: Francesco Guardiani francescoguard@gmail.com

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
@@ -359,7 +359,8 @@ func consumeDataAsBytes(e *Event, isBase64 bool, b []byte) error {
}

mt, _ := e.Context.GetDataMediaType()
if mt != ApplicationJSON && mt != TextJSON {
// Empty content type assumes json
if mt != "" && mt != ApplicationJSON && mt != TextJSON {
Copy link
Member

Choose a reason for hiding this comment

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

This will throw new errors when the body is just an unquoted string or random bytes.

Copy link
Member Author

@slinkydeveloper slinkydeveloper Apr 12, 2021

Choose a reason for hiding this comment

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

Why? Can you provide a test case where do you think this is going to fail? In any case, the spec claims that this assumption is safe (at least, that's my understanding): https://github.com/cloudevents/spec/blob/v1.0.1/json-format.md#31-handling-of-data

Copy link
Member

Choose a reason for hiding this comment

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

ah ok this is already in the context of structured mode decoding.

LGTM

@slinkydeveloper slinkydeveloper merged commit a238efa into cloudevents:main Apr 12, 2021
@slinkydeveloper slinkydeveloper deleted the fix_regression_json branch April 12, 2021 15:25
@matzew
Copy link
Member

matzew commented Apr 14, 2021

@n3wscott @slinkydeveloper will this fix result in a 2.4.1 release ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants