From 5715e0484fdf4971f598e27a90f609092e809e7c Mon Sep 17 00:00:00 2001 From: Anne-Catherine <57360138+accognet@users.noreply.github.com> Date: Fri, 17 Jul 2026 16:40:09 +0200 Subject: [PATCH] =?UTF-8?q?N=C2=B09759=20-=20Truncating=20AttributeText=20?= =?UTF-8?q?doesn't=20work=20as=20expected=20in=20case=20of=20multibytes=20?= =?UTF-8?q?characters=20(#964)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * N°9759 - Truncate AttributeText don't work as expected in case of multibytes characters * WIP * Move TrimValue() base definition to AttributeDefinition class, assign DBObject::SetTrim old behavior to it * Fix variable types * Fix variable types * Apply code review fixes, add tests * Define GetSize in AttributeDefinition to easy polymorphic calls instead of obscure AttributeDBFieldVoid * Update php with right since version and ticket, remove useless method overload * Apply suggestions from code review Co-authored-by: Molkobain * Update core/attributedef.class.inc.php Co-authored-by: Molkobain * Update core/attributedef.class.inc.php Co-authored-by: Molkobain * Apply suggestion from @Molkobain code review Co-authored-by: Molkobain * Apply suggestion from @Molkobain code review Co-authored-by: Molkobain * Apply suggestion from @Molkobain code review Co-authored-by: Molkobain * Apply suggestion from @Molkobain code review Co-authored-by: Molkobain * Apply suggestion from @Molkobain code review Co-authored-by: Molkobain * Correctly handle null values in new methods --------- Co-authored-by: Stephen Abello Co-authored-by: Molkobain --- core/attributedef.class.inc.php | 99 +++++++++++++++++++ core/dbobject.class.php | 11 +-- .../core/DBObject/DBObjectTest.php | 87 +++++++++++++--- webservices/webservices.class.inc.php | 20 +--- 4 files changed, 178 insertions(+), 39 deletions(-) diff --git a/core/attributedef.class.inc.php b/core/attributedef.class.inc.php index afc52329bc..c8637a33b6 100644 --- a/core/attributedef.class.inc.php +++ b/core/attributedef.class.inc.php @@ -897,6 +897,60 @@ abstract class AttributeDefinition return null; } + /** + * Return the size of $value, expressed in the same unit as {@see static::GetMaxSize()} for this attribute class. + * + * Default unit is a number of **characters**, matching MySQL VARCHAR(M) semantics for VARCHAR-based attributes. + * Byte-based attributes (e.g. {@see AttributeText}, stored as MySQL TEXT which is limited to 65535 **bytes**, + * not characters) MUST override both this method and {@see TrimValue()} consistently. + * + * @param string|null $sValue + * + * @return int Size of $value in the unit of GetMaxSize() (characters by default) + * @since 3.2.3-2 3.2.4 3.3.0 N°9759 + */ + public function GetSize(?string $sValue) + { + // If the value is null, we return 0 + if ($sValue === null) { + return 0; + } + + return mb_strlen($sValue); + } + + /** + * Helper to set a value that fits the attribute max size + * + * Truncation is performed in the same unit as GetMaxSize() / {@see GetSize()}: a number of **characters** + * by default (VARCHAR-based attributes). When truncated, a " -truncated (N chars)" suffix is appended and + * the returned value (suffix included) still fits within GetMaxSize(). + * + * Default behavior is what DBObject::SetTrim used to do, now delegated to AttributeDefinition + * + * @param string|null $sValue + * + * @return string $sValue truncated so that it fits within {@see GetMaxSize()}. + * @since 3.2.3-2 3.2.4 3.3.0 N°9759 + */ + public function TrimValue(?string $sValue) + { + // If the value is null, we return an empty string + if ($sValue === null) { + return ''; + } + + $iMaxSize = $this->GetMaxSize(); + $iLength = mb_strlen($sValue); + if ($iMaxSize && ($iLength > $iMaxSize)) { + $sMessage = " -truncated ($iLength chars)"; + + return mb_substr($sValue, 0, $iMaxSize - mb_strlen($sMessage)).$sMessage; + } + + return $sValue; + } + /** * @return mixed|null * @deprecated never used @@ -4219,6 +4273,51 @@ class AttributeText extends AttributeString return "TEXT".CMDBSource::GetSqlStringColumnDefinition(); } + /** + * @inheritDoc + * + * Unlike the default implementation, the size is expressed in **bytes**: MySQL TEXT columns are limited + * in bytes (65535), not in characters, and {@see static::GetMaxSize()} for this class returns a number of bytes. + */ + public function GetSize(?string $sValue) + { + // If the value is null, we return 0 + if ($sValue === null) { + return 0; + } + + return strlen($sValue); + } + /** + * @inheritDoc + * + * Truncation is performed on a **byte** budget (MySQL TEXT limit) without ever cutting through a multibyte + * UTF-8 sequence: the returned value is always valid UTF-8 and never exceeds GetMaxSize() bytes, + * truncation suffix included. + */ + public function TrimValue(?string $sValue) + { + // If the value is null, we return an empty string + if ($sValue === null) { + return ''; + } + + $iMaxSize = $this->GetMaxSize(); + $iLength = strlen($sValue); + $iLengthChar = mb_strlen($sValue); + if ($iMaxSize && ($iLength > $iMaxSize)) { + $sMessage = " -truncated ($iLengthChar chars)"; + $iTruncatedValueMaxSize = $iMaxSize - strlen($sMessage); + // mb_strcut cuts on a byte budget but moves the cut point back to a character boundary, + // so it never returns a broken multibyte sequence at the end of the value + $sTruncatedValue = mb_strcut($sValue, 0, $iTruncatedValueMaxSize, 'UTF-8'); + + return $sTruncatedValue.$sMessage; + } + + return $sValue; + } + public function GetSQLColumns($bFullSpec = false) { $aColumns = []; diff --git a/core/dbobject.class.php b/core/dbobject.class.php index 0db1783d9c..82a56928d3 100644 --- a/core/dbobject.class.php +++ b/core/dbobject.class.php @@ -732,13 +732,8 @@ abstract class DBObject implements iDisplay public function SetTrim($sAttCode, $sValue) { $oAttDef = MetaModel::GetAttributeDef(get_class($this), $sAttCode); - $iMaxSize = $oAttDef->GetMaxSize(); - $sLength = mb_strlen($sValue); - if ($iMaxSize && ($sLength > $iMaxSize)) { - $sMessage = " -truncated ($sLength chars)"; - $sValue = mb_substr($sValue, 0, $iMaxSize - mb_strlen($sMessage)).$sMessage; - } - $this->Set($sAttCode, $sValue); + + $this->Set($sAttCode, $oAttDef->TrimValue($sValue)); } /** @@ -2040,7 +2035,7 @@ abstract class DBObject implements iDisplay } } if (!is_null($iMaxSize = $oAtt->GetMaxSize())) { - $iLen = mb_strlen($toCheck); + $iLen = $oAtt->GetSize($toCheck); if ($iLen > $iMaxSize) { return "String too long (found $iLen, limited to $iMaxSize)"; } diff --git a/tests/php-unit-tests/unitary-tests/core/DBObject/DBObjectTest.php b/tests/php-unit-tests/unitary-tests/core/DBObject/DBObjectTest.php index adb3e6b528..7f2c27f1a1 100644 --- a/tests/php-unit-tests/unitary-tests/core/DBObject/DBObjectTest.php +++ b/tests/php-unit-tests/unitary-tests/core/DBObject/DBObjectTest.php @@ -37,7 +37,6 @@ use Team; use User; use UserRequest; use UserRights; -use utils; /** * @group specificOrgInSampleData @@ -1297,19 +1296,21 @@ class DBObjectTest extends ItopDataTestCase { return [ // UserRequest.title is an AttributeString (maxsize = 255) - 'title 250 chars' => ['title', 250], - 'title 254 chars' => ['title', 254], - 'title 255 chars' => ['title', 255], - 'title 256 chars' => ['title', 256], - 'title 300 chars' => ['title', 300], + 'title 250 chars' => ['title', 250, 250, true], + 'title 254 chars' => ['title', 254, 254, true], + 'title 255 chars' => ['title', 255, 255, true], + 'title 256 chars' => ['title', 256, 255, false], + 'title 300 chars' => ['title', 300, 255, false], // UserRequest.pending_reason is an AttributeText (maxsize=65535) with format=text - 'pending_reason 250 chars' => ['pending_reason', 250], - 'pending_reason 60000 chars' => ['pending_reason', 60000], - 'pending_reason 65534 chars' => ['pending_reason', 65534], - 'pending_reason 65535 chars' => ['pending_reason', 65535], - 'pending_reason 65536 chars' => ['pending_reason', 65536], - 'pending_reason 70000 chars' => ['pending_reason', 70000], + 'pending_reason 250 chars' => ['pending_reason', 250, 250, true], + 'pending_reason 65534 chars' => ['pending_reason', 65534, 16403, false], + 'pending_reason 65535 chars' => ['pending_reason', 65535, 16403, false], + 'pending_reason 65536 chars' => ['pending_reason', 65536, 16403, false], + 'pending_reason 16385 chars' => ['pending_reason', 16385, 16403, false], + 'pending_reason 16384 chars' => ['pending_reason', 16384, 16384, true], + 'pending_reason 16383 chars' => ['pending_reason', 16383, 16383, true], + 'pending_reason 16382 chars' => ['pending_reason', 16382, 16382, true], ]; } @@ -1322,7 +1323,7 @@ class DBObjectTest extends ItopDataTestCase * * @since 3.1.2 N°3448 - Framework field size check not correctly implemented for multi-bytes languages/strings */ - public function testCheckLongValueInAttribute(string $sAttrCode, int $iValueLength) + public function testCheckLongValueInAttribute(string $sAttrCode, int $iValueLength, int $iExpectedLength, bool $bIsValueToSetBelowAttrMaxSize): void { $sPrefix = 'a'; // just a small prefix so that the emoji bytes won't have a power of 2 (we want a non even value) $sEmojiToRepeat = '😎'; // this emoji is 4 bytes long @@ -1343,17 +1344,18 @@ class DBObjectTest extends ItopDataTestCase $oAttDef = MetaModel::GetAttributeDef(UserRequest::class, $sAttrCode); $iAttrMaxSize = $oAttDef->GetMaxSize(); - $bIsValueToSetBelowAttrMaxSize = ($iValueLength <= $iAttrMaxSize); + $bExpectedStatus = ($oAttDef->GetSize($sValueToSet) <= $iAttrMaxSize); + $this->assertSame($bExpectedStatus, $bIsValueToSetBelowAttrMaxSize, 'The data provider must stay aligned with the attribute max size logic.'); /** @noinspection PhpUnusedLocalVariableInspection */ [$bCheckStatus, $aCheckIssues, $bSecurityIssue] = $oTicket->CheckToWrite(); $this->assertEquals($bIsValueToSetBelowAttrMaxSize, $bCheckStatus, "CheckResult result:".var_export($aCheckIssues, true)); $oTicket->SetTrim($sAttrCode, $sValueToSet); $sValueInObject = $oTicket->Get($sAttrCode); + $this->assertEquals($iExpectedLength, mb_strlen($sValueInObject), 'Should match expected resulting value length.'); if ($bIsValueToSetBelowAttrMaxSize) { $this->assertEquals($sValueToSet, $sValueInObject, 'Should not alter string that is already shorter than attribute max length'); } else { - $this->assertEquals($iAttrMaxSize, mb_strlen($sValueInObject), 'Should truncate at the same length than attribute max length'); $sLastCharsOfValueInObject = mb_substr($sValueInObject, -30); $this->assertStringContainsString(' -truncated', $sLastCharsOfValueInObject, 'Should end with "truncated" comment'); } @@ -1386,6 +1388,61 @@ class DBObjectTest extends ItopDataTestCase $this->assertEquals($sResult, $oOrganisation->Get('name'), 'SetTrim must limit string to 255 characters'); } + /** + * Check that DBObject::SetTrim doesn't cut through multibytes characters + * + * @covers DBObject::SetTrim + * @dataProvider SetTrimAttributeTextProvider + */ + public function testSetTrimOnAttributeTextKeepsUtf8Validity(string $sChar, int $iRepeatCount, bool $bExpectExactByteFill): void + { + $oTicket = MetaModel::NewObject('UserRequest', [ + 'ref' => 'Test Ticket', + 'title' => 'Create OK', + 'description' => 'Create OK', + 'caller_id' => 15, + 'org_id' => 3, + ]); + + $sValueToSet = str_repeat($sChar, $iRepeatCount); + $oTicket->SetTrim('pending_reason', $sValueToSet); + $sValueInObject = $oTicket->Get('pending_reason'); + + $oAttDef = MetaModel::GetAttributeDef('UserRequest', 'pending_reason'); + $iAttrMaxSize = $oAttDef->GetMaxSize(); + $iOriginalCharLength = mb_strlen($sValueToSet); + $sMessage = " -truncated ($iOriginalCharLength chars)"; + + $this->assertStringEndsWith($sMessage, $sValueInObject, 'Trimmed value should keep the expected truncation suffix.'); + $this->assertTrue(mb_check_encoding($sValueInObject, 'UTF-8'), 'Trimmed value should stay valid UTF-8.'); + $this->assertLessThanOrEqual($iAttrMaxSize, strlen($sValueInObject), 'Trimmed value should never exceed attribute byte max size.'); + + if ($bExpectExactByteFill) { + $this->assertSame($iAttrMaxSize, strlen($sValueInObject), 'When byte cut lands on a character boundary, SetTrim should use all available bytes.'); + } + } + + public function SetTrimAttributeTextProvider() + { + return [ + // 2-byte UTF-8 chars: truncation payload size is byte-aligned and should fill the max size exactly. + 'pending_reason 2-byte chars on byte boundary' => ["\xC3\xA9", 32768, true], + // 4-byte UTF-8 chars: truncation payload size is not byte-aligned and must backtrack to valid UTF-8. + 'pending_reason 4-byte chars with mid-character byte cut' => ['💃', 16385, false], + ]; + } + + /** + * @covers DBObject::SetTrim + */ + public function testSetTrimOnNonStringAttributeDoesNotTrim() + { + $oTicket = MetaModel::NewObject(UserRequest::class); + $oTicket->SetTrim('caller_id', '15'); + + $this->assertEquals(15, $oTicket->Get('caller_id'), 'SetTrim should keep non-string attributes untouched before regular Set conversion'); + } + /** * @covers DBObject::SetComputedDate * @return void diff --git a/webservices/webservices.class.inc.php b/webservices/webservices.class.inc.php index a65f56ab19..cf90fe997d 100644 --- a/webservices/webservices.class.inc.php +++ b/webservices/webservices.class.inc.php @@ -277,25 +277,13 @@ abstract class WebServicesBase $oLog->Set('userinfo', UserRights::GetUser()); $oLog->Set('verb', $sVerb); $oLog->Set('result', $oRes->IsOk()); - $this->TrimAndSetValue($oLog, 'log_info', (string)$oRes->GetInfoAsText()); - $this->TrimAndSetValue($oLog, 'log_warning', (string)$oRes->GetWarningsAsText()); - $this->TrimAndSetValue($oLog, 'log_error', (string)$oRes->GetErrorsAsText()); - $this->TrimAndSetValue($oLog, 'data', (string)$oRes->GetReturnedDataAsText()); + $oLog->SetTrim('log_info', (string)$oRes->GetInfoAsText()); + $oLog->SetTrim('log_warning', (string)$oRes->GetWarningsAsText()); + $oLog->SetTrim('log_error', (string)$oRes->GetErrorsAsText()); + $oLog->SetTrim('data', (string)$oRes->GetReturnedDataAsText()); $oLog->DBInsertNoReload(); } - protected function TrimAndSetValue($oLog, $sAttCode, $sValue) - { - $oAttDef = MetaModel::GetAttributeDef(get_class($oLog), $sAttCode); - if (is_object($oAttDef)) { - $iMaxSize = $oAttDef->GetMaxSize(); - if ($iMaxSize && (mb_strlen($sValue) > $iMaxSize)) { - $sValue = mb_substr($sValue, 0, $iMaxSize); - } - $oLog->Set($sAttCode, $sValue); - } - } - /** * Helper to set a scalar attribute *