506015e · opencode-android

app/src/main/java/soy/iko/opencode/core/HttpClient.kt+17−7

compare with master

unifiedsplit

@@ -32,18 +32,28 @@ class HttpClient(

32 private val jsonMedia = "application/json".toMediaType()

33

34 suspend fun get(url: String, auth: String?): Result<HttpResp> =

35− exec(Request.Builder().url(url).get(), auth)

35+ exec(url, auth) { get() }

36

37 suspend fun postJson(url: String, body: String, auth: String?): Result<HttpResp> =

38− exec(Request.Builder().url(url).post(body.toRequestBody(jsonMedia)), auth)

38+ exec(url, auth) { post(body.toRequestBody(jsonMedia)) }

39

40 suspend fun delete(url: String, auth: String?): Result<HttpResp> =

41− exec(Request.Builder().url(url).delete(), auth)

41+ exec(url, auth) { delete() }

42

43+ // Build and run the request off the main thread. Building it — parsing [url]

44+ // in particular — can throw (e.g. a URL with no scheme), so that happens

45+ // inside [runCatching] too: a bad URL then surfaces as [Result.failure]

46+ // instead of crashing the caller's coroutine.

47+ private suspend fun exec(

48+ url: String,

49+ auth: String?,

50+ method: Request.Builder.() -> Request.Builder,

43− private suspend fun exec(builder: Request.Builder, auth: String?): Result<HttpResp> = withContext(Dispatchers.IO) {

51+ ): Result<HttpResp> = withContext(Dispatchers.IO) {

52+ runCatching {

53+ val builder = Request.Builder().url(url).method()

44− if (auth != null) builder.header("Authorization", auth)

54+ if (auth != null) builder.header("Authorization", auth)

45− val request = builder.build()

46− runCatching { await(okhttp.newCall(request)) }

55+ await(okhttp.newCall(builder.build()))

56+ }

47 }

57 }

48

58

49 private suspend fun await(call: Call): HttpResp = suspendCancellableCoroutine { cont ->

59 private suspend fun await(call: Call): HttpResp = suspendCancellableCoroutine { cont ->

app/src/main/java/soy/iko/opencode/core/OpencodeCore.kt+10−1

@@ -444,7 +444,16 @@ private fun messageFromWire(env: Protocol.WireMessageEnvelope): MsgState {

444private fun pickText(text: String, delta: String?, prior: String?): String =

445 if (text.isNotEmpty()) text else (prior.orEmpty() + (delta ?: ""))

446

447−private fun normalizeUrl(url: String): String = url.trim().trimEnd('/')

447+private fun normalizeUrl(url: String): String {

448+ val trimmed = url.trim().trimEnd('/')

449+ // Default to http:// when the user didn't type a scheme (e.g. "192.168.1.10:4096").

450+ // Without a scheme OkHttp's URL parser throws and the connect attempt crashes.

451+ return when {

452+ trimmed.isEmpty() -> ""

453+ "://" in trimmed -> trimmed

454+ else -> "http://$trimmed"

455+ }

456+}

448

457

449private fun displayTitle(title: String): String = title.trim().ifEmpty { "Untitled" }

458private fun displayTitle(title: String): String = title.trim().ifEmpty { "Untitled" }

450

459

app/src/test/java/soy/iko/opencode/core/ConnectUrlTest.ktnew file+46

@@ -0,0 +1,46 @@

1+package soy.iko.opencode.core

2+

3+import kotlinx.coroutines.CoroutineScope

4+import kotlinx.coroutines.Dispatchers

5+import kotlinx.coroutines.test.runTest

6+import org.junit.Assert.assertEquals

7+import org.junit.Assert.assertTrue

8+import org.junit.Test

9+

10+/**

11+ * Regression coverage for the "app crashes after submitting a URL" bug: a

12+ * scheme-less server URL (e.g. "192.168.1.10:4096") used to reach OkHttp's URL

13+ * parser and throw, crashing the connect coroutine. It must now (a) be

14+ * normalized to an http:// URL by the core, and (b) never crash the HTTP client

15+ * even if something un-parseable slips through.

16+ */

17+class ConnectUrlTest {

18+ private fun core() = OpencodeCore(CoroutineScope(Dispatchers.Unconfined))

19+

20+ @Test

21+ fun `scheme-less url defaults to http`() {

22+ val core = core()

23+ core.dispatch(Event.ServerUrlChanged("192.168.1.10:4096"))

24+ assertEquals("http://192.168.1.10:4096", core.view.value.serverUrl)

25+ }

26+

27+ @Test

28+ fun `url with scheme is preserved and trimmed`() {

29+ val core = core()

30+ core.dispatch(Event.ServerUrlChanged(" https://host:4096/ "))

31+ assertEquals("https://host:4096", core.view.value.serverUrl)

32+ }

33+

34+ @Test

35+ fun `blank url normalizes to empty`() {

36+ val core = core()

37+ core.dispatch(Event.ServerUrlChanged(" "))

38+ assertEquals("", core.view.value.serverUrl)

39+ }

40+

41+ @Test

42+ fun `http client returns failure rather than throwing on an unparseable url`() = runTest {

43+ val result = HttpClient().get("not a valid url", auth = null)

44+ assertTrue("expected a Result.failure, got $result", result.isFailure)

45+ }

46+}

shared/src/app.rs+29−2

@@ -740,9 +740,18 @@ fn set_last_pending(model: &mut Model, status: MessageStatus) {

740 }

741}

742

743−/// Trim whitespace and any trailing `/` so paths concatenate cleanly.

743+/// Trim whitespace and any trailing `/` so paths concatenate cleanly, and

744+/// default to an `http://` scheme when the user didn't type one (e.g.

745+/// "192.168.1.10:4096") — otherwise the shell's URL parser rejects it.

744fn normalize_url(url: &str) -> String {

746fn normalize_url(url: &str) -> String {

745− url.trim().trim_end_matches('/').to_string()

747+ let trimmed = url.trim().trim_end_matches('/');

748+ if trimmed.is_empty() {

749+ String::new()

750+ } else if trimmed.contains("://") {

751+ trimmed.to_string()

752+ } else {

753+ format!("http://{trimmed}")

754+ }

746}

755}

747

756

748fn display_title(title: &str) -> String {

757fn display_title(title: &str) -> String {

@@ -801,6 +810,24 @@ mod tests {

801 assert_eq!(app.view(&model).server_url, "http://host:4096");

810 assert_eq!(app.view(&model).server_url, "http://host:4096");

802 }

811 }

803

812

813+ #[test]

814+ fn server_url_without_scheme_defaults_to_http() {

815+ let app = app();

816+ let mut model = Model::default();

817+ // A scheme-less host (what a user often types) must gain http://, or the

818+ // shell's URL parser throws and the connect attempt crashes.

819+ let _ = app.update(Event::ServerUrlChanged("192.168.1.10:4096".into()), &mut model);

820+ assert_eq!(app.view(&model).server_url, "http://192.168.1.10:4096");

821+ }

822+

823+ #[test]

824+ fn server_url_keeps_https_scheme() {

825+ let app = app();

826+ let mut model = Model::default();

827+ let _ = app.update(Event::ServerUrlChanged("https://host:4096".into()), &mut model);

828+ assert_eq!(app.view(&model).server_url, "https://host:4096");

829+ }

830+

804 #[test]

831 #[test]

805 fn connect_requires_a_url() {

832 fn connect_requires_a_url() {

806 let app = app();

833 let app = app();