[Master]-Post Inventory Cost to G/L fails when concatenated dimension text exceeds 250 characters - #11144
Conversation
GitHub Actions
Inactive
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Good Sense Reviewer - Round 1Recommendation: Request ChangesWhat this PR doesThis change builds the next dimension-text candidate before assigning it to the report field, so the report can stop before the value exceeds 250 characters. The logic matches the reported failure and the added test covers the posting-report preview path, but the changed assignment still introduces a new analyzer warning that fails validation. Problem-solution fitFit: Strong The bug is clear: a complete dimension pair can make the concatenated text too long. The fix targets that exact boundary and keeps the last complete value that fits. SuggestionsS1 (🔴 High): Make the bounded assignment analyzer-safe Risk assessment and necessityRisk: This is a posting report path, so a bad fix can block inventory-cost posting review output. The change is narrow and does not change posting amounts, events, or public signatures, but the current validation failure blocks safe merge. Necessity: The change is required because valid dimension data can exceed the report field limit. The added regression test exercises the important path and confirms the report keeps complete dimension pairs only.
|
|
GetDimText builds the next concatenated dimension-text value into Suggested fix (apply manually — could not be anchored as a one-click suggestion): local procedure GetDimText(var DimSetEntry: Record "Dimension Set Entry")
var
CandidateDimText: Text;
begin
DimText := '';
if DimSetEntry.FindSet() then
repeat
if DimText = '' then
CandidateDimText := StrSubstNo('%1 - %2', DimSetEntry."Dimension Code", DimSetEntry."Dimension Value Code")
else
CandidateDimText :=
StrSubstNo(
'%1; %2 - %3', DimText, DimSetEntry."Dimension Code", DimSetEntry."Dimension Value Code");
if StrLen(CandidateDimText) > MaxStrLen(DimText) then
exit;
DimText := CandidateDimText;
until DimSetEntry.Next() = 0;
end;Line mapping was unavailable, so this was posted as an issue comment. 👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.39.6 |
|
In Suggested fix (apply manually — could not be anchored as a one-click suggestion): CandidateDimText: Text[250];
begin
DimText := '';
if DimSetEntry.FindSet() then
repeat
if DimText = '' then
CandidateDimText := StrSubstNo('%1 - %2', DimSetEntry."Dimension Code", DimSetEntry."Dimension Value Code")
else
CandidateDimText :=
StrSubstNo(
'%1; %2 - %3', DimText, DimSetEntry."Dimension Code", DimSetEntry."Dimension Value Code");
if StrLen(CandidateDimText) > MaxStrLen(DimText) then
exit;
DimText := CandidateDimText;
until DimSetEntry.Next() = 0;Line mapping was unavailable, so this was posted as an issue comment. 👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.39.6 |
Good Sense Reviewer - Round 2Recommendation: Request ChangesWhat this PR doesThis change tries to keep the last complete dimension text that fits in Status of previous suggestions
New observations (commits since round 1)S1 (🔴 High): Keep the candidate text unbounded Risk assessment and necessityRisk: This is an inventory cost posting report path. If the fix merges as-is, the same long-dimension preview scenario can still fail instead of showing the report. The change does not affect posting amounts, events, or public signatures. Necessity: The fix is required because valid dimension data can exceed the report field limit. The scope is right, but the candidate text must stay unbounded until after the length guard.
|
Good Sense Reviewer - Round 3Recommendation: Request ChangesWhat this PR doesThis update tries to prevent the dimension text overflow by truncating each candidate with Status of previous suggestions
New observations (commits since round 2)None - the latest commit only attempts to address the previous suggestion, but the blocker remains open. Risk assessment and necessityRisk: This is an inventory cost posting report preview path. The change does not affect posting amounts, events, or public signatures, but it can still show a partial dimension pair instead of the last complete valid text. Necessity: The fix is required because valid dimension data can exceed the report field limit. The scope is right, but the candidate must stay unbounded until after the length guard.
|
| DimText := OldDimText; | ||
| OldDimText := | ||
| StrSubstNo('%1; %2 - %3', DimText, DimSetEntry."Dimension Code", DimSetEntry."Dimension Value Code"); | ||
| DimText := CopyStr(OldDimText, 1, MaxStrLen(DimText)); |
There was a problem hiding this comment.
In the rewritten GetDimText, DimText := CopyStr(OldDimText, 1, MaxStrLen(DimText)) runs unconditionally, before the overflow check if StrLen(OldDimText) > MaxStrLen(DimText) then exit. When the newly built OldDimText overflows (e.g. the 6th dimension pair pushes it to 268 characters against a 250-char limit), CopyStr still commits a truncated value into DimText that contains the first 223 correct characters plus a partial, mid-pair fragment of the 6th dimension pair (chars 224-250) — not the clean 5-pair value the new test (PostInventoryCostToGLWithDimensionTextLongerThan250Characters) expects via ExpectedDimText. The exit then leaves this corrupted/truncated value in DimText instead of the last cleanly-fitting value. Reorder so the overflow check runs before the assignment (or restore the prior DimText value on overflow, as the old code did via DimText := OldDimText), so a partially-cut dimension pair is never committed to the report output.
Suggested fix (apply manually — could not be anchored as a one-click suggestion):
if StrLen(OldDimText) > MaxStrLen(DimText) then
exit;
DimText := CopyStr(OldDimText, 1, MaxStrLen(DimText));👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.41.6
| OldDimText := DimText; | ||
| if DimText = '' then | ||
| DimText := StrSubstNo('%1 - %2', DimSetEntry."Dimension Code", DimSetEntry."Dimension Value Code") | ||
| OldDimText := |
There was a problem hiding this comment.
GetDimText now builds the next dimension string directly into OldDimText: Text[250]. In the new test scenario, the sixth StrSubstNo(...) result is 268 characters long, so assigning it with := can raise AL's runtime "string too long" error before CopyStr(...) or the subsequent length check ever runs, because OldDimText has the exact same fixed length (250) as DimText. Build into an unbounded Text temporary (or truncate before assigning to a fixed-length variable) so the report can safely stop after the last fitting pair instead of risking a runtime overflow error.
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.41.6
| QuantityErr: Label 'Quantity Must Be %1 for %2 Document No. %3'; | ||
| NothingToPostTxt: Label 'There is nothing to post to the general ledger.'; | ||
| ValueEntriesWerePostedTxt: Label 'value entries have been posted to the general ledger.'; | ||
| QuantityErr: Label 'Quantity Must Be %1 for %2 Document No. %3'; |
There was a problem hiding this comment.
SetupBlockedErr has a Comment, but it still groups %2-%5 together as "posting groups" instead of naming each placeholder individually. This label is only used with two caption/value pairs, so translators need separate descriptions for each ordinal to localize the sentence correctly.
Suggested fix (apply manually — could not be anchored as a one-click suggestion):
SetupBlockedErr: Label 'Setup is blocked in %1 for %2 %3 and %4 %5.', Comment = '%1 = Setup table caption, %2 = Gen. Bus. Posting Group caption, %3 = Gen. Bus. Posting Group value, %4 = Gen. Prod. Posting Group caption, %5 = Gen. Prod. Posting Group value.';Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.41.6
|
This report enforces the request-page journal fields only in OnPreReport(). If Journal Template Name or Journal Batch Name is left blank, the request page closes and then errors, so the user loses the entered options instead of correcting them in place. Add the same validation to requestpage.OnQueryClosePage() behind 'if CloseAction = Action::OK then', keep the OnPreReport() check for non-interactive runs, and mark the visible required controls with ShowMandatory. Knowledge:
Line mapping was unavailable, so this was posted as an issue comment. 👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.41.6 |
| '%1; %2 - %3', DimText, DimSetEntry."Dimension Code", DimSetEntry."Dimension Value Code"); | ||
| if StrLen(DimText) > MaxStrLen(OldDimText) then begin | ||
| DimText := OldDimText; | ||
| OldDimText := |
There was a problem hiding this comment.
This change still leaves GetDimText vulnerable to the same unhandled runtime overflow it is trying to avoid. OldDimText is declared as Text[250], but on the sixth 20-character dimension pair the expression StrSubstNo('%1; %2 - %3', DimText, ...) can reach 268 characters (223 already accumulated in DimText after five pairs, plus 45 more), so assigning it to OldDimText raises the AL string-length runtime error ('The length of the string is X, but it must be less than or equal to Y characters') before CopyStr(...) or the following length check can run. The fix relocates the overflow from DimText to OldDimText without removing it, since both are bounded Text[250] variables. Use an unbounded temporary Text value for the computed candidate string, then CopyStr into DimText and exit when the unbounded value's length exceeds MaxStrLen(DimText). Note: this is a concrete, high-impact defect (the report would still error/crash on dimension text over ~250-268 chars) but is capped to 'minor' severity here per the agent-finding contract because no BCQuality knowledge file backs it directly; it should be verified against a live BC runtime and, if confirmed, treated as blocking for merge.
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.41.6
| DimensionValue: Record "Dimension Value"; | ||
| DimensionCode: Code[20]; | ||
| DimensionValueCode: Code[20]; | ||
| i: Integer; |
There was a problem hiding this comment.
The new fixture helper CreateMaxLengthDimensionSet hand-rolls Dimension records with Init/Validate/Insert(true) and invented primary keys instead of using the test library. That is the anti-pattern this rule calls out: the helper now owns record-shape details that the Library codeunits already maintain, which makes the test more fragile across schema changes and increases the risk of data/setup failures unrelated to the behavior under test.
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.41.6
|
|
||
| // [THEN] The report contains the five complete pairs that fit and omits the sixth pair. | ||
| LibraryReportDataset.LoadDataSetFile(); | ||
| LibraryReportDataset.AssertElementWithValueExists('DimText', ExpectedDimText); |
There was a problem hiding this comment.
With the fixed six 20-character dimension codes and values created by this test, ExpectedDimText is built from only the first five pairs (223 characters), while the test's [THEN] step comments describe the sixth pair as merely 'omitted'. Given the arithmetic in GetDimText, the sixth pair's StrSubstNo result is 268 characters, which (per the related error-handling finding) triggers an AL runtime length error on assignment to OldDimText (Text[250]) rather than being cleanly excluded. If that error-handling defect is real, this test would fail with a runtime error rather than passing with a clean 5-pair DimText, so the assertion does not actually validate the intended 'no failure' scenario end-to-end. Recommend adding an assertion or handler that also verifies the report completes without an unhandled error, and confirming the exact truncation boundary against a live BC runtime before relying on this test as proof of the fix.
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.41.6
Good Sense Reviewer - Round 4Recommendation: Request ChangesWhat this PR doesThis update now builds the next dimension-text candidate, copies it into the report field, and then checks whether the candidate is too long. The reported scenario is still not safely fixed: the candidate variable is still bounded to 250 characters, and Status of previous suggestions
New observations (commits since round 3)None - the latest commit only attempts to address the previous suggestion, but the blocker remains open. Risk assessment and necessityRisk: This is an inventory cost posting report preview path. It does not change posting amounts, public signatures, or events, but it can still fail before the guard or show a partial dimension pair instead of the last complete valid text. Necessity: The fix is required because valid dimension data can exceed the report field limit. The scope is right, but the candidate must stay unbounded until after the length guard passes.
|
|
|
||
| if DimSetEntry.FindSet() then | ||
| repeat | ||
| OldDimText := DimText; |
There was a problem hiding this comment.
GetDimText in PostInventoryCosttoGL.Report.al (lines ~857-867) has a logic bug introduced by this PR's refactor: on the first loop iteration (DimText = ''), the if-branch correctly computes the no-separator format '%1 - %2' into OldDimText, but the code then unconditionally falls through (outside the if/else) to recompute OldDimText using the separator format '%1; %2 - %3' with DimText still ''. This overwrites the correct value and produces a spurious leading '; ' before the first dimension pair (e.g. '; DIM1 - VAL1' instead of 'DIM1 - VAL1'). This corrupts the DimText report column for every posting that has at least one dimension. It also means the new test added in this same PR (PostInventoryCostToGLWithDimensionTextLongerThan250Characters / CreateMaxLengthDimensionSet), which builds ExpectedDimText using the no-separator format for the first pair, will not match the actual report output and either fails or was validated against incorrect output.
Recommendation:
- only perform the unconditional StrSubstNo('%1; %2 - %3', ...) + CopyStr assignment in the non-empty-DimText branch; in the DimText = '' branch, assign DimText from the already-computed OldDimText (bounded via CopyStr) instead of recomputing it with the wrong format.
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.41.6
Good Sense Reviewer - Round 5Recommendation: AcceptWhat this PR doesThis update changes the dimension text builder so it checks the next complete dimension pair before assigning it to the bounded report text. The reported scenario is now handled: when the next pair would exceed 250 characters, the report keeps the previous complete value and exits without adding a partial pair. Status of previous suggestions
New observations (commits since round 4)None - the latest commits address the previous blocker without adding a new issue. Risk assessment and necessityRisk: This is limited to dimension text rendering in the Post Inventory Cost to G/L report. It does not change posting amounts, ledger entries, public signatures, or events, and shorter dimension sets keep the same output. Necessity: The change is needed because valid dimension data can exceed the report field limit. The scope is now right because it stops only when the next complete pair would exceed the limit.
|
Good Sense Reviewer - Round 6Recommendation: AcceptWhat this PR doesThis update builds the next dimension-text candidate in an unbounded variable before copying it into the 250-character report field. The reported long-dimension scenario is handled because the report now keeps the last complete dimension text and exits before assigning an overlong value to the bounded field. Status of previous suggestions
New observations (commits since round 5)None - the latest commit addresses the previous blocker without adding a new issue. Risk assessment and necessityRisk: This is limited to dimension text rendering in the Post Inventory Cost to G/L report. It does not change posting amounts, ledger entries, public signatures, or events; shorter dimension sets keep the same output, and overlong sets now stop at the last complete pair that fits. Necessity: The change is needed because valid dimension data can produce display text longer than the report field. The scope is right because it changes only the local text-building path and the added regression test covers the long-dimension case.
|
2 participants
Fixes AB#649310