diff --git a/evaluation/review-fixtures.json b/evaluation/review-fixtures.json index da172ff..9b0acfb 100644 --- a/evaluation/review-fixtures.json +++ b/evaluation/review-fixtures.json @@ -26,6 +26,7 @@ "pictures-must-use-media-not-blob", "report-barcodes-must-use-barcode-module-and-production-font-name", "table-design-must-match-bc-table-type-conventions", + "tablerelation-field-length-must-match-related-field", "transferfields-mirrored-fields-must-match-type-and-length" ] }, @@ -157,7 +158,8 @@ "test-one-when-per-test", "transactionmodel-attribute-governs-test-transactions", "ui-test-codeunit-naming", - "use-assert-isfalse-not-asserterror-for-boolean-checks" + "use-assert-isfalse-not-asserterror-for-boolean-checks", + "recordref-open-temp-parameter-defeats-real-table-checks" ] }, "ui": { diff --git a/microsoft/knowledge/data-modeling/tablerelation-field-length-must-match-related-field.bad.al b/microsoft/knowledge/data-modeling/tablerelation-field-length-must-match-related-field.bad.al new file mode 100644 index 0000000..58d85eb --- /dev/null +++ b/microsoft/knowledge/data-modeling/tablerelation-field-length-must-match-related-field.bad.al @@ -0,0 +1,30 @@ +table 50100 "Sample Category" +{ + fields + { + field(1; "Code"; Code[20]) { } + } + keys + { + key(PK; "Code") { Clustered = true; } + } +} + +table 50102 "Sample Contract" +{ + fields + { + field(1; "Code"; Code[20]) { } + // Code[10] cannot hold every "Sample Category"."Code" value (Code[20]). + // Compiles without a diagnostic; assigning or validating a category code + // longer than 10 characters fails at runtime. + field(2; "Category Code"; Code[10]) + { + TableRelation = "Sample Category"."Code"; + } + } + keys + { + key(PK; "Code") { Clustered = true; } + } +} diff --git a/microsoft/knowledge/data-modeling/tablerelation-field-length-must-match-related-field.good.al b/microsoft/knowledge/data-modeling/tablerelation-field-length-must-match-related-field.good.al new file mode 100644 index 0000000..7f65ff0 --- /dev/null +++ b/microsoft/knowledge/data-modeling/tablerelation-field-length-must-match-related-field.good.al @@ -0,0 +1,65 @@ +enum 50100 "Sample Source Type" +{ + Extensible = true; + + value(0; Category) { } + value(1; Region) { } +} + +table 50100 "Sample Category" +{ + fields + { + field(1; "Code"; Code[20]) { } + } + keys + { + key(PK; "Code") { Clustered = true; } + } +} + +table 50101 "Sample Region" +{ + fields + { + field(1; "Code"; Code[10]) { } + } + keys + { + key(PK; "Code") { Clustered = true; } + } +} + +table 50102 "Sample Contract" +{ + fields + { + field(1; "Code"; Code[20]) { } + // Unconditional relation: same type and exactly the same length as + // "Sample Category"."Code". + field(2; "Category Code"; Code[20]) + { + TableRelation = "Sample Category"."Code"; + } + field(3; "Source Type"; Enum "Sample Source Type") { } + // All branches conditional: at least as long as the longest related + // field (Code[20] covers both Code[20] and Code[10]). + field(4; "Source Code"; Code[20]) + { + TableRelation = if ("Source Type" = const(Category)) "Sample Category"."Code" + else + if ("Source Type" = const(Region)) "Sample Region"."Code"; + } + // Filter field: holds a filter expression, not one code, so it is + // deliberately longer and opts out of relation validation. + field(5; "Category Filter"; Code[250]) + { + TableRelation = "Sample Category"."Code"; + ValidateTableRelation = false; + } + } + keys + { + key(PK; "Code") { Clustered = true; } + } +} diff --git a/microsoft/knowledge/data-modeling/tablerelation-field-length-must-match-related-field.md b/microsoft/knowledge/data-modeling/tablerelation-field-length-must-match-related-field.md new file mode 100644 index 0000000..654c052 --- /dev/null +++ b/microsoft/knowledge/data-modeling/tablerelation-field-length-must-match-related-field.md @@ -0,0 +1,61 @@ +--- +bc-version: [all] +domain: data-modeling +keywords: [tablerelation, field-length, code, text, conditional-relation, lookup] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# A TableRelation field's length must match the related field + +## Description + +`TableRelation` points a field at another table, or at a specific field of it with `Table.Field`. The relation is also used to validate entries and to drive the lookup. A referencing field that is shorter than the related field compiles without a length diagnostic: verified with AL compiler 30.0 and the CodeCop, UICop and PerTenantExtensionCop analyzers. Compare `AL0685`, which does warn about an analogous length mismatch for a FlowField's `CalcFormula` target. + +A shorter field compiles cleanly and works for every value that happens to fit. It fails only when a real related value is longer than the field. For example, a `Code[10]` field related to a table with a `Code[20]` key fails when a 15-character code is assigned or validated into it. The runtime error reads "The length of the string is N, but it must be less than or equal to M characters." + +The length Business Central's own relation check expects depends on whether the field has an unconditional relation: + +- **At least one unconditional relation**, either a plain `TableRelation = X` or an unconditional branch: the field must have the **exact** length of the longest related field and the same type. +- **Only conditional relations** (`if (...) X else if (...) Y`): the field must be **at least** as long as the longest related field. A longer field is accepted. + +For the type, the check requires the related field's type, or `Text` when the related fields mix `Code` and `Text`. Only under all-conditional relations, and only when the required type is `Code`, may the field be `Text` instead. + +The check skips any field that sets `ValidateTableRelation = false` or `TestTableRelation = false`. The Base Application sets `ValidateTableRelation = false` on filter and totaling fields, which hold a filter expression rather than one key value. Such a field may be longer than the related field. A field that is shorter than the related field is still a defect even with either property set to `false`, because a real related value still overflows it at runtime. + +The check is codeunit 134926 "Table Relation Test" in the BC test app. Its validation test is `[Scope('OnPrem')]`, so it runs only on an on-premises test surface. It is not a compile-time guarantee. See [`table-relation-test-exclude-known-invalid-relations-via-event.md`](../testing/table-relation-test-exclude-known-invalid-relations-via-event.md) for how that check evaluates relations and how to exclude a known exception. + +## Best Practice + +Before you add or change a `TableRelation`, read the declared type and length of every related field from its table definition. Lengths vary from table to table, so don't assume a typical `Code[10]` or `Code[20]`. Then size the field: + +- For an unconditional relation, give it the same type and exact length as the related field, unless the field holds a filter expression and sets `ValidateTableRelation = false`. +- For an all-conditional relation, make it at least as long as the longest branch target. + +When a `tableextension` adds a branch with `modify(...)`, check the new target's length against the field's existing declaration too. + +See sample: [`tablerelation-field-length-must-match-related-field.good.al`](tablerelation-field-length-must-match-related-field.good.al). + +## Anti Pattern + +A field whose declared `Code`/`Text` length is shorter than the field it relates to. One example is a `Code[10]` field with `TableRelation` to a table keyed on `Code[20]`. Another is a conditional relation where one branch targets a longer field than the declaration. Both compile without a diagnostic and fail at runtime once a real, longer value is used. This applies whether or not `ValidateTableRelation` or `TestTableRelation` is `false`. The Base Application has an instance: "Financial Report Schedule"."Excel Template Code" is `Code[20]` and relates, with a `where` filter, to "Fin. Report Excel Template".Code, which is `Code[50]`. + +A field that is longer than its related field under an unconditional relation, with neither `ValidateTableRelation` nor `TestTableRelation` set to `false`, is a lesser deviation. It holds every valid value, but it is rejected by codeunit 134926's check and accepts values that can never satisfy the relation. + +See sample: [`tablerelation-field-length-must-match-related-field.bad.al`](tablerelation-field-length-must-match-related-field.bad.al). + +Related: [`transferfields-mirrored-fields-must-match-type-and-length.md`](transferfields-mirrored-fields-must-match-type-and-length.md) covers the same length-mismatch failure between fields that `TransferFields` connects. + +## Source + +- [TableRelation property](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/properties/devenv-tablerelation-property): syntax `[.]`, conditional `IF ... ELSE` relations, and use of the relation to validate entries. +- [Compiler Warning (future error) AL0685](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/diagnostics/diagnostic-al685): the analogous FlowField length diagnostic, which warns that the mismatch "could result in a runtime error". +- BCApps [`src/Layers/W1/Tests/Misc/TableRelationTest.Codeunit.al`](https://github.com/microsoft/BCApps/blob/main/src/Layers/W1/Tests/Misc/TableRelationTest.Codeunit.al): line 46 says "Fields must have the exact length of the largest field they relate to". Lines 48-49 skip a field unless both "Test Table Relation" and "Validate Table Relation" are true. Lines 68-84 apply `Field.Len < MaxRelatedFieldLength` when every relation is conditional and `Field.Len <> MaxRelatedFieldLength` otherwise. Line 15 is `[Scope('OnPrem')]`. +- [ValidateTableRelation property](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/properties/devenv-validatetablerelation-property) and [TestTableRelation property](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/properties/devenv-testtablerelation-property): both default to `true`, and setting `ValidateTableRelation` to `false` should be paired with `TestTableRelation = false`. +- Deliberately longer filter fields with `ValidateTableRelation = false` in BCApps W1 Base Application: + - [`src/Layers/W1/BaseApp/Warehouse/Request/WarehouseSourceFilter.Table.al`](https://github.com/microsoft/BCApps/blob/main/src/Layers/W1/BaseApp/Warehouse/Request/WarehouseSourceFilter.Table.al) lines 44-49: "Variant Code Filter" `Code[100]` relates to "Item Variant".Code (`Code[10]`). + - [`src/Layers/W1/BaseApp/Finance/Analysis/AnalysisViewFilter.Table.al`](https://github.com/microsoft/BCApps/blob/main/src/Layers/W1/BaseApp/Finance/Analysis/AnalysisViewFilter.Table.al) lines 49-54: "Dimension Value Filter" `Code[250]` relates to "Dimension Value".Code (`Code[20]`). + - [`src/Layers/W1/BaseApp/Projects/Project/Job/JobTask.Table.al`](https://github.com/microsoft/BCApps/blob/main/src/Layers/W1/BaseApp/Projects/Project/Job/JobTask.Table.al) lines 290-295: Totaling `Text[250]` relates to "Job Task"."Job Task No." (`Code[20]`). +- BCApps [`src/Layers/W1/BaseApp/Finance/FinancialReports/FinancialReportSchedule.Table.al`](https://github.com/microsoft/BCApps/blob/main/src/Layers/W1/BaseApp/Finance/FinancialReports/FinancialReportSchedule.Table.al) lines 48-51: "Excel Template Code" `Code[20]` relates to [`FinReportExcelTemplate.Table.al`](https://github.com/microsoft/BCApps/blob/main/src/Layers/W1/BaseApp/Finance/FinancialReports/FinReportExcelTemplate.Table.al) field Code (`Code[50]`, line 33), an instance of the shorter-field anti-pattern. +- BCApps [`src/Apps/W1/Subcontracting/Test/Tests/SubcCommentsAttachmentTest.Codeunit.al`](https://github.com/microsoft/BCApps/blob/main/src/Apps/W1/Subcontracting/Test/Tests/SubcCommentsAttachmentTest.Codeunit.al) line 295 asserts the runtime text "The length of the string is 101". diff --git a/microsoft/knowledge/testing/recordref-open-temp-parameter-defeats-real-table-checks.bad.al b/microsoft/knowledge/testing/recordref-open-temp-parameter-defeats-real-table-checks.bad.al new file mode 100644 index 0000000..5d8b844 --- /dev/null +++ b/microsoft/knowledge/testing/recordref-open-temp-parameter-defeats-real-table-checks.bad.al @@ -0,0 +1,22 @@ +// Test-library-style helper (uses "Library - Utility"); not a Subtype = Test codeunit. +codeunit 50150 "Sample Payment Terms Codes" +{ + procedure GenerateUnusedPaymentTermsCode(): Code[10] + var + PaymentTerms: Record "Payment Terms"; + LibraryUtility: Codeunit "Library - Utility"; + RecRef: RecordRef; + FieldRef: FieldRef; + NewCode: Code[10]; + begin + // Temp = true: RecRef is an empty temporary instance, so IsEmpty() + // is true on the first pass and existing Payment Terms are never seen. + RecRef.Open(Database::"Payment Terms", true, CompanyName()); + FieldRef := RecRef.Field(PaymentTerms.FieldNo(Code)); + repeat + NewCode := CopyStr(LibraryUtility.GenerateRandomXMLText(MaxStrLen(NewCode)), 1, MaxStrLen(NewCode)); + FieldRef.SetRange(NewCode); + until RecRef.IsEmpty(); + exit(NewCode); + end; +} diff --git a/microsoft/knowledge/testing/recordref-open-temp-parameter-defeats-real-table-checks.good.al b/microsoft/knowledge/testing/recordref-open-temp-parameter-defeats-real-table-checks.good.al new file mode 100644 index 0000000..ae1a955 --- /dev/null +++ b/microsoft/knowledge/testing/recordref-open-temp-parameter-defeats-real-table-checks.good.al @@ -0,0 +1,34 @@ +// Test-library-style helper (uses "Library - Utility"); not a Subtype = Test codeunit. +codeunit 50150 "Sample Payment Terms Codes" +{ + procedure GenerateUnusedPaymentTermsCode(): Code[10] + var + PaymentTerms: Record "Payment Terms"; + LibraryUtility: Codeunit "Library - Utility"; + RecRef: RecordRef; + FieldRef: FieldRef; + NewCode: Code[10]; + begin + // Temp = false: the loop checks the persisted Payment Terms rows. + RecRef.Open(Database::"Payment Terms", false, CompanyName()); + FieldRef := RecRef.Field(PaymentTerms.FieldNo(Code)); + repeat + NewCode := CopyStr(LibraryUtility.GenerateRandomXMLText(MaxStrLen(NewCode)), 1, MaxStrLen(NewCode)); + FieldRef.SetRange(NewCode); + until RecRef.IsEmpty(); + exit(NewCode); + end; + + procedure GetFieldFilterFromView(TableNo: Integer; TableView: Text; FieldNo: Integer): Text + var + RecRef: RecordRef; + FieldRef: FieldRef; + begin + // Temp = true is correct here: the RecordRef only parses a view and + // never reads rows. + RecRef.Open(TableNo, true); + RecRef.SetView(TableView); + FieldRef := RecRef.Field(FieldNo); + exit(FieldRef.GetFilter()); + end; +} diff --git a/microsoft/knowledge/testing/recordref-open-temp-parameter-defeats-real-table-checks.md b/microsoft/knowledge/testing/recordref-open-temp-parameter-defeats-real-table-checks.md new file mode 100644 index 0000000..70d7c74 --- /dev/null +++ b/microsoft/knowledge/testing/recordref-open-temp-parameter-defeats-real-table-checks.md @@ -0,0 +1,45 @@ +--- +bc-version: [all] +domain: testing +keywords: [recordref, open, temporary, isempty, existence-check, uniqueness] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# RecordRef.Open with Temp = true cannot check the real table + +## Description + +`RecordRef.Open(No: Integer [, Temp: Boolean] [, CompanyName: Text])` always takes a real table number. When `Temp` is `true`, though, the `RecordRef` refers to a temporary instance of that table. That instance starts empty and never contains the persisted rows. Microsoft Learn's example opens table 27 temporarily and notes that `Find('-')` "returns false" because "there are no records in a temporary table". So `IsEmpty`, `Find*`, `Next`, `Get` or `Count` on a temp-opened `RecordRef` only sees rows the same code inserted into it. These calls can't answer "does this value already exist in the table?" + +The defect is easy to miss at the call site. `Temp` is a positional, unnamed Boolean, the table number is genuine, and the code compiles without a diagnostic. The usual shape is a generate-until-unused loop: the code sets a range on a field and repeats `until RecRef.IsEmpty()`. That loop exits on its first pass and returns a value that may already exist. The defect then shows up later as a duplicate-key error or a wrong lookup, often only once the table holds data. + +The shape occurs wherever `RecordRef` is used. This article sits in the testing domain because generic test-library helpers, which work on any table number, are where it typically appears. For how specific `LibraryUtility` helpers behave, including `GenerateRandomCode`'s temporary open, see [`use-generateguid-for-unique-test-fixture-values.md`](use-generateguid-for-unique-test-fixture-values.md). That article owns guidance on which helper to call. + +## Best Practice + +When the code that follows has to see persisted rows, open with `Temp = false` or omit the parameter. Learn's first example omits it so the table "will not be open as temporary table". + +`Temp = true` is correct when the `RecordRef` is deliberately a scratch instance that never answers existence questions about the real table, for example: + +- A field-validation sandbox: insert a temporary row, then `Validate` a field on it. +- Parsing a table view with `SetView` and reading `GetFilter`. +- Reading table metadata such as `SystemIdNo`. + +See sample: [`recordref-open-temp-parameter-defeats-real-table-checks.good.al`](recordref-open-temp-parameter-defeats-real-table-checks.good.al). + +## Anti Pattern + +`RecRef.Open(, true[, ...])` followed, on the same `RecordRef` and with no `Insert` into it, by `IsEmpty`, `Find`, `FindFirst`, `FindLast`, `FindSet`, `Next`, `Get` or `Count` whose result decides whether a value already exists in the table. The classic case is a uniqueness loop ending `until RecRef.IsEmpty()`. + +See sample: [`recordref-open-temp-parameter-defeats-real-table-checks.bad.al`](recordref-open-temp-parameter-defeats-real-table-checks.bad.al). + +## Source + +- [RecordRef.Open method](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/methods-auto/recordref/recordref-open-method): the syntax, Example 1 (parameters omitted, not temporary) and Example 2 (temporary open is empty, `Find('-')` returns false). +- BCApps [`src/Layers/W1/Tests/ApplicationTestLibrary/LibraryUtility.Codeunit.al`](https://github.com/microsoft/BCApps/blob/main/src/Layers/W1/Tests/ApplicationTestLibrary/LibraryUtility.Codeunit.al): `GenerateRandomCode` opens with `true` at line 288 and loops `until RecRef.IsEmpty()`. `GenerateRandomCodeWithLength` opens with `false` at line 309. +- Legitimate `Temp = true` uses in BCApps W1: + - [`src/Layers/W1/BaseApp/Inventory/Item/ItemTempl.Table.al`](https://github.com/microsoft/BCApps/blob/main/src/Layers/W1/BaseApp/Inventory/Item/ItemTempl.Table.al) line 1255: temporary `Item` insert plus `Validate`. + - [`src/Apps/W1/Quality Management/app/src/Utilities/QltyFilterHelpers.Codeunit.al`](https://github.com/microsoft/BCApps/blob/main/src/Apps/W1/Quality%20Management/app/src/Utilities/QltyFilterHelpers.Codeunit.al) line 942: `SetView` plus `GetFilter`. + - [`src/Layers/W1/BaseApp/Integration/SynchEngine/IntegrationRecordSynch.Codeunit.al`](https://github.com/microsoft/BCApps/blob/main/src/Layers/W1/BaseApp/Integration/SynchEngine/IntegrationRecordSynch.Codeunit.al) line 221: `SplitLocalTableFilter` splits a table filter, reading only `SystemIdNo` from the temporary open. diff --git a/microsoft/skills/review/al-data-modeling-review.md b/microsoft/skills/review/al-data-modeling-review.md index 789db91..55efe44 100644 --- a/microsoft/skills/review/al-data-modeling-review.md +++ b/microsoft/skills/review/al-data-modeling-review.md @@ -16,7 +16,7 @@ application-area: [all] Reviews AL source changes against the `data-modeling` knowledge domain in BCQuality and emits a findings report. This is a leaf action skill: it invokes no sub-skills. It is one of the skills composed by `al-code-review`. -An orchestrator invokes this skill with a `pr-diff`, `file-path`, or `folder-path`. Data-modeling findings are narrow by design — they apply when the review scope contains setup or master tables, their card pages, primary keys, number-series assignment, block enforcement, audit fields, document print/email/Post-and-Send actions, `Navigate` page subscribers, Report Selection registration or dispatch, price-calculation/price-source extensibility, code that reads or writes sales/purchase/service line price and amount fields, `TransferFields`-based posting-cascade field mirroring, barcode/report-layout font-provider usage, dimension wiring, journal-based posting-routine structure, or Item Ledger Entry document-number lookups after a combined sales post. The skill returns `not-applicable` when none of those apply. +An orchestrator invokes this skill with a `pr-diff`, `file-path`, or `folder-path`. Data-modeling findings are narrow by design — they apply when the review scope contains setup or master tables, their card pages, primary keys, number-series assignment, block enforcement, audit fields, document print/email/Post-and-Send actions, `Navigate` page subscribers, Report Selection registration or dispatch, price-calculation/price-source extensibility, code that reads or writes sales/purchase/service line price and amount fields, `TransferFields`-based posting-cascade field mirroring, `TableRelation` field-length design, barcode/report-layout font-provider usage, dimension wiring, journal-based posting-routine structure, or Item Ledger Entry document-number lookups after a combined sales post. The skill returns `not-applicable` when none of those apply. ## Source @@ -39,7 +39,7 @@ Narrow the relevant files to the subset that applies to the changes under review - The changed AL object names and types — especially `* Setup` singleton tables and Card pages, custom master tables, tableextensions that add master-data fields, document or journal lines that reference a master, document pages/codeunits exposing print/email/Post-and-Send actions, codeunits subscribing to `Navigate`, enumextensions to `"Report Selection Usage"`/`"Price Calculation Handler"`/`"Price Source Type"`, and report objects that render barcodes. - The changed fields, keys, triggers, and procedures, weighted toward `Primary Key`, `No.`, `No. Series`, `Blocked`, `Last Date Modified`, `OnInsert`, `OnModify`, `OnRename`, reference-field `OnValidate`, posting validation, and posting-cascade `TransferFields` calls. -- Tokens extracted from the diff that relate to data modeling (`setup`, `master`, `Primary Key`, `Code[10]`, `Code[20]`, `AutoIncrement`, `SystemId`, `No.`, `No. Series`, `NoSeriesManagement`, `Codeunit "No. Series"`, `GetNextNo`, `IsManual`, `TestManual`, `Blocked`, `TestField`, `Last Date Modified`, `Today`, `WorkDate`, `InsertAllowed`, `DeleteAllowed`, `PageType = Card`, `OnOpenPage`, `GetRecordOnce`, `OnInsert`, `OnModify`, `OnRename`, `InitRecord`, `Round`, `Precision`, `Direction`, `TableRelation`, `tableextension`, `enumextension`, `Media`, `MediaSet`, `Item`, `Count`, `TransferFields`, `Navigate`, `OnAfterFindRecords`, `OnBeforeShowRecords`, `Report Selections`, `Report Selection Usage`, `InsertRecord`, `Document Sending Profile`, `PrintForCust`, `PrintWithDialogForCust`, `PrintWithDialogForVend`, `SendEmailToCust`, `SendEmailToVendor`, `Report.RunModal`, `Report.Run`, `Price Calculation Handler`, `Price Calculation`, `OnFindSupportedSetup`, `Price Calculation Setup`, `Price Source Type`, `PriceSourceList`, `OnAfterAddSources`, `UpdateUnitPrice`, `PlanPriceCalcByField`, `UpdateUnitPriceByField`, `Prices Including VAT`, `Unit Price`, `Direct Unit Cost`, `Line Amount`, `Prepmt. Line Amount`, `Amount Including VAT`, `CalculateOutstandingAmountExclTax`, `Barcode Font Provider`, `Barcode Font Provider 2D`, `EncodeFont`, `ValidateInput`). +- Tokens extracted from the diff that relate to data modeling (`setup`, `master`, `Primary Key`, `Code[10]`, `Code[20]`, `AutoIncrement`, `SystemId`, `No.`, `No. Series`, `NoSeriesManagement`, `Codeunit "No. Series"`, `GetNextNo`, `IsManual`, `TestManual`, `Blocked`, `TestField`, `Last Date Modified`, `Today`, `WorkDate`, `InsertAllowed`, `DeleteAllowed`, `PageType = Card`, `OnOpenPage`, `GetRecordOnce`, `OnInsert`, `OnModify`, `OnRename`, `InitRecord`, `Round`, `Precision`, `Direction`, `TableRelation`, `ValidateTableRelation`, `TestTableRelation`, `Text[`, `tableextension`, `enumextension`, `Media`, `MediaSet`, `Item`, `Count`, `TransferFields`, `Navigate`, `OnAfterFindRecords`, `OnBeforeShowRecords`, `Report Selections`, `Report Selection Usage`, `InsertRecord`, `Document Sending Profile`, `PrintForCust`, `PrintWithDialogForCust`, `PrintWithDialogForVend`, `SendEmailToCust`, `SendEmailToVendor`, `Report.RunModal`, `Report.Run`, `Price Calculation Handler`, `Price Calculation`, `OnFindSupportedSetup`, `Price Calculation Setup`, `Price Source Type`, `PriceSourceList`, `OnAfterAddSources`, `UpdateUnitPrice`, `PlanPriceCalcByField`, `UpdateUnitPriceByField`, `Prices Including VAT`, `Unit Price`, `Direct Unit Cost`, `Line Amount`, `Prepmt. Line Amount`, `Amount Including VAT`, `CalculateOutstandingAmountExclTax`, `Barcode Font Provider`, `Barcode Font Provider 2D`, `EncodeFont`, `ValidateInput`). A file enters the candidate worklist when its `keywords` intersect the extracted tokens or its topic (derived from the index entry's `path`, `title`, and `description`) matches a changed object type. Read an article's full file — its `## Best Practice` / `## Anti Pattern` bodies — only after it makes the worklist; candidate selection uses the index alone. When the diff contains no data-modeling changes by any of the above signals, return `outcome: "not-applicable"` without evaluating files. @@ -55,6 +55,7 @@ The following targeted checks cover every current `data-modeling` article. Treat - A master gains or changes `Blocked`, or a document line, journal line, reference-field `OnValidate`, or posting routine uses that master without `TestField(Blocked, false)` at the point of use; also cue when the check is placed only in the master's own triggers — `check-blocked-in-referencing-code-not-in-master`. - A master table adds or changes `Last Date Modified`, `OnModify`, or `OnRename`, but the non-editable field is not assigned `Today()` in both triggers — `set-last-date-modified-in-onmodify-and-onrename`. - A `tableextension` appends a conditional `TableRelation` as if it overrides an earlier unconditional relation, or relation branches are otherwise designed without accounting for additive top-down evaluation — `table-relation-extensions-are-additive-and-top-down`. +- A field declares a `TableRelation` (or a `tableextension` `modify` adds a relation branch) and the field's declared `Code`/`Text` length is shorter than the related field, or, when any relation on the field is unconditional, differs from it — `tablerelation-field-length-must-match-related-field`. Read the related field's declared length from its table definition rather than assuming one. A field shorter than the related field is a finding in all cases, including when it sets `ValidateTableRelation = false` or `TestTableRelation = false`. Do not flag a field for being longer than its related field when its relations are all conditional, or when it sets `ValidateTableRelation = false` or `TestTableRelation = false` (filter/totaling fields). - A `Media` or `MediaSet` field is assigned directly between different table types or different field IDs instead of registering each shared item with `MediaSet.Insert` — `share-mediaset-items-with-insert-not-field-assignment`. - A custom document header assigns defaults outside an `InitRecord` boundary, calls `InitRecord` before assigning its number, or places UI-independent defaults only in a page trigger — `initialize-document-defaults-in-initrecord`. - Directed `Round` calls use `'<'` as mathematical floor or `'>'` as mathematical ceiling, especially where negative amounts are possible — `round-direction-symbols-use-magnitude`. @@ -101,7 +102,7 @@ Outcome selection: - `completed` — the skill evaluated every worklist item. - `no-knowledge` — no applicable data-modeling knowledge survived filtering. -- `not-applicable` — the diff touches no setup/master table, page, key, numbering, block-check, or audit-field surface, and no document print/email/Post-and-Send action, `Navigate` subscriber, Report Selection registration/dispatch, price-calculation/price-source extensibility point, sales/purchase/service line price or amount read/write, posting-cascade `TransferFields` mirroring, barcode/report-font-provider usage, dimension wiring, posting-routine structure, or Item-Ledger-Entry-document-number surface. +- `not-applicable` — the diff touches no setup/master table, page, key, numbering, block-check, or audit-field surface, and no document print/email/Post-and-Send action, `Navigate` subscriber, Report Selection registration/dispatch, price-calculation/price-source extensibility point, sales/purchase/service line price or amount read/write, posting-cascade `TransferFields` mirroring, `TableRelation` field length, barcode/report-font-provider usage, dimension wiring, posting-routine structure, or Item-Ledger-Entry-document-number surface. - `partial` — a budget was hit before the worklist was exhausted. - `failed` — an unrecoverable error occurred. diff --git a/microsoft/skills/review/al-testing-review.md b/microsoft/skills/review/al-testing-review.md index c42fe85..7e05c70 100644 --- a/microsoft/skills/review/al-testing-review.md +++ b/microsoft/skills/review/al-testing-review.md @@ -16,7 +16,7 @@ application-area: [all] Reviews AL source changes against the `testing` knowledge domain in BCQuality and emits a findings report. This is a leaf action skill: it invokes no sub-skills. It is one of the skills composed by `al-code-review`. -An orchestrator invokes this skill with a `pr-diff`, `file-path`, or `folder-path`. Testing findings are narrow by design — they apply when the review scope contains test codeunits, test runners, test methods, handlers, assertions, or fixture construction. The skill returns `not-applicable` when none of those apply. +An orchestrator invokes this skill with a `pr-diff`, `file-path`, or `folder-path`. Testing findings are narrow by design — they apply when the review scope contains test codeunits, test runners, test methods, handlers, assertions, fixture construction, or test-library helpers such as `RecordRef`-based uniqueness checks. The skill returns `not-applicable` when none of those apply. ## Source @@ -39,7 +39,7 @@ Narrow the relevant files to the subset that applies to the changes under review - The changed AL object names and types — especially codeunits with `Subtype = Test`, test runner codeunits with `TestIsolation`, test libraries, and codeunits that define UI handlers. - The changed methods and attributes, weighted toward `[Test]`, `[TransactionModel(...)]`, `[TestPermissions(...)]`, `[HandlerFunctions(...)]`, handler attributes, `asserterror`, `ExpectedError`, `ExpectedErrorCode`, fixture initialization, and test-library calls. -- Tokens extracted from the diff that relate to testing (`Subtype = Test`, `Subtype = TestRunner`, `TestIsolation`, `TestPermissions`, `Restrictive`, `NonRestrictive`, `Disabled`, `Permissions Mock`, `Library - Lower Permissions`, `TransactionModel`, `AutoRollback`, `AutoCommit`, `Commit`, `asserterror`, `ExpectedError`, `ExpectedErrorCode`, `HandlerFunctions`, `ConfirmHandler`, `MessageHandler`, `StrMenuHandler`, `ModalPageHandler`, `SendNotificationHandler`, `RecallNotificationHandler`, `Enqueue`, `Dequeue`, `AssertEmpty`, `Initialize`, `IsInitialized`, `OnTestInitialize`, `LibrarySetupStorage`, `Library Assert`, `LibraryVariableStorage`, `LibrarySales`, `LibraryPurchase`, `LibraryERM`, `LibraryInventory`, `LibraryRandom`, `Library - Utility`, `LibraryUtility`, `GenerateGUID`, `GenerateRandomCode`, `TestPage`, `.Visible(`, `.Enabled(`, `.Editable(`, `OpenNew`, `OpenView`, `OpenEdit`, `Init`, `Insert`). +- Tokens extracted from the diff that relate to testing (`Subtype = Test`, `Subtype = TestRunner`, `TestIsolation`, `TestPermissions`, `Restrictive`, `NonRestrictive`, `Disabled`, `Permissions Mock`, `Library - Lower Permissions`, `TransactionModel`, `AutoRollback`, `AutoCommit`, `Commit`, `asserterror`, `ExpectedError`, `ExpectedErrorCode`, `HandlerFunctions`, `ConfirmHandler`, `MessageHandler`, `StrMenuHandler`, `ModalPageHandler`, `SendNotificationHandler`, `RecallNotificationHandler`, `Enqueue`, `Dequeue`, `AssertEmpty`, `Initialize`, `IsInitialized`, `OnTestInitialize`, `LibrarySetupStorage`, `Library Assert`, `LibraryVariableStorage`, `LibrarySales`, `LibraryPurchase`, `LibraryERM`, `LibraryInventory`, `LibraryRandom`, `Library - Utility`, `LibraryUtility`, `GenerateGUID`, `GenerateRandomCode`, `RecordRef`, `RecRef.Open`, `IsEmpty`, `TestPage`, `.Visible(`, `.Enabled(`, `.Editable(`, `OpenNew`, `OpenView`, `OpenEdit`, `Init`, `Insert`). A file enters the candidate worklist when its `keywords` intersect the extracted tokens or its topic (derived from the index entry's `path`, `title`, and `description`) matches a changed object type. Read an article's full file — its `## Best Practice` / `## Anti Pattern` bodies — only after it makes the worklist; candidate selection uses the index alone. When the diff contains no testing-related changes by any of the above signals, return `outcome: "not-applicable"` without evaluating files. @@ -60,6 +60,7 @@ The following targeted checks cover every current `testing` article. Treat each - A shared/lazy `Initialize()`-style fixture helper creates fixture data without a following `Commit()`, in a test method whose body later forces its own rollback (for example `asserterror Error(...)` used for end-of-test cleanup) — `commit-shared-test-fixture-inside-lazy-initialize`. The presence of `Commit()` after the fixture is the compliant shape, not the signal to look for; the missing-`Commit()` shape combined with a later deliberate rollback is the anti-pattern. Require runner/repository context for the `TestIsolation` value: a standalone test file cannot prove which runner executes it, and under `Function`-level isolation this whole pattern is moot regardless of `Commit()` — do not raise the finding when the executing runner's `TestIsolation` is known to be `Function`. - Changed code subscribes to `OnAfterRemoveTableRelation`, calls `RemoveTableRelation`, or references `Codeunit "Table Relation Test"`/134926 — `table-relation-test-exclude-known-invalid-relations-via-event`. - Test fixture code assigns a hardcoded literal to a primary-key field or a field the test relies on as a unique lookup identifier, hand-builds a "unique" value for such a field (string concatenation, a counter, `Format(CurrentDateTime)`), or truncates `LibraryUtility.GenerateGUID()`'s result with `CopyStr` for such a field shorter than 10 characters — `use-generateguid-for-unique-test-fixture-values`. Calling `GenerateGUID()` untruncated into a full-length field, `GenerateRandomCodeWithLength` for a shorter field needing real verified uniqueness, or `GenerateRandomCode20` specifically for a `Code[20]` field, is the compliant shape, not the signal to flag. `GenerateRandomCode20` is not a substitute for `GenerateRandomCodeWithLength` on a shorter field — it truncates `GenerateGUID()`'s sequential value down to the field's length by keeping the *leftmost* characters, which change the slowest, so retries against a short field can churn through the same truncated prefix far longer than `GenerateRandomCodeWithLength`'s equivalent. A hardcoded or deterministic value in an ordinary descriptive field is not this anti-pattern — that field carries no uniqueness constraint. Do not claim `GenerateRandomCode` (without `WithLength`/`20`) or `GenerateRandomXMLText` verify uniqueness against the real table, or that `GenerateRandomCode` is collision-free even within one test run for a short field — none of that is true. +- Changed code calls `RecordRef.Open(
, true[, ...])` with a literal `true` and then, on that same `RecordRef` with no `Insert` into it, uses `IsEmpty`, `Find`, `FindFirst`, `FindLast`, `FindSet`, `Next`, `Get`, or `Count` to decide whether a value already exists in the table (typically a generate-until-unused loop ending `until RecRef.IsEmpty()`) — `recordref-open-temp-parameter-defeats-real-table-checks`. A temporary open used only as a validation sandbox (`Insert` then `Validate`), to parse a view via `SetView`/`GetFilter`, or to read metadata such as `SystemIdNo` is not this anti-pattern; neither is a non-literal `Temp` argument. A call to `LibraryUtility.GenerateRandomCode` itself is owned by `use-generateguid-for-unique-test-fixture-values`, not this cue. - A test asserts against a `TestPage` field's `.Visible()` or `.Enabled()` — `use-testpage-visible-enabled-to-verify-field-ui-state`. When the assertion is against `.Editable()`, or the page is opened with `OpenEdit()` specifically to check editability — `use-testpage-editable-to-verify-field-editability`. - A test path raises UI and `[HandlerFunctions(...)]` does not match the invoked handlers, or the test has no meaningful evidence of the UI result (for example, it treats a Boolean set before the action as proof of success) — `ui-handlers-in-tests`. A capture/reset/assert-after-`RunModal` pattern is valid. Enqueue/dequeue and `AssertEmpty` are required only when order, count, text, replies, or a scripted sequence is part of the contract. Only nonoptional handlers have to execute: a listed handler declared `[SendNotificationHandler(true)]` or `[RecallNotificationHandler(true)]` is optional by design, so do not treat it as unmatched when the run never raises the notification. - A test's `[GIVEN]`/setup looks up a hardcoded code/number/name assumed to already exist instead of creating it, leaves a mandatory field on a created record empty, uses a value that doesn't satisfy the scenario's own explicit length/format requirement (for example a truncation test whose value never exceeds the field), or a scenario-defining value (amount, quantity, percentage, date, threshold, rounding precision) is generated/randomized instead of an explicit chosen value — `test-data-must-be-random-and-complete`. Generating incidental fixture values (identifiers, names, descriptions) via the standard library codeunits is the compliant shape, not the signal to flag, and neither is a short-but-valid value in an otherwise-unremarkable field.