-
Notifications
You must be signed in to change notification settings - Fork 165
{{ message }}
fix: strip parameters from data content types to assess if it's JSON format #484
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Author
|
Fixes #481 |
Contributor
|
** This may apply to much more than just the JSON format serialization. Don't the various transport bindings also check for content-type such as application/cloudevents+json ?? theoretically those checks should also include this type of stripping. I'm not sure if now is the time but we may want to break all of this stuff out into a some core utilities that can be shared across all of the formats and transports .. most specifically "take a content-type and strip off all the trailing attributes" and "isJson(stripped-content-type)" type functions. Not sure if we should address this now or add it as a followup .. it might need a bit more analysis to see where everybody is doing a comparison on content-type. |
5a1c42d to
00a37de
Compare
Contributor
Author
|
@JemDay I'm not sure this comment is directed to me but... it makes sense to me, for the potential to be needed elsewhere. However, I'm not familiar with this particular project, so I'd individually prefer to defer refactoring in a different context than this PR. |
JemDay
previously approved these changes
Oct 4, 2022
Contributor
There was a problem hiding this comment.
LGTM .. simplifying the regex would be nice for efficient processing but could be a follow-on activity.
| * JSON Data Content Type Discriminator | ||
| */ | ||
| private static final Pattern JSON_CONTENT_TYPE_PATTERN = Pattern.compile("^(application|text)\\/([a-zA-Z]+\\+)?json$"); | ||
| private static final Pattern JSON_CONTENT_TYPE_PATTERN = Pattern.compile("^(application|text)\\/([a-zA-Z]+\\+)?json(;[\\s\\w\"!#$%&'*+.^_`{|}~=-]*)*$"); |
Contributor
There was a problem hiding this comment.
I wonder if this regex can be simplified so that it simply matches anything following the ";" so something like "(;.*)*$" - my regex may be a bit rusty. ;-)
Contributor
Author
There was a problem hiding this comment.
Thanks, I was overdoing it :)
…format Signed-off-by: Frederic Delechamp <fdelechamp@guidewire.com>
4d42d08 to
04fd4b8
Compare
Member
|
Thanks @fredoboulo! |
Member
|
@fredoboulo would you be willing to backport the merged commit 6362bfb to the 2.4 branch for a patch release? |
Contributor
Author
|
Hi @pierDipi, that's about cherry-picking and making a PR in branch 2.4? |
fredoboulo
added a commit
to fredoboulo/cloudevents-sdk-java
that referenced
this pull request
Oct 12, 2022
cloudevents#484) Signed-off-by: Frederic Delechamp <fdelechamp@guidewire.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #481
Signed-off-by: Frederic Delechamp fdelechamp@guidewire.com