-
Notifications
You must be signed in to change notification settings - Fork 9
Fix TypeError when saving integer CPU core count in global settings #376
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
e65da91
020c004
40cd602
2f65fec
5da3670
983b1b1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // 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, | |
| )); | |
| } |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -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) { | ||||||
|
||||||
| if ($settings->timeout !== null && $settings->timeout > 0) { | |
| if (isset($settings->timeout) && $settings->timeout > 0) { |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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; | ||||||||||||||||||||||
|
|
@@ -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); | ||||||||||||||||||||||
|
||||||||||||||||||||||
| $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); | |
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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
|
||
| </div> | ||
| <div class="div-table-row"> | ||
|
|
@@ -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
|
||
| </div> | ||
| </NcSettingsSection> | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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); | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
| } | |
| } | |
| 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); | |
| } |
There was a problem hiding this comment.
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)''becomes0. That ends up persisting a0value even though the service treats<= 0as “not configured”. Consider coercing "" (and other non-numeric inputs) tonull, and ideally normalizing<= 0tonullbefore calling the service (e.g., via a small helper that returnsnullunless the parsed value is > 0).