Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/Controller/GlobalSettingsController.php
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,8 @@ public function getGlobalSettings() : JSONResponse {
public function setGlobalSettings(array $globalSettings) : JSONResponse {
return $this->tryExecute(function () use ($globalSettings) {
$globalSettingsObject = new GlobalSettings();
$globalSettingsObject->processorCount = $globalSettings['processorCount'];
$globalSettingsObject->timeout = $globalSettings['timeout'] ?? null;
$globalSettingsObject->processorCount = isset($globalSettings['processorCount']) ? (int)$globalSettings['processorCount'] : null;
$globalSettingsObject->timeout = isset($globalSettings['timeout']) ? (int)$globalSettings['timeout'] : null;

Comment on lines 61 to 64
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isset($globalSettings['processorCount']) will treat an empty string ("" from an empty number input) as set, and (int)'' becomes 0. That ends up persisting a 0 value even though the service treats <= 0 as “not configured”. Consider coercing "" (and other non-numeric inputs) to null, and ideally normalizing <= 0 to null before calling the service (e.g., via a small helper that returns null unless the parsed value is > 0).

Copilot uses AI. Check for mistakes.
$this->globalSettingsService->setGlobalSettings($globalSettingsObject);
return $this->globalSettingsService->getGlobalSettings();
Expand Down
107 changes: 107 additions & 0 deletions lib/Migration/Version1034Date20260331194630.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
<?php

declare(strict_types=1);

/**
* @copyright Copyright (c) 2026 Robin Windey <ro.windey@gmail.com>
*
* @license GNU AGPL version 3 or any later version
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License as
* published by the Free Software Foundation, either version 3 of the
* License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

namespace OCA\WorkflowOcr\Migration;

use Closure;
use OCA\WorkflowOcr\AppInfo\Application;
use OCP\DB\ISchemaWrapper;
use OCP\IAppConfig;
use OCP\IDBConnection;
use OCP\Migration\IOutput;
use OCP\Migration\SimpleMigrationStep;

/**
* Converts the 'processorCount' and 'timeout' app-config values from the
* legacy string type (VALUE_STRING = 4) to the correct integer type
* (VALUE_INT = 8). Nextcloud's IAppConfig refuses to call setValueInt() on
* a key that was previously stored via setValueString(), so we must delete the
* old rows and re-insert them with the right type.
*/
class Version1034Date20260331194630 extends SimpleMigrationStep {
/** Integer keys in GlobalSettings that must be migrated. */
private const INT_KEYS = ['processorCount', 'timeout'];

public function __construct(
private IDBConnection $db,
private IAppConfig $appConfig,
) {
}

public function name(): string {
return 'convert GlobalSettings integer config values from string to int type';
}

public function description(): string {
return 'Deletes processorCount and timeout app-config entries that were '
. 'stored as VALUE_STRING and re-inserts them as VALUE_INT so that '
. 'IAppConfig::setValueInt() no longer throws a type-conflict error.';
}

/**
* {@inheritDoc}
*/
public function changeSchema(IOutput $output, Closure $schemaClosure, array $options): ?ISchemaWrapper {
foreach (self::INT_KEYS as $key) {
$this->migrateKey($key, $output);
}
return null;
}

private function migrateKey(string $key, IOutput $output): void {
// Read the raw string value directly from the DB so we can preserve it.
$qb = $this->db->getQueryBuilder();
$result = $qb->select('configvalue', 'type')
->from('appconfig')
->where($qb->expr()->eq('appid', $qb->createNamedParameter(Application::APP_NAME)))
->andWhere($qb->expr()->eq('configkey', $qb->createNamedParameter($key)))
->executeQuery();

$row = $result->fetch();
$result->closeCursor();

if ($row === false) {
// Key not yet stored – nothing to migrate.
return;
}

if ((int)$row['type'] === IAppConfig::VALUE_INT) {
// Already the right type, nothing to do.
return;
}

$intValue = is_numeric($row['configvalue']) ? (int)$row['configvalue'] : 0;

// IAppConfig throws a type-conflict if the stored type differs from the
// requested one, so we must remove the old entry before re-inserting.
$this->appConfig->deleteKey(Application::APP_NAME, $key);
$this->appConfig->setValueInt(Application::APP_NAME, $key, $intValue);

$output->info(sprintf(
"Migrated config key '%s' for app '%s' from string to int (value: %d).",
$key,
Application::APP_NAME,
$intValue,
));
Comment on lines +96 to +105
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The migration currently re-inserts a VALUE_INT even when the legacy string value is empty/non-numeric (mapped to 0). Since the rest of the code treats <= 0 as “not configured”, consider deleting the old key and only re-inserting when the parsed value is actually > 0; otherwise just delete and leave it unset. This avoids persisting a sentinel/invalid value in appconfig.

Suggested change
// requested one, so we must remove the old entry before re-inserting.
$this->appConfig->deleteKey(Application::APP_NAME, $key);
$this->appConfig->setValueInt(Application::APP_NAME, $key, $intValue);
$output->info(sprintf(
"Migrated config key '%s' for app '%s' from string to int (value: %d).",
$key,
Application::APP_NAME,
$intValue,
));
// requested one, so we must remove the old entry before deciding whether
// to re-insert it as an integer value.
$this->appConfig->deleteKey(Application::APP_NAME, $key);
if ($intValue > 0) {
$this->appConfig->setValueInt(Application::APP_NAME, $key, $intValue);
$output->info(sprintf(
"Migrated config key '%s' for app '%s' from string to int (value: %d).",
$key,
Application::APP_NAME,
$intValue,
));
} else {
// For empty/non-numeric or non-positive values, treat as "not configured"
// and leave the key unset instead of persisting a sentinel 0 value.
$output->info(sprintf(
"Deleted legacy string config key '%s' for app '%s' due to non-positive or invalid value.",
$key,
Application::APP_NAME,
));
}

Copilot uses AI. Check for mistakes.
}
}
6 changes: 2 additions & 4 deletions lib/Model/GlobalSettings.php
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,6 @@
namespace OCA\WorkflowOcr\Model;

class GlobalSettings {
/** @var string? */
public $processorCount;
/** @var string? */
public $timeout;
public ?int $processorCount = null;
public ?int $timeout = null;
}
2 changes: 1 addition & 1 deletion lib/OcrProcessors/CommandLineUtils.php
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ public function getCommandlineArgs(WorkflowSettings $settings, GlobalSettings $g
}

// Number of CPU's to be used
$processorCount = intval($globalSettings->processorCount);
$processorCount = $globalSettings->processorCount ?? 0;
if ($processorCount > 0) {
$args[] = '--jobs ' . $processorCount;
}
Expand Down
7 changes: 2 additions & 5 deletions lib/OcrProcessors/Remote/Client/ApiClient.php
Original file line number Diff line number Diff line change
Expand Up @@ -111,11 +111,8 @@ private function exAppRequest(string $path, ?array $options, string $method, boo
*/
private function getTimeout(object $settings): int {
$timeout = self::DEFAULT_TIMEOUT;
if (isset($settings->timeout) && $settings->timeout !== null && $settings->timeout !== '') {
$timeoutInt = (int)$settings->timeout;
if ($timeoutInt > 0) {
$timeout = $timeoutInt;
}
if ($settings->timeout !== null && $settings->timeout > 0) {
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getTimeout() is typed as object but directly accesses $settings->timeout without an isset()/property_exists() guard. If anything other than GlobalSettings is ever passed, this will trigger an undefined-property warning. Consider either tightening the parameter type to GlobalSettings (preferred, since that’s what the caller passes) or reintroducing a guard before accessing the property.

Suggested change
if ($settings->timeout !== null && $settings->timeout > 0) {
if (isset($settings->timeout) && $settings->timeout > 0) {

Copilot uses AI. Check for mistakes.
$timeout = $settings->timeout;
}
return $timeout;
}
Expand Down
22 changes: 19 additions & 3 deletions lib/Service/GlobalSettingsService.php
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,15 @@ public function getGlobalSettings() : GlobalSettings {

foreach ($this->getProperties($settings) as $prop) {
$key = $prop->getName();
$configValue = $this->config->getValueString(Application::APP_NAME, $key);
$settings->$key = $configValue;
$type = $prop->getType();
if ($type instanceof \ReflectionNamedType && $type->getName() === 'int') {
$intValue = $this->config->getValueInt(Application::APP_NAME, $key, 0);
// 0 is used as the "not configured" sentinel: none of the integer settings
// have a meaningful zero value, so 0 → null (not set).
$settings->$key = $intValue > 0 ? $intValue : null;
} else {
$settings->$key = $this->config->getValueString(Application::APP_NAME, $key);
}
}

return $settings;
Expand All @@ -69,7 +76,16 @@ public function setGlobalSettings(GlobalSettings $settings) : void {
foreach ($this->getProperties($settings) as $prop) {
$key = $prop->getName();
$value = $settings->$key;
$this->config->setValueString(Application::APP_NAME, $key, $value);
$type = $prop->getType();
$isNullable = $type !== null && $type->allowsNull();

if ($isNullable && $value === null) {
$this->config->deleteKey(Application::APP_NAME, $key);
} elseif ($type instanceof \ReflectionNamedType && $type->getName() === 'int') {
$this->config->setValueInt(Application::APP_NAME, $key, (int)$value);
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getGlobalSettings() maps int values <= 0 to null, but setGlobalSettings() will still persist 0/negative values via setValueInt() when they are provided. This creates inconsistent semantics (stored value vs returned value) and can leave invalid values in appconfig. Consider treating non-positive ints as “unset” here too (delete the key or skip writing) to keep storage consistent with the read-path logic.

Suggested change
$this->config->setValueInt(Application::APP_NAME, $key, (int)$value);
$intValue = (int)$value;
if ($intValue <= 0) {
// Keep storage consistent with read-path semantics: non-positive ints
// are treated as "not configured", so we delete the key instead of
// persisting an invalid/sentinel value.
$this->config->deleteKey(Application::APP_NAME, $key);
} else {
$this->config->setValueInt(Application::APP_NAME, $key, $intValue);
}

Copilot uses AI. Check for mistakes.
} else {
$this->config->setValueString(Application::APP_NAME, $key, (string)($value ?? ''));
}
}
}

Expand Down
1 change: 1 addition & 0 deletions psalm.xml
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
<directory name="lib" />
<ignoreFiles>
<directory name="vendor" />
<directory name="lib/Migration" />
</ignoreFiles>
</projectFiles>
<extraFiles>
Expand Down
8 changes: 4 additions & 4 deletions src/components/GlobalSettings.vue
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,10 @@
</div>
<div class="div-table-col">
<input
v-model="settings.processorCount"
:value="settings.processorCount"
name="processorCount"
type="number"
@input="save">
@input="settings.processorCount = Number($event.target.value) || null; save()">
</div>
Comment on lines 36 to 41
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The input coercion Number($event.target.value) || null only converts falsy values to null; negative numbers (e.g. “-1”) will be kept and sent to the backend, where they can be persisted even though the service treats <= 0 as unset. Consider explicitly normalizing with a > 0 check (and possibly clamping) so that any <= 0 (and NaN) becomes null.

Copilot uses AI. Check for mistakes.
</div>
<div class="div-table-row">
Expand All @@ -50,11 +50,11 @@
</div>
<div class="div-table-col">
<input
v-model="settings.timeout"
:value="settings.timeout"
name="timeout"
type="number"
min="1"
@input="save">
@input="settings.timeout = Number($event.target.value) || null; save()">
</div>
Comment on lines 51 to 58
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as processorCount: Number($event.target.value) || null allows negative values to flow through. Even though min="1" exists, users can still type negatives manually and the handler will accept them. Consider normalizing in the handler to null unless the parsed number is > 0 to prevent persisting invalid timeouts.

Copilot uses AI. Check for mistakes.
</div>
</NcSettingsSection>
Expand Down
27 changes: 24 additions & 3 deletions src/test/components/GlobalSettings.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ describe('Init tests', () => {

describe('Interaction tests', () => {
test('Should update settings when processorCount is changed', async () => {
const initialMockSettings = { processorCount: '2' }
const initialMockSettings = { processorCount: 2 }
getGlobalSettings.mockResolvedValueOnce(initialMockSettings)

const afterSaveMockSettings = { processorCount: 42 }
Expand All @@ -62,16 +62,37 @@ describe('Interaction tests', () => {
processorCount.element.value = '42'
await processorCount.trigger('input')

// v-model on type="number" input converts to number
// Inline @input handler converts to Number and falls back to null
expect(wrapper.vm.settings.processorCount).toBe(42)
expect(setGlobalSettings).toHaveBeenCalledTimes(1)
expect(setGlobalSettings).toHaveBeenCalledWith(expect.objectContaining({
processorCount: 42,
}))
})

test('Should convert empty input to null', async () => {
const initialMockSettings = { processorCount: 2 }
getGlobalSettings.mockResolvedValueOnce(initialMockSettings)

const afterSaveMockSettings = { processorCount: null }
setGlobalSettings.mockResolvedValueOnce(afterSaveMockSettings)

const wrapper = mount(GlobalSettings, mountOptions)
await new Promise(process.nextTick)

const processorCount = wrapper.find('input[name="processorCount"]')
processorCount.element.value = ''
await processorCount.trigger('input')

expect(wrapper.vm.settings.processorCount).toBeNull()
expect(setGlobalSettings).toHaveBeenCalledTimes(1)
expect(setGlobalSettings).toHaveBeenCalledWith(expect.objectContaining({
processorCount: null,
}))
})

test('Should show error when save fails', async () => {
const initialMockSettings = { processorCount: '2' }
const initialMockSettings = { processorCount: 2 }
getGlobalSettings.mockResolvedValueOnce(initialMockSettings)

setGlobalSettings.mockRejectedValueOnce(new Error('save-failed'))
Expand Down
6 changes: 3 additions & 3 deletions src/test/service/globalSettingsService.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,14 @@ test('getGlobalSettings returns correct data from server', async () => {
})

test('setGlobalSettings sends correct data to server', async () => {
const request = { data: { processorCount: 42 } }
const mockedResponse = { data: { processorCount: '42' } }
const request = { processorCount: 42 }
const mockedResponse = { data: { processorCount: 42 } }

axios.put.mockResolvedValueOnce(mockedResponse)

const result = await setGlobalSettings(request)

expect(result.processorCount).toBe('42')
expect(result.processorCount).toBe(42)
expect(axios.put).toHaveBeenCalledTimes(1)
expect(axios.put).toHaveBeenCalledWith('/apps/workflow_ocr/globalSettings', { globalSettings: request })
})
15 changes: 15 additions & 0 deletions tests/Unit/Controller/GlobalSettingsControllerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -120,4 +120,19 @@ public function testSetSettingsWithoutTimeout() {

$this->controller->setGlobalSettings($settings);
}

public function testSetSettingsPassesNullValues() {
$settings = [
'processorCount' => null,
'timeout' => null,
];

$this->globalSettingsService->expects($this->once())
->method('setGlobalSettings')
->with($this->callback(function (GlobalSettings $settings) {
return $settings->processorCount === null && $settings->timeout === null;
}));

$this->controller->setGlobalSettings($settings);
}
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The controller now casts values to int/null, but there’s no unit test covering the API behavior when the client submits an empty string (""), which is a common payload from cleared number inputs and is called out in the PR description. Adding a test case for "" → null (or "" → delete/unset) would protect the intended behavior.

Suggested change
}
}
public function testSetSettingsConvertsEmptyStringsToNull() {
$settings = [
'processorCount' => '',
'timeout' => '',
];
$this->globalSettingsService->expects($this->once())
->method('setGlobalSettings')
->with($this->callback(function (GlobalSettings $settings) {
return $settings->processorCount === null && $settings->timeout === null;
}));
$this->controller->setGlobalSettings($settings);
}

Copilot uses AI. Check for mistakes.
}
12 changes: 6 additions & 6 deletions tests/Unit/OcrProcessors/Remote/Client/ApiClientTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ public function testHeartbeatFailure(): void {

public function testProcessOcrUsesConfiguredTimeout(): void {
$settings = new GlobalSettings();
$settings->timeout = '120';
$settings->timeout = 120;

$globalSettingsService = $this->createMock(IGlobalSettingsService::class);
$globalSettingsService->method('getGlobalSettings')->willReturn($settings);
Expand Down Expand Up @@ -212,19 +212,19 @@ public function testGetTimeoutParsesValuesCorrectly(): void {
$method->setAccessible(true);

$settings = new GlobalSettings();
$settings->timeout = '';
$settings->timeout = null;
$this->assertEquals(60, $method->invoke($realClient, $settings));

$settings->timeout = '120';
$settings->timeout = 120;
$this->assertEquals(120, $method->invoke($realClient, $settings));

$settings->timeout = '0';
$settings->timeout = 0;
$this->assertEquals(60, $method->invoke($realClient, $settings));

$settings->timeout = '-10';
$settings->timeout = -10;
$this->assertEquals(60, $method->invoke($realClient, $settings));

$settings->timeout = '3600';
$settings->timeout = 3600;
$this->assertEquals(3600, $method->invoke($realClient, $settings));
}
}
Loading
Loading