mirror of
https://github.com/microsoft/BCQuality.git
synced 2026-10-05 14:46:55 +01:00
Preserve business filters when recommending Record.Get (#205)
Some checks failed
Validate knowledge index / validate-index (push) Has been cancelled
Validate AL review fixtures / validate-review-fixtures (push) Has been cancelled
Validate skill index and report schemas / validate-contract (push) Has been cancelled
Validate frontmatter and structure / validate (push) Has been cancelled
Some checks failed
Validate knowledge index / validate-index (push) Has been cancelled
Validate AL review fixtures / validate-review-fixtures (push) Has been cancelled
Validate skill index and report schemas / validate-contract (push) Has been cancelled
Validate frontmatter and structure / validate (push) Has been cancelled
This commit is contained in:
parent
488ce50775
commit
ac249ba4c9
4 changed files with 44 additions and 5 deletions
|
|
@ -62,6 +62,7 @@
|
||||||
"performance": {
|
"performance": {
|
||||||
"articles": [
|
"articles": [
|
||||||
"use-isempty-for-existence-check",
|
"use-isempty-for-existence-check",
|
||||||
|
"use-get-instead-of-findfirst-on-full-primary-key",
|
||||||
"job-queue-category-code-serializes-conflicting-jobs",
|
"job-queue-category-code-serializes-conflicting-jobs",
|
||||||
"job-queue-external-effects-must-be-idempotent",
|
"job-queue-external-effects-must-be-idempotent",
|
||||||
"job-queue-handlers-must-not-require-ui",
|
"job-queue-handlers-must-not-require-ui",
|
||||||
|
|
|
||||||
|
|
@ -8,4 +8,13 @@ codeunit 50211 "Perf Sample GetByPK Bad"
|
||||||
if Customer.FindFirst() then
|
if Customer.FindFirst() then
|
||||||
Message(Customer.Name);
|
Message(Customer.Name);
|
||||||
end;
|
end;
|
||||||
|
|
||||||
|
procedure ShowUnblockedName(CustomerNo: Code[20])
|
||||||
|
var
|
||||||
|
Customer: Record Customer;
|
||||||
|
begin
|
||||||
|
Customer.SetRange(Blocked, Customer.Blocked::" ");
|
||||||
|
if Customer.Get(CustomerNo) then
|
||||||
|
Message(Customer.Name);
|
||||||
|
end;
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -7,4 +7,25 @@ codeunit 50210 "Perf Sample GetByPK Good"
|
||||||
if Customer.Get(CustomerNo) then
|
if Customer.Get(CustomerNo) then
|
||||||
Message(Customer.Name);
|
Message(Customer.Name);
|
||||||
end;
|
end;
|
||||||
|
|
||||||
|
procedure ShowUnblockedName(CustomerNo: Code[20])
|
||||||
|
var
|
||||||
|
Customer: Record Customer;
|
||||||
|
begin
|
||||||
|
Customer.SetRange("No.", CustomerNo);
|
||||||
|
Customer.SetRange(Blocked, Customer.Blocked::" ");
|
||||||
|
if Customer.FindFirst() then
|
||||||
|
Message(Customer.Name);
|
||||||
|
end;
|
||||||
|
|
||||||
|
procedure ShowUnblockedNameByKey(CustomerNo: Code[20])
|
||||||
|
var
|
||||||
|
Customer: Record Customer;
|
||||||
|
begin
|
||||||
|
if not Customer.Get(CustomerNo) then
|
||||||
|
exit;
|
||||||
|
if Customer.Blocked <> Customer.Blocked::" " then
|
||||||
|
exit;
|
||||||
|
Message(Customer.Name);
|
||||||
|
end;
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -1,26 +1,34 @@
|
||||||
---
|
---
|
||||||
bc-version: [all]
|
bc-version: [all]
|
||||||
domain: performance
|
domain: performance
|
||||||
keywords: [get, findfirst, primary-key, setrange, lookup]
|
keywords: [get, findfirst, primary-key, setrange, lookup, filters, blocked]
|
||||||
technologies: [al]
|
technologies: [al]
|
||||||
countries: [w1]
|
countries: [w1]
|
||||||
application-area: [all]
|
application-area: [all]
|
||||||
---
|
---
|
||||||
|
|
||||||
# Use Get when the full primary key is known; FindFirst is the wrong tool
|
# Use Get for primary-key lookups without losing filter conditions
|
||||||
|
|
||||||
## Description
|
## Description
|
||||||
|
|
||||||
`Get(...)` is the direct primary-key lookup. `FindFirst()` walks an index — even when narrowed by `SetRange` on every primary-key field. The upstream review guidance treats `Customer.SetRange("No.", CustomerNo); if Customer.FindFirst() then ...` as a bad pattern and `if Customer.Get(CustomerNo) then ...` as the correction. The two reach the same record; only `Get` expresses the lookup as a primary-key seek.
|
`Get(...)` retrieves a record by primary key, but ignores normal record filters. Replacing a `FindFirst()` filtered only by the full primary key with `Get` expresses the lookup directly. The same replacement is not equivalent when additional filters enforce business conditions, such as requiring an unblocked customer. Security filters are a separate mechanism: their effect on `Get` depends on Security Filter Mode.
|
||||||
|
|
||||||
## Best Practice
|
## Best Practice
|
||||||
|
|
||||||
When all primary-key fields are available at the call site, call `Get` (or `GetBySystemId`) with them. Reserve `FindFirst` for cases where the filter is on something other than the full primary key — a unique secondary field, a partial composite key, a sort that the caller cares about.
|
When all primary-key fields are available and no additional normal filter constrains the result, call `Get` with them. Reserve `FindFirst` for filtered searches, including partial keys, secondary fields, or additional business conditions.
|
||||||
|
|
||||||
|
Before recommending a replacement, inspect the effective filters at the call site, including filters set by callers or helpers. If additional conditions matter, retain the filtered `FindFirst` or explicitly enforce equivalent conditions after a successful `Get`, before using the record. Do not flag `FindFirst` merely because all primary-key fields are filtered when a non-key filter must also hold. A `SetRange` before `Get` does not enforce that condition.
|
||||||
|
|
||||||
See sample: [`use-get-instead-of-findfirst-on-full-primary-key.good.al`](use-get-instead-of-findfirst-on-full-primary-key.good.al).
|
See sample: [`use-get-instead-of-findfirst-on-full-primary-key.good.al`](use-get-instead-of-findfirst-on-full-primary-key.good.al).
|
||||||
|
|
||||||
## Anti Pattern
|
## Anti Pattern
|
||||||
|
|
||||||
Composing `SetRange` calls that exactly cover the primary key and then calling `FindFirst`. The result is correct but the call site reads as "search the table" rather than "look up by key", which obscures both the intent and the access pattern from later reviewers.
|
Composing `SetRange` calls that cover only the full primary key and then calling `FindFirst` obscures a direct key lookup. Do not extend this finding to a lookup with additional business filters unless the proposed correction preserves them.
|
||||||
|
|
||||||
|
Replacing a filtered lookup with `Get` while assuming a normal filter still excludes records is a correctness defect: a blocked or otherwise ineligible record can pass the lookup. Recommending that replacement without preserving the condition is also an incorrect review finding.
|
||||||
|
|
||||||
See sample: [`use-get-instead-of-findfirst-on-full-primary-key.bad.al`](use-get-instead-of-findfirst-on-full-primary-key.bad.al).
|
See sample: [`use-get-instead-of-findfirst-on-full-primary-key.bad.al`](use-get-instead-of-findfirst-on-full-primary-key.bad.al).
|
||||||
|
|
||||||
|
## References
|
||||||
|
|
||||||
|
- [Record.Get remarks: primary-key lookup and filter semantics](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/methods-auto/record/record-get-method).
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue