microsoft / AL

Home of the Dynamics 365 Business Central AL Language extension for Visual Studio Code. Used to track issues regarding the latest version of the AL compiler and developer tools available in the Visual Studio Code Marketplace or as part of the AL Developer Preview builds for Dynamics 365 Business Central.
MIT License
732 stars 243 forks source link

CommitBehavior only checks explicit commits. Implicit Commits are still performed without throwing errors! #7589

Closed DavidFeldhoff closed 9 months ago

DavidFeldhoff commented 10 months ago

Please include the following with each issue:

1. Describe the bug With [CommitBehavior(CommitBehavior::Error)] or [CommitBehavior(CommitBehavior::Ignore)] we should be able to avoid commits. But unfortunately implicit commits are still executed and therefore you can't trust it right now.

2. To Reproduce Steps to reproduce the behavior:

  1. Execute the current code.
pageextension 50115 CustomerListExt extends "Customer List"
{
    actions
    {
        addfirst(processing)
        {
            action(TestTransaction)
            {
                Caption = 'Test Transaction';
                ApplicationArea = All;
                trigger OnAction()
                var
                    DataInTransaction: Record DataInTransaction;
                begin
                    DataInTransaction.DeleteAll();
                    Commit();

                    StartTransaction();
                end;
            }
        }
    }

    [CommitBehavior(CommitBehavior::Error)]
    local procedure StartTransaction()
    var
        DataInTransaction: Record DataInTransaction;
        StartTransactionWithRollback: Codeunit StartTransactionWithRollback;
        Count: Integer;
    begin
        StartTransactionWithRollback.DoImplicitCommit(false);
        if StartTransactionWithRollback.Run() then;
        Count := DataInTransaction.Count(); // returns 0 as data was rolled back.

        StartTransactionWithRollback.DoImplicitCommit(true);
        if StartTransactionWithRollback.Run() then;
        Count := DataInTransaction.Count(); // returns 1 as data was committed and the Error('') just could roll back to the last commit
    end;
}
codeunit 50116 StartTransactionWithRollback
{
    trigger OnRun()
    var
        InsertData: Codeunit InsertData;
    begin
        if ImplicitCommit then begin
            if InsertData.Run() then;
            //no error was thrown - Data was commited!
        end else begin
            InsertData.Run();
            Commit(); //throws error: Commit is prohibited in the current scope. The operation cannot continue. Contact your system administrator.
        end;
        Error('');
    end;

    var
        ImplicitCommit: Boolean;

    procedure DoImplicitCommit(ImplicitCommit2: Boolean)
    begin
        ImplicitCommit := ImplicitCommit2;
    end;
}
codeunit 50115 InsertData
{
    trigger OnRun()
    var
        DataInTransaction: Record DataInTransaction;
    begin
        DataInTransaction.Code := 'Test';
        DataInTransaction.Insert();
    end;
}
table 50115 DataInTransaction
{
    fields
    {
        field(1; Code; Code[20])
        {
            Caption = 'Test';
        }
    }
}

3. Expected behavior Implicit commits can't be done as well. Otherwise you can't trust the CommitBehavior.

4. Actual behavior Implicit Commits, e.g. when working with the return value of Codeunit.Run, are still performed which is quite scary.

5. Versions:

Final Checklist

Please remember to do the following:

jehelles commented 9 months ago

The CommitBehavior function was designed for the explicit commit, so this is by-design.

If we were to error out on implicit commits, the Codeunit.Run functionality would stop being useful. If the goal is to prevent Codeunit.Run, a LockTable/insert/modify on a table is enough to prevent any usage of Codeunit.Run, as Codeunit.Run requires that no changes are done on the transaction before calling Codeunit.Run.

I will log a bug to document that CommitBehavior only applies to explicit commits.