Skip to content

Slice 611704: With Autoincrement Tax Transaction Value ID field changed to BigInteger to prevent IDENTITY exhaustion #10510

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.

Sign up for GitHub

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

Open
Tabrez Ajaz (v-ajaztabrez) wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Slice 611704: With Autoincrement Tax Transaction Value ID field changed to BigInteger to prevent IDENTITY exhaustion #10510

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 1 addition & 2 deletions .../TaxEngine-PostingHandler/src/PostingManagement/codeunit/TaxDocumentGLPosting.Codeunit.al
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,7 @@ codeunit 20341 "Tax Document GL Posting"
ToTaxTransactionValue.Init();
ToTaxTransactionValue := FromTaxTransactionValue;
ToTaxTransactionValue."Tax Record ID" := ToRecID;
ToTaxTransactionValue.ID := 0;
ToTaxTransactionValue.Insert();
ToTaxTransactionValue.Insert(true);
until FromTaxTransactionValue.Next() = 0;
end;

Expand Down
2 changes: 1 addition & 1 deletion .../TaxEngine-PostingHandler/src/PostingManagement/codeunit/TaxPostingBufferMgmt.Codeunit.al
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,7 @@ codeunit 20343 "Tax Posting Buffer Mgmt."
InvoiceQty: Decimal)
var
TaxTransactionValue: Record "Tax Transaction Value";
NextID: Integer;
NextID: BigInteger;
begin
TempTransactionValue.Reset();
NextID := TempTransactionValue.Count();
Expand Down
18 changes: 17 additions & 1 deletion ...gine/app/TaxEngine-TaxTypeHandler/src/TransactionValue/table/TaxTransactionValue.Table.al
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ namespace Microsoft.Finance.TaxEngine.TaxTypeHandler;
using Microsoft.Finance.Currency;
using Microsoft.Finance.GeneralLedger.Journal;
using Microsoft.Finance.TaxEngine.Core;
using Microsoft.Foundation.NoSeries;
using Microsoft.Inventory.Transfer;
using Microsoft.Purchases.Archive;
using Microsoft.Purchases.Document;
Expand Down Expand Up @@ -101,12 +102,16 @@ table 20261 "Tax Transaction Value"
DataClassification = EndUserIdentifiableInformation;
Caption = 'Visible on Interface';
}
field(18; "ID"; Integer)
#pragma warning disable AS0146
#pragma warning disable AS0041
field(18; "ID"; BigInteger)

@github-actions github-actions Bot Aug 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

$\textbf{🟡\ Medium\ Severity\ —\ Breaking\ Changes}$

Table 20261 "Tax Transaction Value" field 18 "ID" changes type from Integer to BigInteger in place (kept AutoIncrement = true), wrapped in #pragma warning disable AS0146 / AS0041 to suppress the analyzer rules that specifically guard against changing a published field's type/AutoIncrement semantics. Suppressing those analyzer warnings rather than following the obsoletion lifecycle (new field + Obsolete on the old one) is a real breaking-change risk for any extension compiled against or persisting this field as Integer.

Knowledge:

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.35.4

@github-actions github-actions Bot Aug 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

$\textbf{🟡\ Medium\ Severity\ —\ Data\ Modeling}$

Table 20261 field 18 "ID" keeps AutoIncrement = true while the new OnInsert trigger also assigns Rec.ID from SequenceNoMgt.GetNextSeqNoBigInt. Every other table in this codebase whose key/ID field is populated through SequenceNoMgt.GetNextSeqNo* (e.g. "Sent Notification Entry", "Interaction Log Entry") does NOT mark that field AutoIncrement, precisely because the two ID-assignment mechanisms are meant to be mutually exclusive. Leaving AutoIncrement = true alongside the manual sequence assignment deviates from that established convention and, if the impact of the interaction turns out to be functionally significant (e.g. duplicate/gapped IDs across the two counters), this should be escalated to a major/blocker defect and a corresponding knowledge rule authored — remove AutoIncrement from the field definition to match the rest of the codebase.

Agent judgement — not directly backed by a BCQuality knowledge article.

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.35.4

{
DataClassification = SystemMetadata;
Caption = 'ID';
AutoIncrement = true;
}
#pragma warning restore AS0041
#pragma warning restore AS0146
field(19; "Tax Type"; Code[20])
{
DataClassification = EndUserIdentifiableInformation;
Expand Down Expand Up @@ -153,6 +158,17 @@ table 20261 "Tax Transaction Value"
{
}
}

trigger OnInsert()

@github-actions github-actions Bot Aug 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

$\textbf{🟡\ Medium\ Severity\ —\ Performance}$

The new Tax Transaction Value.OnInsert trigger assigns ID through SequenceNoMgt.GetNextSeqNoBigInt() per insert, and this PR switches the bulk-copy path (TaxDocumentGLPosting.TransferTransactionValue) and TaxRateComputation.InsertTaxTransactionValue to Insert(true) without pre-reserving a sequence buffer, adding one number-sequence lookup per row for high-volume tax posting. Consider SequenceNoMgt.AllocateSeqNoBuffer before the row loop for the bulk-copy path.

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.35.4

var
SequenceNoMgt: Codeunit "Sequence No. Mgt.";
begin
if Rec.IsTemporary() then
exit;

Rec.ID := SequenceNoMgt.GetNextSeqNoBigInt(Database::"Tax Transaction Value");
end;

procedure GetAttributeColumName(): Text
var
TaxAttribute: Record "Tax Attribute";
Expand Down
2 changes: 1 addition & 1 deletion ...axEngine/app/TaxEngine-UseCaseBuilder/src/UseCase/codeunit/TaxRateComputation.Codeunit.al
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ codeunit 20291 "Tax Rate Computation"
TaxTransactionValue."Value Type" := TransactionValueType;
TaxTransactionValue."Tax Type" := TaxType;
TaxTransactionValue."Value ID" := ID;
TaxTransactionValue.Insert();
TaxTransactionValue.Insert(true);
end;

local procedure ModifyTaxTransactionValue(
Expand Down
71 changes: 71 additions & 0 deletions src/Layers/W1/BaseApp/Foundation/NoSeries/SequenceNoMgt.Codeunit.al
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,65 @@ codeunit 9500 "Sequence No. Mgt."
exit(CurrSeqNo);
end;

/// <summary>

@github-actions github-actions Bot Aug 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

$\textbf{🟡\ Medium\ Severity\ —\ Style}$

GetNextSeqNoBigInt and GetCurrentSeqNoBigInt add new public return-value procedures to the Sequence No. Mgt. library codeunit, but their XML doc stops at <param> and omits a <returns> tag, unlike similar existing procedures.

Knowledge:

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.35.4

/// Returns the next NumberSequence value for a given table ID as a BigInteger.
/// Use for tables whose primary key number field is a BigInteger. If the sequence does not exist, it will be created.
/// </summary>
/// <param name="TableNo">The ID of the table being checked</param>
procedure GetNextSeqNoBigInt(TableNo: Integer): BigInteger

@github-actions github-actions Bot Aug 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

$\textbf{🟠\ High\ Severity\ —\ Security}$

The new public BigInteger sequence APIs (GetNextSeqNoBigInt, ValidateSeqNoBigInt, GetCurrentSeqNoBigInt) accept an arbitrary TableNo and route it into the existing GetLastEntryNoFromTable(), which does RecordRef.Open(TableNo) on the caller-controlled table without an allow-list. This mirrors the same shape as the pre-existing GetNextSeqNo/ValidateSeqNo Integer variants in this codeunit, so it is a continuation of an existing pattern rather than a newly introduced flaw, but it is still worth flagging on the new public surface: restrict these procedures' callers or validate TableNo before opening the table.

Knowledge:

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.35.4

var
NewSeqNo: BigInteger;
PreviewMode: Boolean;
begin
PreviewMode := IsPreviewMode(); // Only call once to minimize sql calls during preview.
ValidateSeqNoBigInt(TableNo);
if TryGetNextNoBigInt(PreviewMode, TableNo, NewSeqNo) then
exit(NewSeqNo);
ClearLastError();
CreateNewTableSequence(PreviewMode, TableNo);
TryGetNextNoBigInt(PreviewMode, TableNo, NewSeqNo);

@github-actions github-actions Bot Aug 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

$\textbf{🟠\ High\ Severity\ —\ Error\ Handling}$

This standalone call ignores the Boolean result of [TryFunction] TryGetNextNoBigInt, so a failure after CreateNewTableSequence is handled as an ordinary throwing call instead of try-method logic. Consume the return value and handle false explicitly.

Suggested fix (apply manually — could not be anchored as a one-click suggestion):

if not TryGetNextNoBigInt(PreviewMode, TableNo, NewSeqNo) then
    Error(GetLastErrorText());

Knowledge:

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.35.4

exit(NewSeqNo);
end;

/// <summary>
/// Returns the current NumberSequence value for a given table ID as a BigInteger.
/// if the sequence does not exist, it will be created.
/// </summary>
/// <param name="TableNo">The ID of the table being checked</param>
procedure GetCurrentSeqNoBigInt(TableNo: Integer): BigInteger
var
CurrSeqNo: BigInteger;
PreviewMode: Boolean;
begin
PreviewMode := IsPreviewMode(); // Only call once to minimize sql calls during preview.
if TryGetCurrentNoBigInt(PreviewMode, TableNo, CurrSeqNo) then
exit(CurrSeqNo);
ClearLastError();
CreateNewTableSequence(PreviewMode, TableNo);
TryGetCurrentNoBigInt(PreviewMode, TableNo, CurrSeqNo);

@github-actions github-actions Bot Aug 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

$\textbf{🟠\ High\ Severity\ —\ Error\ Handling}$

This standalone call ignores the Boolean result of [TryFunction] TryGetCurrentNoBigInt, so a failure after CreateNewTableSequence is handled as an ordinary throwing call instead of try-method logic. Consume the return value and handle false explicitly.

Suggested fix (apply manually — could not be anchored as a one-click suggestion):

if not TryGetCurrentNoBigInt(PreviewMode, TableNo, CurrSeqNo) then
    Error(GetLastErrorText());

Knowledge:

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.35.4

exit(CurrSeqNo);
end;

/// <summary>
/// BigInteger-safe variant of ValidateSeqNo. Ensures the NumberSequence is not behind the last entry in the table.
/// </summary>
/// <param name="TableNo">The ID of the table being checked</param>
procedure ValidateSeqNoBigInt(TableNo: Integer)
var
LastEntryNo: BigInteger;
begin
if IsPreviewMode() then
exit;
if LastSeqNoChecked.Contains(TableNo) then
exit;

LastEntryNo := GetLastEntryNoFromTable(TableNo, false);
if GetCurrentSeqNoBigInt(TableNo) < LastEntryNo then
RebaseSeqNo(TableNo);

LastSeqNoChecked.Add(TableNo);
end;

/// <summary>
/// Ensures that the NumberSequence is not behind the last entry in the table.
/// if the sequence does not exist, it will be created.
Expand Down Expand Up @@ -187,6 +246,18 @@ codeunit 9500 "Sequence No. Mgt."
CurrSeqNo := NumberSequence.Current(GetTableSequenceName(PreviewMode, TableNo));
end;

[TryFunction]
local procedure TryGetNextNoBigInt(PreviewMode: Boolean; TableNo: Integer; var NewSeqNo: BigInteger)
begin
NewSeqNo := NumberSequence.Next(GetTableSequenceName(PreviewMode, TableNo));
end;

[TryFunction]
local procedure TryGetCurrentNoBigInt(PreviewMode: Boolean; TableNo: Integer; var CurrSeqNo: BigInteger)
begin
CurrSeqNo := NumberSequence.Current(GetTableSequenceName(PreviewMode, TableNo));
end;

/// <summary>
/// Restarts or recreates the NumberSequence for the specified Table ID.
/// </summary>
Expand Down
Loading