Skip to content
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.

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

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

Open
pri-kise opened this issue Dec 18, 2023 · 6 comments
Labels
question Further information is requested

Comments

@pri-kise
Copy link

Is the following line a false positive?

Error(GetLastErrorText());
@pri-kise
Copy link
Author

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;

@pri-kise pri-kise changed the title LC0048 - False positive? Error(GetLastErrorText()); LC0048 - False positive? Error with GetLastErrorText or Rec."Error Message" Dec 18, 2023
@Arthurvdv
Copy link
Collaborator

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.

@Arthurvdv Arthurvdv added the question Further information is requested label Dec 18, 2023
@pri-kise
Copy link
Author

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
Copy link
Collaborator

Arthurvdv commented Dec 20, 2023

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.

  • Message not shown because the NavBaseException constructor was used without privacy classification
  • The Item doesn't exist. Identification fields and values: <redacted>

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
Copy link

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
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants