From 5daac7aeb7d04d3322947f9d0acbdba8c9213e8d Mon Sep 17 00:00:00 2001 From: "tis.wu" Date: Sun, 19 Jul 2026 19:15:25 +0800 Subject: [PATCH] fix(v0.24): link uploads with PATCH, and stop hiding link failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Attaching an upload to a memo on a v0.24 server used POST, but the proto binds SetMemoResources to `patch: "/api/v1/{name=memos/*}/resources"`. Real v0.24.0 and v0.24.3 both answer 501 Method Not Allowed to the POST. The upload itself had already succeeded, so the file sat on the server as an unused resource, attached to nothing and visible on no client — the memo posted fine, just without its images. v0.25+ was never affected; it uses the attachments endpoint, which the app already patches. This was invisible for as long as it was because images used to be embedded in the markdown content as well, which rendered them even when the link call failed. Removing that embed in c463cd2 (issue #5) took the cover away and left the plain bug — on v0.24 only. The link failure was also caught and logged and nothing else, so a save that lost its attachments looked exactly like one that worked. It now surfaces the error and keeps the editor open. The memo itself is already created at that point, so memoId is captured from the create: retrying updates that memo rather than posting a duplicate. Verified end-to-end from the app against memos 0.24.3 and 0.25.3: memo posted with an image, attachment linked server-side, no unused resources left behind. The Retrofit-level test pins both verbs — it fails on the old POST. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01RZ9mXqLfk6PKwo8V9pVDEC --- core/network/build.gradle.kts | 1 + .../whtis/memosly/core/network/api/MemoApi.kt | 5 +- .../api/MemoApiAttachmentEndpointTest.kt | 90 +++++++++++++++++++ core/ui/src/main/res/values-zh/strings.xml | 1 + core/ui/src/main/res/values/strings.xml | 1 + .../feature/memo/MemoEditorViewModel.kt | 24 ++++- gradle/libs.versions.toml | 1 + 7 files changed, 118 insertions(+), 5 deletions(-) create mode 100644 core/network/src/test/java/com/whtis/memosly/core/network/api/MemoApiAttachmentEndpointTest.kt diff --git a/core/network/build.gradle.kts b/core/network/build.gradle.kts index 51a148a..50edc21 100644 --- a/core/network/build.gradle.kts +++ b/core/network/build.gradle.kts @@ -23,4 +23,5 @@ dependencies { implementation(libs.kotlinx.coroutines.android) testImplementation(libs.junit) + testImplementation(libs.okhttp.mockwebserver) } diff --git a/core/network/src/main/java/com/whtis/memosly/core/network/api/MemoApi.kt b/core/network/src/main/java/com/whtis/memosly/core/network/api/MemoApi.kt index 20d1929..bd5e0d6 100644 --- a/core/network/src/main/java/com/whtis/memosly/core/network/api/MemoApi.kt +++ b/core/network/src/main/java/com/whtis/memosly/core/network/api/MemoApi.kt @@ -79,8 +79,9 @@ interface MemoApi { @Body request: SetMemoRelationsRequest, ) - // v0.24: link resources to memo - @POST("api/v1/memos/{id}/resources") + // v0.24: link resources to memo. PATCH, not POST — the proto binds this to + // `patch: "/api/v1/{name=memos/*}/resources"`, and POST answers 501. + @PATCH("api/v1/memos/{id}/resources") suspend fun setMemoResources( @Path("id") id: String, @Body request: SetMemoResourcesRequest, diff --git a/core/network/src/test/java/com/whtis/memosly/core/network/api/MemoApiAttachmentEndpointTest.kt b/core/network/src/test/java/com/whtis/memosly/core/network/api/MemoApiAttachmentEndpointTest.kt new file mode 100644 index 0000000..d8b4b82 --- /dev/null +++ b/core/network/src/test/java/com/whtis/memosly/core/network/api/MemoApiAttachmentEndpointTest.kt @@ -0,0 +1,90 @@ +package com.whtis.memosly.core.network.api + +import com.squareup.moshi.Moshi +import com.squareup.moshi.kotlin.reflect.KotlinJsonAdapterFactory +import com.whtis.memosly.core.network.dto.ResourceRef +import com.whtis.memosly.core.network.dto.SetMemoAttachmentsRequest +import com.whtis.memosly.core.network.dto.SetMemoResourcesRequest +import kotlinx.coroutines.runBlocking +import okhttp3.mockwebserver.MockResponse +import okhttp3.mockwebserver.MockWebServer +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Before +import org.junit.Test +import retrofit2.Retrofit +import retrofit2.converter.moshi.MoshiConverterFactory + +/** + * Pins the HTTP method and path used to link uploaded files to a memo. + * + * Both calls are bound to `patch:` in the Memos proto. Sending POST to the v0.24 + * endpoint answers 501 and the upload stays orphaned — server-side it shows up as an + * unused resource, and the memo renders without its images on every client. That + * shipped, because a wrong verb fails quietly and looks exactly like a save that worked. + */ +class MemoApiAttachmentEndpointTest { + + private lateinit var server: MockWebServer + private lateinit var api: MemoApi + + @Before + fun setUp() { + server = MockWebServer() + server.start() + api = Retrofit.Builder() + .baseUrl(server.url("/")) + .addConverterFactory( + MoshiConverterFactory.create( + Moshi.Builder().add(KotlinJsonAdapterFactory()).build() + ) + ) + .build() + .create(MemoApi::class.java) + } + + @After + fun tearDown() { + server.shutdown() + } + + @Test + fun `v024 setMemoResources patches the resources endpoint`() { + server.enqueue(MockResponse().setResponseCode(200).setBody("{}")) + + runBlocking { + api.setMemoResources( + "4fLRfWCwya4YA2CvnBmWZY", + SetMemoResourcesRequest(listOf(ResourceRef("resources/R6dURNySEeeTUbaCz43Ft2"))), + ) + } + + val request = server.takeRequest() + assertEquals("PATCH", request.method) + assertEquals("/api/v1/memos/4fLRfWCwya4YA2CvnBmWZY/resources", request.path) + assertEquals( + """{"resources":[{"name":"resources/R6dURNySEeeTUbaCz43Ft2"}]}""", + request.body.readUtf8(), + ) + } + + @Test + fun `v025 and up setMemoAttachments patches the attachments endpoint`() { + server.enqueue(MockResponse().setResponseCode(200).setBody("{}")) + + runBlocking { + api.setMemoAttachments( + "bsveqyE5j3H6YXvWBXuzBP", + SetMemoAttachmentsRequest(listOf(ResourceRef("attachments/Bc8eUUhk7jCGLgSkKrArYW"))), + ) + } + + val request = server.takeRequest() + assertEquals("PATCH", request.method) + assertEquals("/api/v1/memos/bsveqyE5j3H6YXvWBXuzBP/attachments", request.path) + assertEquals( + """{"attachments":[{"name":"attachments/Bc8eUUhk7jCGLgSkKrArYW"}]}""", + request.body.readUtf8(), + ) + } +} diff --git a/core/ui/src/main/res/values-zh/strings.xml b/core/ui/src/main/res/values-zh/strings.xml index 33ff921..fcf1838 100644 --- a/core/ui/src/main/res/values-zh/strings.xml +++ b/core/ui/src/main/res/values-zh/strings.xml @@ -147,6 +147,7 @@ 已跳过 %1$s — 超过 %2$d MB 无法读取 %1$s 仅添加了前 %1$d 个文件 + 备忘已保存，但附件关联失败。点击保存可重试。 下载 diff --git a/core/ui/src/main/res/values/strings.xml b/core/ui/src/main/res/values/strings.xml index d4275d8..6565d99 100644 --- a/core/ui/src/main/res/values/strings.xml +++ b/core/ui/src/main/res/values/strings.xml @@ -148,6 +148,7 @@ Skipped %1$s — larger than %2$d MB Couldn\'t read %1$s Only the first %1$d files were added + The memo was saved, but its attachments could not be linked. Tap save to retry. Download diff --git a/feature/memo/src/main/java/com/whtis/memosly/feature/memo/MemoEditorViewModel.kt b/feature/memo/src/main/java/com/whtis/memosly/feature/memo/MemoEditorViewModel.kt index d1486e4..1db0822 100644 --- a/feature/memo/src/main/java/com/whtis/memosly/feature/memo/MemoEditorViewModel.kt +++ b/feature/memo/src/main/java/com/whtis/memosly/feature/memo/MemoEditorViewModel.kt @@ -71,7 +71,11 @@ class MemoEditorViewModel @Inject constructor( val serverUrl: String get() = tokenManager.serverUrl.value ?: "" - private val memoId: String = savedStateHandle["memoId"] ?: "" + /** + * Blank for a new memo. Once a create succeeds this holds the new id, so a retry after a + * failed attachment link updates that memo instead of creating a second one. + */ + private var memoId: String = savedStateHandle["memoId"] ?: "" private val sharedText: String = savedStateHandle["sharedText"] ?: "" private val hasSharedMedia: Boolean = savedStateHandle["sharedMedia"] ?: false @@ -236,10 +240,12 @@ class MemoEditorViewModel @Inject constructor( _uiState.value = _uiState.value.copy(isSaving = true, error = null) try { val visibilityStr = _uiState.value.visibility.name - val memo = if (memoId.isNotBlank()) { + val isUpdate = memoId.isNotBlank() + val memo = if (isUpdate) { memoRepository.updateMemo(memoId, content, visibilityStr) } else { memoRepository.createMemo(content, visibilityStr).also { + memoId = it.name.substringAfterLast("/") analyticsHelper.logEvent("memo_create", mapOf( "has_tags" to if (content.extractTags().isNotEmpty()) "true" else "false", )) @@ -254,7 +260,7 @@ class MemoEditorViewModel @Inject constructor( // them now just orphans them. val pendingResources = _uiState.value.pendingResources val existingResources = _uiState.value.existingResources - val existingChanged = memoId.isNotBlank() && existingResources.size != memo.resources.size + val existingChanged = isUpdate && existingResources.size != memo.resources.size if (pendingResources.isNotEmpty() || existingChanged) { try { val existingNames = existingResources.map { it.name } @@ -262,7 +268,19 @@ class MemoEditorViewModel @Inject constructor( val allNames = (existingNames + newNames).distinct() memoRepository.setMemoResources(memo.name, allNames) } catch (e: Exception) { + // Swallowing this is what let the v0.24 verb bug ship: the memo saved, + // the upload vanished, and nothing said so. The memo exists either way, + // so stay in the editor and let the user retry — memoId now points at + // it, so retrying updates rather than posting a duplicate. android.util.Log.e("MemoEditor", "setMemoResources failed: ${e.message}", e) + _uiState.update { + it.copy( + isSaving = false, + isEditMode = true, + error = context.getString(UiR.string.attachment_link_failed), + ) + } + return@launch } } diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index 46cca08..ce36083 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -122,6 +122,7 @@ kotlinx-coroutines-android = { group = "org.jetbrains.kotlinx", name = "kotlinx- # Testing junit = { group = "junit", name = "junit", version.ref = "junit" } +okhttp-mockwebserver = { group = "com.squareup.okhttp3", name = "mockwebserver", version.ref = "okhttp" } [plugins] android-application = { id = "com.android.application", version.ref = "agp" }