Format URL encoded forms #323

gm-vm · 2020-04-11T14:04:46Z

📷 Screenshots

Current	Formatted

📄 Context

Reading encoded forms is nearly impossible, especially when dealing values have symbols in them. Unlike for XML and JSON requests, this does not simply pretty print the body and that may be undesired.

cortinico

Thanks for the PR. Having a test would definitely be beneficial giving the effort we're doing in increasing the coverage.

Moreover I was wondering if we should hook this feature inside the URL encode/decode button we already have. @vbuberen do you have any ideas about this?

cortinico · 2020-04-12T12:58:56Z

library/src/main/java/com/chuckerteam/chucker/internal/support/FormatUtils.kt

@@ -94,6 +96,19 @@ internal object FormatUtils {
        }
    }

+    fun formatUrlEncodedForm(form: String): String {


Please add a test for this function.

Sure, I'll also rewrite the method not to make any assumption on the input.

gm-vm · 2020-04-12T13:38:15Z

I thought about connecting the existing button, but I had some doubts and, honestly, I don't know why I didn't mention them.

What I've just come up with is the following: I'd add a new button that allows to switch between two display modes: "raw" and "formatted". This button could be either the existing button or a new one, not sure what's better. I simply don't want to add request-specific parameters to HttpTransaction and this is the only way to do that. I'd also include responses, just for consistency.

But how should the share button behave? Would it follow the current display mode? Should it include both the formatted request and the raw request? Just the one currently displayed?

cortinico · 2020-04-12T14:38:31Z

This button could be either the existing button or a new one, not sure what's better. I simply don't want to add request-specific parameters to HttpTransaction and this is the only way to do that. I'd also include responses, just for consistency.

Agree. The stored information should be unchanged. What we're addressing here is just on the presentation layer.

If possible I would hook to the same button we already have on the URL side. The semantic is pretty clear as it's an encoded/decoded toggle.

gm-vm · 2020-04-12T15:46:35Z

Agree. The stored information should be unchanged. What we're addressing here is just on the presentation layer.

My bad, I didn't notice you were also exposing the original request body, which makes sense, and thought I had to do some invasive changes.

Anyway, I hooked the existing button. I should probably implement something like doesUrlRequireEncoding and maybe rename some variables, but first let me know if something like this is OK.

vbuberen · 2020-04-13T09:15:59Z

Joining the conversation here - agree with @cortinico that we should attach this feature to an existing button. However, we might need to reevaluate UI in next PRs (I suggest to do it in first of 4.x releases).

vbuberen

Thanks for doing such a useful improvement. Some changes required before we can merge it.

vbuberen · 2020-04-13T09:22:02Z

library/src/main/java/com/chuckerteam/chucker/internal/data/entity/HttpTransaction.kt

@@ -196,6 +196,8 @@ internal class HttpTransaction(
                FormatUtils.formatJson(body)
            contentType != null && contentType.contains("xml", ignoreCase = true) ->
                FormatUtils.formatXml(body)
+            contentType != null && contentType.contains("form-urlencoded", ignoreCase = true) ->


I see that we already have 3 checks for contentType != null in this when. Can I ask to you to refactor it to be something like this (or how you would prefer it):

return if (contentType.isNullOrBlank()) { body } else { when { contentType.contains("json", ignoreCase = true) -> FormatUtils.formatJson(body) contentType.contains("xml", ignoreCase = true) -> FormatUtils.formatXml(body) else -> body } }

vbuberen · 2020-04-13T09:23:46Z

.../src/main/java/com/chuckerteam/chucker/internal/ui/transaction/TransactionPayloadFragment.kt

                    hideProgress()
                }
            }
        )
    }

+    private suspend fun updateUpdater(


I guess there is a typo and this fun meant to be named updateAdapter ?

Definitely, I had a typo there and instead of fixing it my brain decided to make it worse.

vbuberen · 2020-04-13T10:27:23Z

.../src/main/java/com/chuckerteam/chucker/internal/ui/transaction/TransactionPayloadFragment.kt

-                    val result = processPayload(type, transaction)
-                    payloadBinding.responseRecyclerView.adapter = TransactionBodyAdapter(result)
-                    payloadBinding.responseRecyclerView.setHasFixedSize(true)
+                    val encode = viewModel.encodeUrl.value ?: true


Why don't you call this variable the same way as you named parameter updateAdapter fun?

As I mentioned, I need to rename some stuff. I started with just the formatting of url encoded forms and ended up with more. I was wondering if I should stick with just the formatting of url encoded forms or continue with this more generic solution.

vbuberen · 2020-04-13T10:28:59Z

.../src/main/java/com/chuckerteam/chucker/internal/ui/transaction/TransactionPayloadFragment.kt

+        viewModel.encodeUrl.observe(
+            viewLifecycleOwner,
+            Observer { encode ->
+                if (adapter.itemCount == 0) {


Why do you need this check if you also do the check for transaction value below?

Definitely something I should have commented. There are cases in which transaction is not null here (e.g., configuration change), so you end up notifying the adapter twice: once here and once when observing transaction. Not a big deal, but it's unnecessary work.

I tried to think of a way not to have this here and the only thing I could think of is having a separate transaction LiveData that under the hood is just a MediatorLiveData that combines transaction and encodeUrl, taking care of notifying observers only once the first time it's observed. It didn't look pretty and it was more convoluted than this simple check, which is just a micro-optimization and not something required. With or without this check everything would work just fine. But I don't really like the fact that this trick only works if you start observing this LiveData before the other.

vbuberen · 2020-04-13T10:32:01Z

.../src/main/java/com/chuckerteam/chucker/internal/ui/transaction/TransactionPayloadFragment.kt

-        menu.findItem(R.id.encode_url).isVisible = false
+        menu.findItem(R.id.encode_url).apply {
+            isVisible = true
+            setOnMenuItemClickListener {


This options item already had click listener in TransactionActivity and called exactly the same fun from ViewModel.

vbuberen · 2020-04-13T10:43:58Z

library/src/main/java/com/chuckerteam/chucker/internal/support/FormatUtils.kt

+                return form
+            }
+            form.split("&").joinToString(separator = "\n") { entry ->
+                val split = entry.split("=")


Rename this val please for more clarity.

vbuberen · 2020-04-13T10:49:00Z

.../src/main/java/com/chuckerteam/chucker/internal/ui/transaction/TransactionPayloadFragment.kt

+        transaction: HttpTransaction,
+        formatted: Boolean
+    ) {
+        data.clear()


In general this function shouldn't be in this fragment. It should be inside the adapter itself named something like setItems().

Also, in case of having such thing as setItems() function you can remove adapter initialisation with an empty list and make its declaration lazy for cases when we have empty payload.

Definitely, I didn't like this to begin with and I guess I just didn't want to change too many things. I'll fix this. But I'm not sure I get exactly what you meant with your proposed solution. Are you suggesting the use of a lazy property or something else?

Here I can only see a nullable list as marginally better, given that the moment you attach the adapter to the RecyclerView its getItemCount() gets called, so anything that creates the list lazily would do it immediately.

Yes, I suggest to have a lazy property and create it only when we have some result to show.
However, now I think it might be not the best idea, since it will cause logcat show No adapter attached, like it does now.

We don't have to worry about injections, so disable HTML escaping and display the real values.

gm-vm · 2020-04-18T22:54:53Z

I rebased my changes and applied your suggestions.

I also changed the way things work. The button now only encodes/decodes url encoded requests.

I don't really like that formatRequestBody boolean there. I gave it a generic name, but we know that's a lie. I'd rather have the fragment not do anything and receive already formatted bodies. And maybe not as simple Strings, but something like a Spannable, which I see is what we end up with anyway, or some other complex object. This would allow to format nicely certain type of requests.

For example, application/x-www-form-urlencoded requests are generally represented as I'm doing here, but keys and values are generally shown either with different colors or font weights. Then there are also multipart/form-data, where things get a bit more complicated.

vbuberen · 2020-04-20T09:30:16Z

I don't really like that formatRequestBody boolean there. I gave it a generic name, but we know that's a lie. I'd rather have the fragment not do anything and receive already formatted bodies.

Agree with you. It is something I also suggested recently. Look at Next steps here #325 (comment) . TL:DR - it left since times we had no view models. But it is going to change soon.

vbuberen

LGTM 🚢

I would like to wait for another review from @cortinico as well, since he reviewed this PR as well.

cortinico

LGTM, just left a couple of minor comments

cortinico · 2020-04-23T23:46:48Z

library/src/main/java/com/chuckerteam/chucker/internal/data/entity/HttpTransaction.kt

+        return if (contentType.isNullOrBlank()) {
+            body
+        } else {
+            when {
+                contentType.contains("json", ignoreCase = true) -> FormatUtils.formatJson(body)
+                contentType.contains("xml", ignoreCase = true) -> FormatUtils.formatXml(body)
+                contentType.contains("x-www-form-urlencoded", ignoreCase = true) ->
+                    FormatUtils.formatUrlEncodedForm(body)
+                else -> body
+            }


Suggested change

return if (contentType.isNullOrBlank()) {

body

} else {

when {

contentType.contains("json", ignoreCase = true) -> FormatUtils.formatJson(body)

contentType.contains("xml", ignoreCase = true) -> FormatUtils.formatXml(body)

contentType.contains("x-www-form-urlencoded", ignoreCase = true) ->

FormatUtils.formatUrlEncodedForm(body)

else -> body

}

return when {

contentType.isNullOrBlank() -> body

contentType.contains("json", ignoreCase = true) -> FormatUtils.formatJson(body)

contentType.contains("xml", ignoreCase = true) -> FormatUtils.formatXml(body)

contentType.contains("x-www-form-urlencoded", ignoreCase = true) ->

FormatUtils.formatUrlEncodedForm(body)

else -> body

}

Please keep the single return when style as it's easier to read and follow.

library/src/main/java/com/chuckerteam/chucker/internal/support/FormatUtils.kt

cortinico · 2020-04-23T23:57:44Z

library/src/main/java/com/chuckerteam/chucker/internal/ui/transaction/TransactionViewModel.kt

+            if (transaction == null) {
+                false
+            } else {
+                transaction.requestContentType?.contains("x-www-form-urlencoded", ignoreCase = true) ?: false
+            }


Suggested change

if (transaction == null) {

false

} else {

transaction.requestContentType?.contains("x-www-form-urlencoded", ignoreCase = true) ?: false

}

transaction?.requestContentType?.contains("x-www-form-urlencoded", ignoreCase = true) ?: false

vbuberen · 2020-04-24T17:02:11Z

@cortinico So, we are good to merge now? I see that PR is updated already.

cortinico · 2020-04-24T18:05:49Z

Yes 👍

cortinico requested changes Apr 12, 2020

View reviewed changes

cortinico added the enhancement New feature or improvement to the library label Apr 12, 2020

gm-vm requested a review from cortinico April 12, 2020 15:59

vbuberen requested changes Apr 13, 2020

View reviewed changes

vbuberen mentioned this pull request Apr 15, 2020

Empty state for payloads #325

Merged

Disable GSON's HTML escaping

b55a231

We don't have to worry about injections, so disable HTML escaping and display the real values.

vbuberen approved these changes Apr 20, 2020

View reviewed changes

cortinico approved these changes Apr 23, 2020

View reviewed changes

Format URL encoded requests

57d86f7

cortinico merged commit 921608f into ChuckerTeam:develop Apr 24, 2020

cortinico added this to the 3.3.0 milestone Apr 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Format URL encoded forms #323

Format URL encoded forms #323

gm-vm commented Apr 11, 2020

cortinico left a comment

cortinico Apr 12, 2020

gm-vm Apr 12, 2020

gm-vm commented Apr 12, 2020

cortinico commented Apr 12, 2020

gm-vm commented Apr 12, 2020

vbuberen commented Apr 13, 2020

vbuberen left a comment

vbuberen Apr 13, 2020

vbuberen Apr 13, 2020

gm-vm Apr 13, 2020

vbuberen Apr 13, 2020

gm-vm Apr 13, 2020

vbuberen Apr 13, 2020

gm-vm Apr 13, 2020

vbuberen Apr 13, 2020

vbuberen Apr 13, 2020

vbuberen Apr 13, 2020

vbuberen Apr 13, 2020

gm-vm Apr 13, 2020

vbuberen Apr 14, 2020

gm-vm commented Apr 18, 2020 •

edited

Loading

vbuberen commented Apr 20, 2020

vbuberen left a comment

cortinico left a comment

cortinico Apr 23, 2020

cortinico Apr 23, 2020

vbuberen commented Apr 24, 2020

cortinico commented Apr 24, 2020

Format URL encoded forms #323

Format URL encoded forms #323

Conversation

gm-vm commented Apr 11, 2020

📷 Screenshots

📄 Context

cortinico left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gm-vm commented Apr 12, 2020

cortinico commented Apr 12, 2020

gm-vm commented Apr 12, 2020

vbuberen commented Apr 13, 2020

vbuberen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gm-vm commented Apr 18, 2020 • edited Loading

vbuberen commented Apr 20, 2020

vbuberen left a comment

Choose a reason for hiding this comment

cortinico left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbuberen commented Apr 24, 2020

cortinico commented Apr 24, 2020

gm-vm commented Apr 18, 2020 •

edited

Loading