feat(config): store ExApp config & preferences in the server's app/user config#889
feat(config): store ExApp config & preferences in the server's app/user config#889oleksandr-nc wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughThis PR completes the migration of AppAPI's configuration and preference storage from custom appconfig_ex/preferences_ex tables with Entity-based mappers to Nextcloud's standard IAppConfig and IUserConfig interfaces. Changes include refactoring ExAppConfig and ExAppPreference into plain typed DTOs, rewriting services to use server-backed config with transparent encryption and lazy writes, adding backfill and gated-drop migrations, updating controller messages and route comments, and adding integration tests for lazy/eager cache and sensitive-flag behaviors. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1a16ec45-d77e-4401-bbc7-f43eef67dab4
📒 Files selected for processing (16)
appinfo/info.xmlappinfo/routes.phplib/Controller/AppConfigController.phplib/Controller/PreferencesController.phplib/Db/ExAppConfig.phplib/Db/ExAppConfigMapper.phplib/Db/ExAppPreference.phplib/Db/ExAppPreferenceMapper.phplib/Migration/Version035000Date20260529120000.phplib/Migration/Version035000Date20260529130000.phplib/Service/ExAppConfigService.phplib/Service/ExAppPreferenceService.phptests/php/Controller/AppConfigControllerTest.phptests/php/Controller/PreferencesControllerTest.phptests/php/Migration/Version034000Date20260428144801Test.phptests/php/Service/StorageLazyLoadingTest.php
💤 Files with no reviewable changes (3)
- lib/Db/ExAppConfigMapper.php
- lib/Db/ExAppPreferenceMapper.php
- tests/php/Migration/Version034000Date20260428144801Test.php
…g/IUserConfig storage Signed-off-by: Oleksander Piskun <oleksandr2088@icloud.com>
oleksandr-nc
force-pushed
the
feat/migrate-exapp-config-to-server-tables
branch
from
b11d908 to
3067622
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/php/Service/StorageLazyLoadingTest.php (1)
94-105: ⚡ Quick winAdd cold-cache lazy-read coverage for preferences too.
The config side has parity tests for “not in eager cache”, “readable after lazy load”, and “sensitive round-trip from cold cache”, but the preference side currently only asserts
isLazy()plus eager external visibility. That leaves the rewrittenIUserConfiglazy-read path largely untested, so a regression there would still pass this suite. Please add at least one cold-cache lazy preference read and one sensitive lazy preference round-trip test here.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7170d5be-bfd1-411d-8229-7bd84f508687
📒 Files selected for processing (16)
appinfo/info.xmlappinfo/routes.phplib/Controller/AppConfigController.phplib/Controller/PreferencesController.phplib/Db/ExAppConfig.phplib/Db/ExAppConfigMapper.phplib/Db/ExAppPreference.phplib/Db/ExAppPreferenceMapper.phplib/Migration/Version035000Date20260529120000.phplib/Migration/Version035000Date20260529130000.phplib/Service/ExAppConfigService.phplib/Service/ExAppPreferenceService.phptests/php/Controller/AppConfigControllerTest.phptests/php/Controller/PreferencesControllerTest.phptests/php/Migration/Version034000Date20260428144801Test.phptests/php/Service/StorageLazyLoadingTest.php
💤 Files with no reviewable changes (3)
- tests/php/Migration/Version034000Date20260428144801Test.php
- lib/Db/ExAppConfigMapper.php
- lib/Db/ExAppPreferenceMapper.php
✅ Files skipped from review due to trivial changes (2)
- appinfo/info.xml
- appinfo/routes.php
🚧 Files skipped from review as they are similar to previous changes (6)
- lib/Controller/PreferencesController.php
- lib/Controller/AppConfigController.php
- lib/Db/ExAppPreference.php
- lib/Service/ExAppPreferenceService.php
- lib/Service/ExAppConfigService.php
- lib/Db/ExAppConfig.php
…on gate flag once no legacy table remains Signed-off-by: Oleksander Piskun <oleksandr2088@icloud.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/Migration/Version035000Date20260529120000.php (1)
121-159:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBound
IUserConfigper-user cache growth during preferences backfill to preventocc upgradeOOM
migratePreferences()now writes viaIUserConfig::setValueString(... lazy: true, ...)whileIUserConfigmaintains an internal in-memory per-user cache for the lifetime of the PHP process/command; with large backfills this can accumulate cache entries for every touched user (no automatic size/LRU eviction is provided), reintroducing the OOM risk even though legacy reads are batched. Using the provided invalidation hookIUserConfig::clearCache(string $userId, bool $reload=false): voidafter each batch bounds memory.🔧 Proposed fix
$lastId = 0; while (($rows = $this->fetchBatch('preferences_ex', ['userid', 'appid', 'configkey', 'configvalue', 'sensitive'], $lastId)) !== []) { + $touchedUsers = []; foreach ($rows as $row) { $lastId = (int)$row['id']; $userId = (string)$row['userid']; + $touchedUsers[$userId] = true; $appId = (string)$row['appid']; $configKey = (string)$row['configkey']; $sensitive = (int)($row['sensitive'] ?? 0) === 1; @@ } catch (Throwable $e) { $output->warning(sprintf('Preferences migration: failed to migrate user %s app %s key %s (row id=%d): %s — skipping', $userId, $appId, $configKey, $lastId, $e->getMessage())); $failed++; } } + // Drop per-user caches so the backfill cannot OOM on instances with many users. + foreach (array_keys($touchedUsers) as $uid) { + $this->userConfig->clearCache($uid, false); + } }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 169ffa10-7007-4619-9088-17f02605bffd
📒 Files selected for processing (2)
lib/Migration/Version035000Date20260529120000.phplib/Migration/Version035000Date20260529130000.php
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/Migration/Version035000Date20260529130000.php
1 participant
Moves ExApp config and per-user preferences from AppAPI's custom
appconfig_ex/preferences_extables to the server'sIAppConfig(oc_appconfig) andIUserConfig(oc_preferences). The server gained typed/lazy/sensitive value support in NC 29 and 32, and AppAPI now targets 35, so the custom tables are no longer needed.The OCS endpoints used by ExApps are unchanged, so nc_py_api and existing ExApps keep working as-is. One migration backfills existing rows into the server tables (decrypting and re-encrypting sensitive values through the server); a second drops the old tables once the backfill reports no failures.