Skip to content

Commit 32a22f3

Browse files
oleksandr-ncoleksandr-nc
committed
fix(occ): preserve ExApp deploy options on update (#808)
Signed-off-by: Oleksander Piskun <oleksandr2088@icloud.com>
1 parent 47b612f commit 32a22f3

4 files changed

Lines changed: 380 additions & 2 deletions

File tree

‎.github/workflows/tests-deploy.yml‎

Lines changed: 164 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1563,14 +1563,177 @@ jobs:
15631563
if: always()
15641564
run: tail -v -n +1 *container.json *.log data/nextcloud.log
15651565

1566+
nc-update-preserves-deploy-options:
1567+
runs-on: ubuntu-22.04
1568+
name: Update preserves deploy options (Docker)
1569+
# Regression test for https://github.com/nextcloud/app_api/issues/808
1570+
# When two or more ExApps have rows in `ex_deploy_options`, the update flow
1571+
# used to drop the target app's env vars because it read the unfiltered
1572+
# options table. This job seeds a stray second row, bumps the skeleton's
1573+
# version via a local info.xml, runs `app_api:app:update`, and asserts the
1574+
# user-provided env vars are still set on the new container.
1575+
1576+
services:
1577+
postgres:
1578+
image: ghcr.io/nextcloud/continuous-integration-postgres-14:latest # zizmor: ignore[unpinned-images]
1579+
ports:
1580+
- 4444:5432/tcp
1581+
env:
1582+
POSTGRES_USER: root
1583+
POSTGRES_PASSWORD: rootpassword
1584+
POSTGRES_DB: nextcloud
1585+
options: --health-cmd pg_isready --health-interval 5s --health-timeout 2s --health-retries 5
1586+
1587+
steps:
1588+
- name: Set app env
1589+
run: echo "APP_NAME=${GITHUB_REPOSITORY##*/}" >> $GITHUB_ENV
1590+
1591+
- name: Checkout server
1592+
uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
1593+
with:
1594+
persist-credentials: false
1595+
submodules: true
1596+
repository: nextcloud/server
1597+
ref: master
1598+
1599+
- name: Checkout AppAPI
1600+
uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
1601+
with:
1602+
persist-credentials: false
1603+
path: apps/${{ env.APP_NAME }}
1604+
1605+
- name: Set up php
1606+
uses: shivammathur/setup-php@accd6127cb78bee3e8082180cb391013d204ef9f # v2
1607+
with:
1608+
php-version: '8.3'
1609+
extensions: bz2, ctype, curl, dom, fileinfo, gd, iconv, intl, json, libxml, mbstring, openssl, pcntl, posix, session, simplexml, xmlreader, xmlwriter, zip, zlib, pgsql, pdo_pgsql
1610+
coverage: none
1611+
ini-file: development
1612+
ini-values:
1613+
apc.enabled=on, apc.enable_cli=on, disable_functions=
1614+
env:
1615+
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
1616+
1617+
- name: Check composer file existence
1618+
id: check_composer
1619+
uses: andstor/file-existence-action@558493d6c74bf472d87c84eab196434afc2fa029 # v2
1620+
with:
1621+
files: apps/${{ env.APP_NAME }}/composer.json
1622+
1623+
- name: Set up dependencies
1624+
if: steps.check_composer.outputs.files_exists == 'true'
1625+
working-directory: apps/${{ env.APP_NAME }}
1626+
run: composer i
1627+
1628+
- name: Set up Nextcloud
1629+
env:
1630+
DB_PORT: 4444
1631+
run: |
1632+
mkdir data
1633+
./occ maintenance:install --verbose --database=pgsql --database-name=nextcloud --database-host=127.0.0.1 \
1634+
--database-port=$DB_PORT --database-user=root --database-pass=rootpassword \
1635+
--admin-user admin --admin-pass admin
1636+
./occ config:system:set loglevel --value=0 --type=integer
1637+
./occ config:system:set debug --value=true --type=boolean
1638+
./occ app:enable --force ${{ env.APP_NAME }}
1639+
1640+
- name: Register daemon & Skeleton v1 with user env vars
1641+
run: |
1642+
PHP_CLI_SERVER_WORKERS=2 php -S 127.0.0.1:8080 &
1643+
./occ app_api:daemon:register docker_local_sock Docker docker-install http /var/run/docker.sock http://127.0.0.1:8080/index.php
1644+
./occ app_api:app:register app-skeleton-python docker_local_sock \
1645+
--info-xml https://raw.githubusercontent.com/nextcloud/app-skeleton-python/main/appinfo/info.xml \
1646+
--env='TEST_ENV_2=user_provided_value'
1647+
1648+
- name: Verify env vars on the freshly registered container
1649+
run: |
1650+
docker inspect --format '{{ json .Config.Env }}' nc_app_app-skeleton-python | grep -q 'TEST_ENV_1=0' \
1651+
|| { echo "TEST_ENV_1 default missing after register"; exit 1; }
1652+
docker inspect --format '{{ json .Config.Env }}' nc_app_app-skeleton-python | grep -q 'TEST_ENV_2=user_provided_value' \
1653+
|| { echo "TEST_ENV_2 user value missing after register"; exit 1; }
1654+
1655+
- name: Seed stray ex_deploy_options row for a second app
1656+
# Without a second appid in the table, the Update.php bug from #808 is
1657+
# latent — formatDeployOptions() with no $appId filter still produces
1658+
# the single app's rows by luck. The `zz_` prefix ensures this stray row
1659+
# iterates AFTER `app-skeleton-python`, so the last-wins flattening
1660+
# actually clobbers the skeleton's env_vars entry.
1661+
run: |
1662+
php -r '
1663+
require "lib/base.php";
1664+
OCP\Server::get(OCP\App\IAppManager::class)->loadApp("app_api");
1665+
$svc = OCP\Server::get(OCA\AppAPI\Service\ExAppDeployOptionsService::class);
1666+
$svc->addExAppDeployOptions("zz_fake_second_app", [
1667+
"environment_variables" => ["UNRELATED_VAR" => ["name" => "UNRELATED_VAR", "value" => "x"]],
1668+
"mounts" => [],
1669+
]);
1670+
'
1671+
1672+
- name: Build v2 info.xml with bumped version
1673+
# Update.php:153 short-circuits when versions match; bumping to 999.0.0
1674+
# forces the real update path to run without touching the image.
1675+
run: |
1676+
curl -sS https://raw.githubusercontent.com/nextcloud/app-skeleton-python/main/appinfo/info.xml \
1677+
| sed 's#<version>[^<]*</version>#<version>999.0.0</version>#' > /tmp/info-v2.xml
1678+
grep -q '<version>999.0.0</version>' /tmp/info-v2.xml || { echo "version bump failed"; exit 1; }
1679+
1680+
- name: Update ExApp
1681+
run: |
1682+
./occ app_api:app:update app-skeleton-python --info-xml /tmp/info-v2.xml --wait-finish
1683+
1684+
- name: After update — TEST_ENV_1 default still present
1685+
run: |
1686+
docker inspect --format '{{ json .Config.Env }}' nc_app_app-skeleton-python | grep -q 'TEST_ENV_1=0' \
1687+
|| { echo "#808 regression: TEST_ENV_1 lost on update"; exit 1; }
1688+
1689+
- name: After update — TEST_ENV_2 user value still present
1690+
run: |
1691+
docker inspect --format '{{ json .Config.Env }}' nc_app_app-skeleton-python | grep -q 'TEST_ENV_2=user_provided_value' \
1692+
|| { echo "#808 regression: user-provided TEST_ENV_2 lost on update"; exit 1; }
1693+
1694+
- name: Save container info & logs
1695+
if: always()
1696+
run: |
1697+
docker inspect nc_app_app-skeleton-python | json_pp > container.json
1698+
docker logs nc_app_app-skeleton-python > container.log 2>&1
1699+
1700+
- name: Unregister Skeleton & Daemon
1701+
run: |
1702+
./occ app_api:app:unregister app-skeleton-python
1703+
./occ app_api:daemon:unregister docker_local_sock
1704+
1705+
- name: Upload Container info
1706+
if: always()
1707+
uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1
1708+
with:
1709+
name: nc_update_preserves_deploy_options_container.json
1710+
path: container.json
1711+
if-no-files-found: warn
1712+
1713+
- name: Upload Container logs
1714+
if: always()
1715+
uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1
1716+
with:
1717+
name: nc_update_preserves_deploy_options_container.log
1718+
path: container.log
1719+
if-no-files-found: warn
1720+
1721+
- name: Upload NC logs
1722+
if: always()
1723+
uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1
1724+
with:
1725+
name: nc_update_preserves_deploy_options_nextcloud.log
1726+
path: data/nextcloud.log
1727+
if-no-files-found: warn
1728+
15661729
tests-deploy-success:
15671730
permissions:
15681731
contents: none
15691732
runs-on: ubuntu-22.04
15701733
needs: [nc-host-app-docker, nc-docker-app-docker, nc-docker-dsp-http,
15711734
nc-docker-dsp-https, nc-host-app-docker-redis, nc-host-network-host,
15721735
nc-docker-harp-bridge, nc-docker-harp-bridge-no-tls, nc-host-harp-host,
1573-
nc-host-manual-harp-host]
1736+
nc-host-manual-harp-host, nc-update-preserves-deploy-options]
15741737
name: Tests-Deploy-OK
15751738
steps:
15761739
- run: echo "Tests-Deploy passed successfully"

‎lib/Command/ExApp/Update.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int
9595
private function updateExApp(InputInterface $input, OutputInterface $output, string $appId): int {
9696
$outputConsole = !$input->getOption('silent');
9797
$deployOptions = $this->exAppDeployOptionsService->formatDeployOptions(
98-
$this->exAppDeployOptionsService->getDeployOptions()
98+
$this->exAppDeployOptionsService->getDeployOptions($appId)
9999
);
100100
$appInfo = $this->exAppService->getAppInfo(
101101
$appId, $input->getOption('info-xml'), $input->getOption('json-info'),

‎tests/php/Service/ExAppDeployOptionsServiceTest.php‎

Lines changed: 124 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,124 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
7+
* SPDX-License-Identifier: AGPL-3.0-or-later
8+
*/
9+
10+
namespace OCA\AppAPI\Tests\php\Service;
11+
12+
use OCA\AppAPI\Db\ExAppDeployOptionsMapper;
13+
use OCA\AppAPI\Service\ExAppDeployOptionsService;
14+
use OCP\ICacheFactory;
15+
use PHPUnit\Framework\MockObject\MockObject;
16+
use PHPUnit\Framework\TestCase;
17+
use Psr\Log\LoggerInterface;
18+
19+
class ExAppDeployOptionsServiceTest extends TestCase {
20+
private ExAppDeployOptionsService $service;
21+
private ExAppDeployOptionsMapper&MockObject $mapper;
22+
23+
protected function setUp(): void {
24+
parent::setUp();
25+
26+
$this->mapper = $this->createMock(ExAppDeployOptionsMapper::class);
27+
$cacheFactory = $this->createMock(ICacheFactory::class);
28+
$cacheFactory->method('isAvailable')->willReturn(false);
29+
30+
$this->service = new ExAppDeployOptionsService(
31+
$this->createMock(LoggerInterface::class),
32+
$this->mapper,
33+
$cacheFactory,
34+
);
35+
}
36+
37+
/**
38+
* Two apps with distinct env-var keys. Raw fetch returns both; filtered fetch
39+
* returns only the requested app's rows.
40+
*/
41+
public function testGetDeployOptionsFiltersByAppId(): void {
42+
$this->mapper->method('findAll')->willReturn($this->twoAppRecords());
43+
44+
$all = $this->service->getDeployOptions();
45+
self::assertCount(4, $all);
46+
47+
$appA = $this->service->getDeployOptions('app_a');
48+
self::assertCount(2, $appA);
49+
foreach ($appA as $entry) {
50+
self::assertSame('app_a', $entry->getAppid());
51+
}
52+
53+
$appB = $this->service->getDeployOptions('app_b');
54+
self::assertCount(2, $appB);
55+
foreach ($appB as $entry) {
56+
self::assertSame('app_b', $entry->getAppid());
57+
}
58+
}
59+
60+
/**
61+
* Regression guard for issue #808.
62+
*
63+
* `formatDeployOptions` keys by `type`, so passing the unfiltered result
64+
* (rows for every app) causes the last app in iteration order to overwrite
65+
* entries for earlier apps. Calling `getDeployOptions($appId)` first is the
66+
* only way to get a clean per-app map.
67+
*/
68+
public function testFormatDeployOptionsUnfilteredOverwritesAcrossApps(): void {
69+
$this->mapper->method('findAll')->willReturn($this->twoAppRecords());
70+
71+
$unfiltered = $this->service->formatDeployOptions($this->service->getDeployOptions());
72+
// Last app in iteration order wins — app_b's keys leak into the env_vars entry.
73+
self::assertArrayHasKey('environment_variables', $unfiltered);
74+
self::assertSame(
75+
['APP_B_VAR'],
76+
array_keys($unfiltered['environment_variables']),
77+
'Unfiltered format is order-dependent: last app overwrites earlier apps.',
78+
);
79+
80+
$filtered = $this->service->formatDeployOptions($this->service->getDeployOptions('app_a'));
81+
self::assertSame(
82+
['APP_A_VAR_1', 'APP_A_VAR_2'],
83+
array_keys($filtered['environment_variables']),
84+
'Filtered format returns only the requested app\'s env vars.',
85+
);
86+
}
87+
88+
/**
89+
* @return list<array{id: int, appid: string, type: string, value: array<string, mixed>}>
90+
*/
91+
private function twoAppRecords(): array {
92+
return [
93+
[
94+
'id' => 1,
95+
'appid' => 'app_a',
96+
'type' => 'environment_variables',
97+
'value' => [
98+
'APP_A_VAR_1' => ['name' => 'APP_A_VAR_1', 'value' => 'a1'],
99+
'APP_A_VAR_2' => ['name' => 'APP_A_VAR_2', 'value' => 'a2'],
100+
],
101+
],
102+
[
103+
'id' => 2,
104+
'appid' => 'app_a',
105+
'type' => 'mounts',
106+
'value' => [],
107+
],
108+
[
109+
'id' => 3,
110+
'appid' => 'app_b',
111+
'type' => 'environment_variables',
112+
'value' => [
113+
'APP_B_VAR' => ['name' => 'APP_B_VAR', 'value' => 'b1'],
114+
],
115+
],
116+
[
117+
'id' => 4,
118+
'appid' => 'app_b',
119+
'type' => 'mounts',
120+
'value' => [],
121+
],
122+
];
123+
}
124+
}

0 commit comments

Comments
 (0)