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

support dynamic data content type #471

Merged

Conversation

segator
Copy link
Contributor

@segator segator commented Aug 24, 2022

Fixes #470
Now checks if dataContentType matches the next regex.

^(application|text)\/([a-zA-Z]+\+)?json$")

This regex support
application/foobar+json
or standard

application/json
text/json

@segator segator force-pushed the bugfix/support_dynamic_datacontenttype branch 3 times, most recently from 0372101 to 15a7e3c Compare August 24, 2022 12:56
@segator segator force-pushed the bugfix/support_dynamic_datacontenttype branch from 15a7e3c to d34b27f Compare August 24, 2022 12:58
@n3wscott
Copy link
Member

n3wscott commented Sep 1, 2022

Makes sense. Likely a change we should do in the go sdk too to support application/foobar+json

@matejvasek
Copy link
Contributor

matejvasek commented Sep 1, 2022

Just maybe rename PATTERN_CONTENT_TYPE to JSON_CONTENT_TYPE_PATTERN.

Signed-off-by: Isaac Aymerich <[email protected]>
Signed-off-by: Isaac Aymerich <[email protected]>
@segator
Copy link
Contributor Author

segator commented Sep 2, 2022

Just maybe rename PATTERN_CONTENT_TYPE to JSON_CONTENT_TYPE_PATTERN.

updated

@@ -43,7 +44,10 @@ public final class JsonFormat implements EventFormat {
* Content type associated with the JSON event format
*/
public static final String CONTENT_TYPE = "application/cloudevents+json";

/**
* Suppoted Content type
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure if it's good to call it "Supported Content type".
Other content type should work too, they will just be exposed as byte[] not objects IIRC.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood, any suggestion for the naming?

Copy link
Contributor

@JemDay JemDay Sep 2, 2022

Choose a reason for hiding this comment

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

How about something along the lines of "JSON Data Content Type Discriminator" - long-winded I know ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JemDay Done!

@JemDay
Copy link
Contributor

JemDay commented Sep 2, 2022

While we're changing this test-case could we also enhance it to ensure that the 'datacontenttype' in the deserialized event matches the 'contentType' that was used to construct the original event (from your test argument list)

Just an extra level of sanity checking.

@segator
Copy link
Contributor Author

segator commented Sep 3, 2022

While we're changing this test-case could we also enhance it to ensure that the 'datacontenttype' in the deserialized event matches the 'contentType' that was used to construct the original event (from your test argument list)

Just an extra level of sanity checking.

Done!

@JemDay
Copy link
Contributor

JemDay commented Sep 3, 2022

Thanks LGTM....

@matejvasek
Copy link
Contributor

/cc @pierDipi

Copy link
Member

@pierDipi pierDipi left a comment

Choose a reason for hiding this comment

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

Thanks @segator, LGTM

@pierDipi pierDipi merged commit f8d27b0 into cloudevents:master Sep 5, 2022
@segator segator deleted the bugfix/support_dynamic_datacontenttype branch September 5, 2022 18:17
abutch3r pushed a commit to abutch3r/sdk-java that referenced this pull request Sep 22, 2022
Now checks if `datacontenttype` matches the regex:

`^(application|text)\/([a-zA-Z]+\+)?json$")`

This regex support 
`application/foobar+json`
or standard
```
application/json
text/json
```

Signed-off-by: Isaac Aymerich <[email protected]>
Signed-off-by: alex-butcher <[email protected]>
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.

JSON content type detection not spec compliant
6 participants