N°9759 - Truncating AttributeText doesn't work as expected in case of multibytes characters (#964)

* 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 <lajarige.guillaume@free.fr> * Update core/attributedef.class.inc.php Co-authored-by: Molkobain <lajarige.guillaume@free.fr> * Update core/attributedef.class.inc.php Co-authored-by: Molkobain <lajarige.guillaume@free.fr> * Apply suggestion from @Molkobain code review Co-authored-by: Molkobain <lajarige.guillaume@free.fr> * Apply suggestion from @Molkobain code review Co-authored-by: Molkobain <lajarige.guillaume@free.fr> * Apply suggestion from @Molkobain code review Co-authored-by: Molkobain <lajarige.guillaume@free.fr> * Apply suggestion from @Molkobain code review Co-authored-by: Molkobain <lajarige.guillaume@free.fr> * Apply suggestion from @Molkobain code review Co-authored-by: Molkobain <lajarige.guillaume@free.fr> * Correctly handle null values in new methods --------- Co-authored-by: Stephen Abello <stephen.abello@combodo.com> Co-authored-by: Molkobain <lajarige.guillaume@free.fr>
2026-07-18 04:36:36 +02:00 · 2026-07-17 16:40:09 +02:00
parent d6ef4fb7bb
commit 5715e0484f
4 changed files with 178 additions and 39 deletions
--- 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 = [];
--- 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)";
 				}
--- 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
--- 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
 	 *