StefanMaron / BusinessCentral.LinterCop

Community driven code linter for AL (MS Dynamics 365 Business Central)
https://stefanmaron.com
MIT License
72 stars 29 forks source link

LC0048 - False positive? Error with GetLastErrorText or Rec."Error Message" #446

Open pri-kise opened 8 months ago

pri-kise commented 8 months ago

Is the following line a false positive?

Error(GetLastErrorText());
pri-kise commented 8 months ago

I've got another one where I'm not sure if this is a false positive or if I'd need a pragma

//Note Method is called from an external Service, but we'd like to protocol the Error on the Record, but return the error to the external service.
procedure DoMagic(EntryNo: Integer)
var
   PTEQueueEntry: Record "PTE Queue Entry"
begin
    PTEQueueEntry.Get(EntryNo);
    PTEQueueEntry.Process();
    Commit();
    if PTEQueueEntry.Get(PTEQueueEntry."Entry No.") and (PTEQueueEntry."Error Message" <> '') then begin
        Error(PTEQueueEntry."Error Message"); // Is this a valid warning for LC00048 or do we need a pragma here?
    end;
end;
Arthurvdv commented 8 months ago

I'll have to check, but I'm quite certain these will show up in the telemetry as Use ERROR with a text constant to improve telemetry details.

So i think this is not a false positive, where the rule focuses on having details in the telemetry. To have more details in the telemetry you need to implement the ErrorInfo and optionally determine the DataClassification of the Error message.

procedure MyGetLastErrorText()
var
    MyErrorInfo: ErrorInfo;
begin
    MyErrorInfo := ErrorInfo.Create(GetLastErrorText());
    MyErrorInfo.DataClassification(DataClassification::SystemMetadata);
    Error(MyErrorInfo);
end;
procedure DoMagic(EntryNo: Integer)
var
   PTEQueueEntry: Record "PTE Queue Entry";
   MyErrorInfo: ErrorInfo;
begin
    PTEQueueEntry.Get(EntryNo);
    PTEQueueEntry.Process();
    Commit();
    if PTEQueueEntry.Get(PTEQueueEntry."Entry No.") and (PTEQueueEntry."Error Message" <> '') then begin
        MyErrorInfo := ErrorInfo.Create(PTEQueueEntry."Error Message");
        MyErrorInfo.DataClassification(DataClassification::EndUserPseudonymousIdentifiers);
        Error(MyErrorInfo);
    end;
end;

Do you have access to Telemetry to verify if this in deed the case this? Otherwise I need to setup an environment to verify this, but will take some more time.

pri-kise commented 8 months ago

Sadly this app is currently only used in OnPrem an still under development and we don't have telemetry yet for this purpose.

I will ask in yammer for help on this issue.

I've got another code, that I don't know how to refactor:

field(3; "Dimension Code"; Code[20])
{
    Caption = 'Dimension Code';
    TableRelation = Dimension.Code;

    trigger OnValidate()
    begin
        if not DimMgt.CheckDim("Dimension Code") then
            Error(DimMgt.GetDimErr()); //Will ErrorInfo help here?
        "Dimension Value Code" := '';
    end;
}
Arthurvdv commented 8 months ago

I've created an environment where I could access the application insight to review the possible scenario's.

To have the content of the message of the error itself in the telemetry, you need to wrap the content (label/text/...) in a ErrorInfo object, see the example below;

field(3; "Dimension Code"; Code[20])
{
    Caption = 'Dimension Code';
    TableRelation = Dimension.Code;

    trigger OnValidate()
    begin
        if not DimMgt.CheckDim("Dimension Code") then
            Error(ErrorInfo.Create(DimMgt.GetDimErr()));
        "Dimension Value Code" := '';
    end;
}

Great example by-to-way. I've taken the liberty to use your example in the wiki on the documentation of this rule: https://github.com/StefanMaron/BusinessCentral.LinterCop/wiki/LC0048.

The only thing I'm not sure on howto implement correctly the DataClassification. In the telemetry I find cases like these below, but unsure howto recreate them with the ErrorInfo object.

I've try'ed with every possbile DataClassification setting, where at the moment the complete text is always committed to application insights. I'm unfamiliar at this moment howto handle error messages with sensitive information.

tscottjendev commented 7 months ago

So the implication here is that if I have a function that returns text I need to wrap it inside the ErrorInfo?

What does a Label do differently than a text value from a function?

Arthurvdv commented 7 months ago

I've try'ed to add context about this in the documentation of the rule itself:

When Analyzing Telemetry with the Dynamics 365 Business Central Usage Analytics report, having access to the correct error message is essential for a effective analysis.

https://github.com/StefanMaron/BusinessCentral.LinterCop/wiki/LC0048