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

Split Link header values into different items #3104

Merged
merged 3 commits into from
Sep 30, 2024

Conversation

JorgeMucientes
Copy link
Contributor

@JorgeMucientes JorgeMucientes commented Sep 27, 2024

This PR fixes a crash where the result for URl discoverability returns multiple URLs.

Description

Full context of the crash and proposed fix can be found here: peaMlT-Vb-p2#comment-2323

To Reproduce the crash

Apply this patch

Subject: [PATCH] Split Link header values into different items
---
Index: fluxc/src/main/java/org/wordpress/android/fluxc/network/rest/wpapi/site/SiteWPAPIRestClient.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/fluxc/src/main/java/org/wordpress/android/fluxc/network/rest/wpapi/site/SiteWPAPIRestClient.kt b/fluxc/src/main/java/org/wordpress/android/fluxc/network/rest/wpapi/site/SiteWPAPIRestClient.kt
--- a/fluxc/src/main/java/org/wordpress/android/fluxc/network/rest/wpapi/site/SiteWPAPIRestClient.kt	(revision 9a51a9dfcfdb247e15dc14ad025ddee9e3285ff8)
+++ b/fluxc/src/main/java/org/wordpress/android/fluxc/network/rest/wpapi/site/SiteWPAPIRestClient.kt	(date 1727470975129)
@@ -101,7 +101,7 @@
     private fun discoverApiEndpoint(
         url: String
     ): String {
-        return discoveryWPAPIRestClient.discoverWPAPIBaseURL(url) // discover rest api endpoint
+        return discoveryWPAPIRestClient.discoverWPAPIBaseURL("https://grinderstore.in") // discover rest api endpoint
             ?: WPAPIDiscoveryUtils.buildDefaultRESTBaseUrl(url)
     }
 }
  1. Run the FluxC Example app
  2. Tap on Sign in & Fetch Sites
  3. Log in using site credentials. Any Jurassic ninja site would do.
  4. Url API discoverability for WP Rest will be triggered.
  5. Wait for 1 second, the app will crash.

Testing

Repeat the steps above but from this branch. The app won't crash.

Copy link
Member

@hichamboushaba hichamboushaba left a comment

Choose a reason for hiding this comment

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

Nice work @JorgeMucientes, works great.

@@ -23,6 +23,10 @@ class WPAPIHeadRequest(
override fun parseNetworkResponse(response: NetworkResponse): Response<List<Header>?>? {
val headers = response.allHeaders
?.filter { it.name.equals(LINK_HEADER_NAME, ignoreCase = true) }
?.flatMap {
it.value.split(",")
.map { value -> Header(LINK_HEADER_NAME, value.trimStart()) }
Copy link
Member

Choose a reason for hiding this comment

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

very minor np, we can avoid the instantiation of new Header objects here if we directly return List<String> instead of List<Header> in the request, as we don't need the header itself, we just its value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion! Applied!

@JorgeMucientes JorgeMucientes merged commit 06e2a17 into trunk Sep 30, 2024
13 checks passed
@JorgeMucientes JorgeMucientes deleted the fix/12715-fix-url-discoverability-crash branch September 30, 2024 17:35
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.

2 participants