728 payload content-type check & error handling #738
Conversation
| } | ||
|
|
||
| private boolean isOfType(String contentType, String expectedContentType, String expectedWithDelim) { | ||
| return contentType != null && (contentType.length() > expectedContentType.length() ? |
There was a problem hiding this comment.
shouldn't we just check startsWith?
There was a problem hiding this comment.
also, content type is not case sensitive (https://www.w3.org/Protocols/rfc1341/4_Content-Type.html) so we can match with String.toLowerCase
There was a problem hiding this comment.
checking with startsWith doesn't enforce ';' to be the first character after content-type value.
It would accept values like 'application/jsonANYTEXT'
Do we want to allow such values?
There was a problem hiding this comment.
no, the current algorithm is fine
There was a problem hiding this comment.
Good point.
We can still simplify the check
contentType != null && (contentType.equals(expectedContentType)) || contentType.startsWith(expectedWithDelim))
String.equals has a check for length.
| @@ -22,4 +23,12 @@ public UnsupportedContentTypeException(Subscription subscription) { | |||
| )); | |||
| } | |||
|
|
|||
| public UnsupportedContentTypeException(String payloadContentType, Schema schema) { | |||
There was a problem hiding this comment.
why Avro Schema here? i don't get it, we probably want topic name, not schema name
There was a problem hiding this comment.
That's right. In most cases schema name matches topic name, and tests didn't catch that typo.
Fixed
| } | ||
|
|
||
| private boolean isOfType(String contentType, String expectedContentType, String expectedWithDelim) { | ||
| return contentType != null && (contentType.length() > expectedContentType.length() ? |
There was a problem hiding this comment.
no, the current algorithm is fine
Added check which accepts only "binary/avro" or "application/json" message content-type.
In other cases, http status 400 and proper error message will be returned.
#728