Skip to content

Preserve business filters when recommending Record.Get - #205

Open
Stefano Demiliani (demiliani) wants to merge 1 commit into
microsoft:mainfrom
demiliani:fix/get-preserves-business-filters
Open

Stefano Demiliani (demiliani) wants to merge 1 commit into
microsoft:mainfrom
demiliani:fix/get-preserves-business-filters

Conversation

@demiliani

Copy link
Copy Markdown
Contributor

Why This Correction Is Needed

The existing performance article recommends replacing FindFirst with Get when the full primary key is known, but does not explicitly protect additional business filters. That can turn a performance recommendation into a behavior-changing correction.

For example, a lookup filtered by customer number and Blocked = blank only accepts an unblocked customer. Replacing it with Customer.Get(CustomerNo) does not preserve the Blocked condition. Even leaving SetRange(Blocked, ...) before Get does not help: Get ignores normal record filters.

Microsoft documents this behavior in Record.Get remarks. Security filters are explicitly separate: their effect depends on Security Filter Mode and this PR does not describe them as universally bypassed.

What Changes

  • Correct the existing owning article rather than introduce a duplicate rule. Recommend Get for full-primary-key-only lookups, but require preserving additional effective business conditions, including filters established by callers or helpers.
  • Explain two valid alternatives: retain the filtered FindFirst, or explicitly enforce equivalent eligibility conditions after a successful Get and before using the record.
  • Prevent false-positive review advice: FindFirst is not automatically a defect just because all primary-key fields are filtered when another filter must also hold.
  • Extend the existing bad sample with an ignored Blocked filter before Get.
  • Extend the existing clean sample with both a valid filtered FindFirst and a successful Get followed by an explicit Blocked check. Preserve the existing key-only good/bad examples.
  • Add this paired article to the existing performance evaluation override so the modified guidance participates in the positive/clean corpus.

Scope And Admission Rationale

This is one concern: preserving lookup semantics when recommending a primary-key rewrite. It belongs to the Microsoft-owned performance article that already recommends that rewrite. The BC-specific filter behavior can produce an incorrect remediation or a false-positive review; it is not a generic coding-style rule or a compiler diagnostic.

The article remains universally applicable (bc-version: [all]); the cited Get method is documented from runtime 1.0. No new domain, skill, report schema or protocol is introduced. The composition-validator change is submitted independently in #204.

Validation And Limits

Passed against the knowledge worktree:

  • tools/Test-ReviewFixtures.ps1 (218 cases; 109/330 paired articles across 20 leaf domains, including review-contract checks)
  • .github/scripts/Test-KnowledgeIndex.ps1 and its delegated retrieval tests on a plain temporary copy outside OneDrive (398 articles and 683 samples round-tripped)
  • git diff --check

Editor diagnostics found no errors in the four changed files. Python/PyYAML frontmatter validation was not run locally because no usable Python interpreter was available; it remains for CI.

These are static corpus/index/retrieval checks. The samples are demonstration-only under READ and were not compiled or executed in Business Central. No real-model evaluation was performed, and no claim is made that the new rule has already demonstrated an improvement in model accuracy.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 34424c390cf909a437c80558088f7ef7962da7ed. The guidance accurately reflects AL behavior: Record.Get uses the primary key and ignores ordinary record filters, while FindFirst respects them. The good samples preserve business conditions either by retaining filtered FindFirst or explicitly validating eligibility after Get; the bad sample demonstrates the filter loss. Deterministic retrieval ranks the pair correctly, and no conflict with loop/cache/blocked-master guidance was found. No merge-critical issue remains; the repository validation workflows should still be approved and allowed to run before merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants