Explicitly set MediaType for different body types#164
Explicitly set MediaType for different body types#164ddzobov wants to merge 3 commits intomattermost:masterfrom
Conversation
Implicit set MediaType for different body types
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 47 minutes and 27 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughModified the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
android/src/main/java/com/mattermost/networkclient/NetworkClient.kt (1)
424-436:⚠️ Potential issue | 🟠 MajorPOST requests with
null/missing body lack explicitContent-Typeheader.Lines 425 and 435 use
EMPTY_REQUESTwithout setting a media type, while all other body types (Array, Map, String, Boolean, Number) explicitly set media types viaMediaType.parse(). This inconsistency can cause WAF or server-side validation to reject empty-body POST requests that lack aContent-Typeheader. Align these cases with the existing pattern.Suggested fix
ReadableType.Null -> { - requestBody = EMPTY_REQUEST + requestBody = "".toRequestBody(MediaType.parse("text/plain; charset=utf-8")) }} else if (method.uppercase(Locale.ENGLISH) == "POST") { - requestBody = EMPTY_REQUEST + requestBody = "".toRequestBody(MediaType.parse("text/plain; charset=utf-8")) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/src/main/java/com/mattermost/networkclient/NetworkClient.kt` around lines 424 - 436, When the request body is null or omitted the code currently assigns EMPTY_REQUEST (in NetworkClient.kt) which provides no Content-Type; update the null-body branch and the POST-without-body branch to create an empty RequestBody with an explicit media type (match the other branches' MediaType.parse("text/plain; charset=utf-8") usage) instead of raw EMPTY_REQUEST so the resulting request always has a Content-Type header; adjust any use of EMPTY_REQUEST in the branches handling ReadableType.Null and the method.uppercase(...) == "POST" check (and factor into a named EMPTY_REQUEST_WITH_MEDIA constant if helpful) so behavior is consistent with String/Boolean/Number branches.
🧹 Nitpick comments (1)
android/src/main/java/com/mattermost/networkclient/NetworkClient.kt (1)
411-431: Replace repeatedMediaType.parse()calls with class-level constants usingtoMediaType().The code parses the same two media type strings five times. Add class-level constants and import
toMediaType()to eliminate redundant parsing and improve performance.Suggested refactor
@@ import okhttp3.* import okhttp3.RequestBody.Companion.toRequestBody +import okhttp3.MediaType.Companion.toMediaType @@ internal class NetworkClient(private val context: Context, private val baseUrl: HttpUrl? = null, options: ReadableMap? = null, cookieJar: CookieJar? = null) { private var okHttpClient: OkHttpClient + private val jsonUtf8MediaType = "application/json; charset=utf-8".toMediaType() + private val textUtf8MediaType = "text/plain; charset=utf-8".toMediaType() @@ requestBody = jsonBody.toString().toRequestBody(MediaType.parse("application/json; charset=utf-8")) - requestBody = jsonBody.toString().toRequestBody(jsonUtf8MediaType) + requestBody = jsonBody.toString().toRequestBody(jsonUtf8MediaType) @@ requestBody = jsonBody?.toString()?.toRequestBody(MediaType.parse("application/json; charset=utf-8")) - requestBody = jsonBody?.toString()?.toRequestBody(jsonUtf8MediaType) + requestBody = jsonBody?.toString()?.toRequestBody(jsonUtf8MediaType) @@ requestBody = options.getString("body")!!.toRequestBody(MediaType.parse("text/plain; charset=utf-8")) - requestBody = options.getString("body")!!.toRequestBody(textUtf8MediaType) + requestBody = options.getString("body")!!.toRequestBody(textUtf8MediaType) @@ requestBody = options.getBoolean("body").toString().toRequestBody(MediaType.parse("text/plain; charset=utf-8")) - requestBody = options.getBoolean("body").toString().toRequestBody(textUtf8MediaType) + requestBody = options.getBoolean("body").toString().toRequestBody(textUtf8MediaType) @@ requestBody = options.getDouble("body").toString().toRequestBody(MediaType.parse("text/plain; charset=utf-8")) - requestBody = options.getDouble("body").toString().toRequestBody(textUtf8MediaType) + requestBody = options.getDouble("body").toString().toRequestBody(textUtf8MediaType)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/src/main/java/com/mattermost/networkclient/NetworkClient.kt` around lines 411 - 431, Replace repeated MediaType.parse() calls by declaring class-level constants (e.g., MEDIA_TYPE_JSON and MEDIA_TYPE_TEXT) using the okhttp3 extension toMediaType() and reuse them when creating requestBody in NetworkClient.kt; add the import okhttp3.MediaType.Companion.toMediaType, define something like private val MEDIA_TYPE_JSON = "application/json; charset=utf-8".toMediaType() and private val MEDIA_TYPE_TEXT = "text/plain; charset=utf-8".toMediaType(), then update the requestBody assignments in the body-handling switch (the ReadableType.Map/String/Boolean/Number branches) to pass MEDIA_TYPE_JSON or MEDIA_TYPE_TEXT instead of calling MediaType.parse() repeatedly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@android/src/main/java/com/mattermost/networkclient/NetworkClient.kt`:
- Around line 424-436: When the request body is null or omitted the code
currently assigns EMPTY_REQUEST (in NetworkClient.kt) which provides no
Content-Type; update the null-body branch and the POST-without-body branch to
create an empty RequestBody with an explicit media type (match the other
branches' MediaType.parse("text/plain; charset=utf-8") usage) instead of raw
EMPTY_REQUEST so the resulting request always has a Content-Type header; adjust
any use of EMPTY_REQUEST in the branches handling ReadableType.Null and the
method.uppercase(...) == "POST" check (and factor into a named
EMPTY_REQUEST_WITH_MEDIA constant if helpful) so behavior is consistent with
String/Boolean/Number branches.
---
Nitpick comments:
In `@android/src/main/java/com/mattermost/networkclient/NetworkClient.kt`:
- Around line 411-431: Replace repeated MediaType.parse() calls by declaring
class-level constants (e.g., MEDIA_TYPE_JSON and MEDIA_TYPE_TEXT) using the
okhttp3 extension toMediaType() and reuse them when creating requestBody in
NetworkClient.kt; add the import okhttp3.MediaType.Companion.toMediaType, define
something like private val MEDIA_TYPE_JSON = "application/json;
charset=utf-8".toMediaType() and private val MEDIA_TYPE_TEXT = "text/plain;
charset=utf-8".toMediaType(), then update the requestBody assignments in the
body-handling switch (the ReadableType.Map/String/Boolean/Number branches) to
pass MEDIA_TYPE_JSON or MEDIA_TYPE_TEXT instead of calling MediaType.parse()
repeatedly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d0b3c6f5-ab6d-4436-b872-c44559237647
📒 Files selected for processing (1)
android/src/main/java/com/mattermost/networkclient/NetworkClient.kt
add explicit content-type for empty requests
Refactoring from review comments
|
Thanks for the PR @ddzobov as soon as I have a bit of time, I'll check if this in fact resolves the issue with WAF. But two things:
|
|
So if it works in both cases, I rather not do this here and set the default headers on the mobile app instead and not in the library |
Yes, but current logic in iOS code not sets application/json header explicitly too, currently it determines header implicitly based on serialization type, so with changes in this PR logic will be equal. If we will set headers on the mobile app, we need to do more complex changes in each request in each class call for request. |
|
This PR has been automatically labelled "stale" because it hasn't had recent activity. |
|
Hello |
3 participants
Summary
If Mattermost server published behind WAF, then WAF may block requests from Android clients, because by default it not sending any Content-Type headers in POST requests.
okhttp3 not sets Content-Type headers by default, so we need to do this explicitly.
mattermost/mattermost-mobile#9689