Skip to content

Commit b891d94

Browse files
jkmasseljkmassel
committed
Stop full-row site writes from clobbering wpApiRestUrl
On Atomic / Jetpack-WPCom-REST sites a recovered wpApiRestUrl was wiped to NULL on every app foreground: any full-row insertOrUpdateSite UPDATE built from a partial in-memory SiteModel (FETCH_SITE/FETCH_SITES, RN, cookie-nonce) wrote every column, clobbering the healed value. Exclude WP_API_REST_URL from the generic UpdateAllExceptId mapper so updateWpApiRestUrl is the sole writer on an existing row, and route every legitimate writer through the targeted helpers. Fixes #22905.
1 parent ff597e0 commit b891d94

7 files changed

Lines changed: 216 additions & 9 deletions

File tree

‎libs/fluxc/src/main/java/org/wordpress/android/fluxc/network/rest/wpapi/CookieNonceAuthenticator.kt‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -56,8 +56,11 @@ class CookieNonceAuthenticator @Inject constructor(
5656
): T {
5757
val usingSavedRestUrl = site.wpApiRestUrl != null
5858
if (!usingSavedRestUrl) {
59-
site.wpApiRestUrl = discoverApiEndpoint(site.url)
60-
(siteSqlUtils::insertOrUpdateSite)(site)
59+
val discoveredUrl = discoverApiEndpoint(site.url)
60+
site.wpApiRestUrl = discoveredUrl
61+
// WP_API_REST_URL is excluded from full-row writes (see SiteSqlUtils), so persist the
62+
// freshly discovered value through its dedicated writer.
63+
siteSqlUtils.updateWpApiRestUrl(site.id, discoveredUrl)
6164
}
6265

6366
val response = makeAuthenticatedWPAPIRequest(

‎libs/fluxc/src/main/java/org/wordpress/android/fluxc/persistence/SiteSqlUtils.kt‎

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -221,8 +221,13 @@ class SiteSqlUtils
221221
AppLog.d(DB, "Updating site: " + finalSiteModel.url)
222222
val oldId = siteResult[0].id
223223
try {
224+
// WP_API_REST_URL is healed/discovered locally (see updateWpApiRestUrl) and must not be
225+
// overwritten by stale full-row writes, so it is excluded from the generic update mapper.
224226
WellSql.update(SiteModel::class.java).whereId(oldId)
225-
.put(finalSiteModel, UpdateAllExceptId(SiteModel::class.java)).execute()
227+
.put(
228+
finalSiteModel,
229+
UpdateAllExceptId(SiteModel::class.java, SiteModelTable.WP_API_REST_URL)
230+
).execute()
226231
} catch (e: SQLiteConstraintException) {
227232
AppLog.e(
228233
DB,
@@ -260,6 +265,11 @@ class SiteSqlUtils
260265
}).execute()
261266
}
262267

268+
/**
269+
* Targeted writer for [SiteModel.wpApiRestUrl]. This is the sole writer of WP_API_REST_URL on an
270+
* existing row: the generic full-row update path ([insertOrUpdateSite]) excludes the column so that
271+
* stale in-memory sites can't clobber a value that was healed/discovered out of band.
272+
*/
263273
fun updateWpApiRestUrl(localId: Int, wpApiRestUrl: String): Int {
264274
return WellSql.update(SiteModel::class.java)
265275
.whereId(localId)
@@ -270,6 +280,30 @@ class SiteSqlUtils
270280
}).execute()
271281
}
272282

283+
/**
284+
* Clears [SiteModel.wpApiRestUrl] for the given local id. Use this instead of a full-row update when an
285+
* explicit action (e.g. removing an application password) needs to drop the stored REST URL, since the
286+
* generic update path no longer touches the column.
287+
*/
288+
fun clearWpApiRestUrl(localId: Int): Int = updateWpApiRestUrl(localId, "")
289+
290+
/**
291+
* Updates [SiteModel.wpApiRestUrl] for an application-password (ORIGIN_WPAPI) site identified by its URL.
292+
* Such sites are fetched as fresh models with no local id and no remote site id
293+
* (see SiteWPAPIRestClient.fetchWPAPISite), so the local-id-keyed [updateWpApiRestUrl] can't target them.
294+
* Scoped to ORIGIN_WPAPI so it can't touch a WP.com/Jetpack row that happens to share the same URL.
295+
*/
296+
fun updateWpApiRestUrlForWPAPISite(siteUrl: String, wpApiRestUrl: String): Int {
297+
val site = WellSql.select(SiteModel::class.java)
298+
.where().beginGroup()
299+
.equals(SiteModelTable.URL, siteUrl)
300+
.equals(SiteModelTable.ORIGIN, SiteModel.ORIGIN_WPAPI)
301+
.endGroup().endWhere()
302+
.asModel
303+
.firstOrNull() ?: return 0
304+
return updateWpApiRestUrl(site.id, wpApiRestUrl)
305+
}
306+
273307
val wPComSites: SelectQuery<SiteModel>
274308
get() = WellSql.select(SiteModel::class.java)
275309
.where().beginGroup()

‎libs/fluxc/src/main/java/org/wordpress/android/fluxc/persistence/UpdateAllExceptId.java‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,15 +8,29 @@
88

99
class UpdateAllExceptId<T> implements InsertMapper<T> {
1010
private final SQLiteMapper<T> mMapper;
11+
private final String[] mAdditionalColumnsToSkip;
1112

1213
UpdateAllExceptId(Class<T> clazz) {
14+
this(clazz, new String[0]);
15+
}
16+
17+
/**
18+
* @param additionalColumnsToSkip columns (beyond the primary key) that should be left untouched by the
19+
* resulting UPDATE. Use this when a column has a dedicated writer and must
20+
* not be overwritten by full-row updates built from partial in-memory models.
21+
*/
22+
UpdateAllExceptId(Class<T> clazz, String... additionalColumnsToSkip) {
1323
mMapper = WellSql.mapperFor(clazz);
24+
mAdditionalColumnsToSkip = additionalColumnsToSkip;
1425
}
1526

1627
@Override
1728
public ContentValues toCv(T item) {
1829
ContentValues cv = mMapper.toCv(item);
1930
cv.remove("_id");
31+
for (String column : mAdditionalColumnsToSkip) {
32+
cv.remove(column);
33+
}
2034
return cv;
2135
}
2236
}

‎libs/fluxc/src/main/java/org/wordpress/android/fluxc/store/ReactNativeStore.kt‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -175,10 +175,13 @@ class ReactNativeStore @VisibleForTesting constructor(
175175

176176
val usingSavedRestUrl = wpApiRestUrl != null
177177
if (!usingSavedRestUrl) {
178-
wpApiRestUrl = discoveryWPAPIRestClient.discoverWPAPIBaseURL(site.url) // discover rest api endpoint
178+
val discoveredUrl = discoveryWPAPIRestClient.discoverWPAPIBaseURL(site.url) // discover rest api endpoint
179179
?: slashJoin(site.url, "wp-json/") // fallback to ".../wp-json/" default if discovery fails
180-
site.wpApiRestUrl = wpApiRestUrl
181-
persistSiteSafely(site)
180+
wpApiRestUrl = discoveredUrl
181+
site.wpApiRestUrl = discoveredUrl
182+
// WP_API_REST_URL is excluded from full-row writes (see SiteSqlUtils), so persist the
183+
// freshly discovered value through its dedicated writer.
184+
siteSqlUtils.updateWpApiRestUrl(site.id, discoveredUrl)
182185
}
183186
val fullRestUrl = slashJoin(wpApiRestUrl, path)
184187

‎libs/fluxc/src/main/java/org/wordpress/android/fluxc/store/SiteStore.kt‎

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1615,7 +1615,13 @@ open class SiteStore @Inject constructor(
16151615
if (!siteModel.isError) {
16161616
siteModel.wpApiRestUrl = payload.apiRootUrl
16171617
}
1618-
updateSite(siteModel)
1618+
val result = updateSite(siteModel)
1619+
// updateSite's full-row write skips WP_API_REST_URL, and this fresh site has no local id
1620+
// (it's matched by URL), so persist the discovered URL via the URL-keyed writer. See SiteSqlUtils.
1621+
if (!siteModel.isError && payload.apiRootUrl.isNotEmpty()) {
1622+
siteSqlUtils.updateWpApiRestUrlForWPAPISite(siteModel.url, payload.apiRootUrl)
1623+
}
1624+
result
16191625
} catch (e: Exception) {
16201626
val errorMsg = e.message ?: e.javaClass.simpleName
16211627
AppLog.e(
@@ -1695,7 +1701,15 @@ open class SiteStore @Inject constructor(
16951701
}
16961702
siteFromDB
16971703
}
1698-
OnSiteChanged(siteSqlUtils.insertOrUpdateSite(siteToStore))
1704+
val rowsAffected = siteSqlUtils.insertOrUpdateSite(siteToStore)
1705+
// The generic update path no longer writes WP_API_REST_URL (see SiteSqlUtils), so when the
1706+
// application-password flow discovered a REST URL for an existing site, persist it explicitly.
1707+
if (siteFromDB != null) {
1708+
siteModel.wpApiRestUrl?.takeIf { it.isNotEmpty() }?.let {
1709+
siteSqlUtils.updateWpApiRestUrl(siteFromDB.id, it)
1710+
}
1711+
}
1712+
OnSiteChanged(rowsAffected)
16991713
} catch (e: DuplicateSiteException) {
17001714
OnSiteChanged(SiteError(DUPLICATE_SITE))
17011715
} catch (e: Exception) {
@@ -1726,7 +1740,11 @@ open class SiteStore @Inject constructor(
17261740
apiRestPasswordIV = ""
17271741
wpApiRestUrl = ""
17281742
}
1729-
OnSiteChanged(siteSqlUtils.insertOrUpdateSite(siteFromDB))
1743+
val rowsAffected = siteSqlUtils.insertOrUpdateSite(siteFromDB)
1744+
// The generic update path no longer writes WP_API_REST_URL (see SiteSqlUtils), so clear the
1745+
// stored REST URL explicitly now that the application password backing it is gone.
1746+
siteSqlUtils.clearWpApiRestUrl(siteFromDB.id)
1747+
OnSiteChanged(rowsAffected)
17301748
}
17311749
} catch (e: DuplicateSiteException) {
17321750
OnSiteChanged(SiteError(DUPLICATE_SITE))

‎libs/fluxc/src/test/java/org/wordpress/android/fluxc/persistence/SiteSqlUtilsTest.kt‎

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,4 +49,95 @@ class SiteSqlUtilsTest {
4949

5050
assertThat(rowsUpdated).isEqualTo(0)
5151
}
52+
53+
@Test
54+
fun `insertOrUpdateSite update does not clobber wpApiRestUrl from a stale model`() {
55+
val healed = "https://example.test/wp-json/"
56+
WellSql.insert(SiteModel().apply {
57+
siteId = 42
58+
url = "https://example.test"
59+
name = "Example"
60+
wpApiRestUrl = healed
61+
}).execute()
62+
// WellSql auto-assigns the primary key on insert, so read back the id it actually allocated.
63+
val localId = siteSqlUtils.getSites().single().id
64+
65+
// A later writer (e.g. a FETCH_SITE round-trip) carries a stale, null wpApiRestUrl but a fresh name.
66+
val stale = SiteModel().apply {
67+
id = localId
68+
siteId = 42
69+
url = "https://example.test"
70+
name = "Updated name"
71+
wpApiRestUrl = null
72+
}
73+
val rows = siteSqlUtils.insertOrUpdateSite(stale)
74+
75+
assertThat(rows).isEqualTo(1)
76+
val stored = siteSqlUtils.getSites().single()
77+
assertThat(stored.wpApiRestUrl).isEqualTo(healed) // preserved
78+
assertThat(stored.name).isEqualTo("Updated name") // other columns still update
79+
}
80+
81+
@Test
82+
fun `insertOrUpdateSite insert still persists wpApiRestUrl for a new site`() {
83+
val rows = siteSqlUtils.insertOrUpdateSite(SiteModel().apply {
84+
siteId = 99
85+
url = "https://newsite.test"
86+
name = "New"
87+
wpApiRestUrl = "https://newsite.test/wp-json/"
88+
})
89+
90+
assertThat(rows).isEqualTo(1)
91+
assertThat(siteSqlUtils.getSites().single().wpApiRestUrl)
92+
.isEqualTo("https://newsite.test/wp-json/")
93+
}
94+
95+
@Test
96+
fun `clearWpApiRestUrl empties the stored url`() {
97+
WellSql.insert(SiteModel().apply {
98+
url = "https://example.test"
99+
wpApiRestUrl = "https://example.test/wp-json/"
100+
}).execute()
101+
val localId = siteSqlUtils.getSites().single().id
102+
103+
val rows = siteSqlUtils.clearWpApiRestUrl(localId)
104+
105+
assertThat(rows).isEqualTo(1)
106+
assertThat(siteSqlUtils.getSites().single().wpApiRestUrl).isEmpty()
107+
}
108+
109+
@Test
110+
fun `updateWpApiRestUrlForWPAPISite updates the matching WPAPI site by url`() {
111+
WellSql.insert(SiteModel().apply {
112+
url = "https://selfhosted.test"
113+
origin = SiteModel.ORIGIN_WPAPI
114+
wpApiRestUrl = null
115+
}).execute()
116+
117+
val rows = siteSqlUtils.updateWpApiRestUrlForWPAPISite(
118+
siteUrl = "https://selfhosted.test",
119+
wpApiRestUrl = "https://selfhosted.test/wp-json/"
120+
)
121+
122+
assertThat(rows).isEqualTo(1)
123+
assertThat(siteSqlUtils.getSites().single().wpApiRestUrl)
124+
.isEqualTo("https://selfhosted.test/wp-json/")
125+
}
126+
127+
@Test
128+
fun `updateWpApiRestUrlForWPAPISite leaves a non-WPAPI site with the same url untouched`() {
129+
WellSql.insert(SiteModel().apply {
130+
url = "https://shared.test"
131+
origin = SiteModel.ORIGIN_WPCOM_REST
132+
wpApiRestUrl = null
133+
}).execute()
134+
135+
val rows = siteSqlUtils.updateWpApiRestUrlForWPAPISite(
136+
siteUrl = "https://shared.test",
137+
wpApiRestUrl = "https://shared.test/wp-json/"
138+
)
139+
140+
assertThat(rows).isEqualTo(0)
141+
assertThat(siteSqlUtils.getSites().single().wpApiRestUrl).isNull()
142+
}
52143
}

‎libs/fluxc/src/test/java/org/wordpress/android/fluxc/store/SiteStoreTest.kt‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import org.mockito.kotlin.verify
1616
import org.mockito.kotlin.verifyNoInteractions
1717
import org.mockito.kotlin.whenever
1818
import org.wordpress.android.fluxc.Dispatcher
19+
import org.wordpress.android.fluxc.generated.SiteActionBuilder
1920
import org.wordpress.android.fluxc.model.PostFormatModel
2021
import org.wordpress.android.fluxc.model.SiteModel
2122
import org.wordpress.android.fluxc.model.SitesModel
@@ -575,8 +576,51 @@ class SiteStoreTest {
575576
.isEqualTo("appPass")
576577
assertThat(fetchedSite.wpApiRestUrl)
577578
.isEqualTo("https://example.com/wp-json/")
579+
// updateSite's full-row write skips WP_API_REST_URL, so the discovered URL must be persisted
580+
// via the URL-keyed targeted writer (the fetched site has no local id to key on).
581+
verify(siteSqlUtils).updateWpApiRestUrlForWPAPISite(
582+
"https://example.com",
583+
"https://example.com/wp-json/"
584+
)
578585
}
579586

587+
@Test
588+
fun `updateApplicationPassword persists discovered wpApiRestUrl via targeted writer`() {
589+
val existing = SiteModel().apply {
590+
id = 3
591+
url = "https://selfhosted.test"
592+
}
593+
whenever(siteSqlUtils.getSitesWithLocalId(3)).thenReturn(listOf(existing))
594+
whenever(siteSqlUtils.insertOrUpdateSite(any())).thenReturn(1)
595+
val incoming = SiteModel().apply {
596+
id = 3
597+
apiRestUsernamePlain = "user"
598+
apiRestPasswordPlain = "pass"
599+
wpApiRestUrl = "https://selfhosted.test/wp-json/"
600+
}
601+
602+
siteStore.onAction(SiteActionBuilder.newUpdateApplicationPasswordAction(incoming))
603+
604+
verify(siteSqlUtils).updateWpApiRestUrl(3, "https://selfhosted.test/wp-json/")
605+
}
606+
607+
@Test
608+
fun `removeApplicationPassword clears wpApiRestUrl via targeted writer`() {
609+
val existing = SiteModel().apply {
610+
id = 4
611+
url = "https://selfhosted.test"
612+
wpApiRestUrl = "https://selfhosted.test/wp-json/"
613+
}
614+
whenever(siteSqlUtils.getSitesWithLocalId(4)).thenReturn(listOf(existing))
615+
whenever(siteSqlUtils.insertOrUpdateSite(any())).thenReturn(1)
616+
617+
siteStore.onAction(
618+
SiteActionBuilder.newRemoveApplicationPasswordAction(SiteModel().apply { id = 4 })
619+
)
620+
621+
verify(siteSqlUtils).clearWpApiRestUrl(4)
622+
}
623+
580624
@Test
581625
fun `fetchPostFormats returns empty list for WPAPI site without crashing`() =
582626
test {

0 commit comments

Comments
 (0)