mirror of
https://github.com/microsoft/BCQuality.git
synced 2026-08-06 17:36:53 +01:00
Seed web-services (API v2) knowledge domain (#45)
* Seed web-services (API v2) knowledge domain Add eight web-services API page knowledge articles (each with .good.al/.bad.al samples), a new al-web-services-review leaf skill, and wire it into al-code-review and the README. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Trim web-services domain to 6 non-duplicative articles Drop API entity-naming/camelCase and DelayedInsert articles (owned by the style domain). Reframe the committed-data and API-versioning articles to stay strictly within the endpoint design/behavior lane, and update the leaf skill's worklist tokens and Output example accordingly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
Jesper Schulz-Wedde
GitHub
parent
6140a52b03
commit
13f47f65a8
21 changed files with 774 additions and 2 deletions
|
|
@ -0,0 +1,38 @@
|
|||
// Intended for read-only consumption, but the CRUD guards are omitted. With
|
||||
// InsertAllowed/ModifyAllowed/DeleteAllowed left at their writable defaults the
|
||||
// endpoint silently accepts POST, PATCH, and DELETE, so a client can mutate or
|
||||
// remove ledger data this API was never meant to expose for writing.
|
||||
page 50357 "WS Read Only Bad"
|
||||
{
|
||||
PageType = API;
|
||||
APIPublisher = 'contoso';
|
||||
APIGroup = 'reporting';
|
||||
APIVersion = 'v1.0';
|
||||
EntityName = 'customerLedgerEntry';
|
||||
EntitySetName = 'customerLedgerEntries';
|
||||
ODataKeyFields = SystemId;
|
||||
SourceTable = "Cust. Ledger Entry";
|
||||
|
||||
layout
|
||||
{
|
||||
area(content)
|
||||
{
|
||||
repeater(records)
|
||||
{
|
||||
field(id; Rec.SystemId)
|
||||
{
|
||||
Caption = 'id';
|
||||
Editable = false;
|
||||
}
|
||||
field(entryNumber; Rec."Entry No.")
|
||||
{
|
||||
Caption = 'entryNumber';
|
||||
}
|
||||
field(postingDate; Rec."Posting Date")
|
||||
{
|
||||
Caption = 'postingDate';
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -0,0 +1,39 @@
|
|||
page 50356 "WS Read Only Good"
|
||||
{
|
||||
PageType = API;
|
||||
Caption = 'customerLedgerEntry';
|
||||
APIPublisher = 'contoso';
|
||||
APIGroup = 'reporting';
|
||||
APIVersion = 'v1.0';
|
||||
EntityName = 'customerLedgerEntry';
|
||||
EntitySetName = 'customerLedgerEntries';
|
||||
ODataKeyFields = SystemId;
|
||||
SourceTable = "Cust. Ledger Entry";
|
||||
Editable = false;
|
||||
InsertAllowed = false;
|
||||
ModifyAllowed = false;
|
||||
DeleteAllowed = false;
|
||||
|
||||
layout
|
||||
{
|
||||
area(content)
|
||||
{
|
||||
repeater(records)
|
||||
{
|
||||
field(id; Rec.SystemId)
|
||||
{
|
||||
Caption = 'id';
|
||||
Editable = false;
|
||||
}
|
||||
field(entryNumber; Rec."Entry No.")
|
||||
{
|
||||
Caption = 'entryNumber';
|
||||
}
|
||||
field(postingDate; Rec."Posting Date")
|
||||
{
|
||||
Caption = 'postingDate';
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: web-services
|
||||
keywords: [api-page, insertallowed, modifyallowed, deleteallowed, editable, read-only, reporting-api]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Lock down write operations on read-only API pages
|
||||
|
||||
## Description
|
||||
|
||||
An API meant purely for reading — a reporting or lookup endpoint — is not read-only just because nobody intends to write to it. Unless the page explicitly forbids writes, the platform leaves the endpoint writable, so a client can POST, PATCH, or DELETE against data that was never meant to change through that surface. The fix is explicit: set `InsertAllowed = false`, `ModifyAllowed = false`, and `DeleteAllowed = false` (and `Editable = false`) so the endpoint rejects every write operation. LLMs often assume "I only exposed read fields, so it's read-only" and rely on defaults; this file is remedial because the default for an API page is writable, and the read-only intent has to be encoded as three explicit property settings, not inferred.
|
||||
|
||||
## Best Practice
|
||||
|
||||
For a read-only / reporting API page set all three CRUD guards off — `InsertAllowed = false`, `ModifyAllowed = false`, `DeleteAllowed = false` — and mark the page `Editable = false`. The endpoint then serves GET requests and rejects any insert, modify, or delete, matching the read-only contract regardless of the caller. Make the read-only stance explicit rather than depending on the writable default.
|
||||
|
||||
See sample: `disable-write-operations-on-read-only-api-pages.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
An API intended for read-only consumption that omits the CRUD guards, leaving `InsertAllowed`, `ModifyAllowed`, and `DeleteAllowed` at their writable defaults. The endpoint silently accepts POST, PATCH, and DELETE, so a client can mutate or remove data the API was never meant to expose for writing. The detection signal: a read-only/reporting `PageType = API` page that does not set the three `*Allowed = false` properties.
|
||||
|
||||
See sample: `disable-write-operations-on-read-only-api-pages.bad.al`.
|
||||
|
|
@ -0,0 +1,38 @@
|
|||
// Committed-only contract, but no isolation level is set. Reads run at the
|
||||
// default and can observe in-flight, uncommitted writes from concurrent
|
||||
// transactions. A consumer may fetch a row that is later rolled back — a dirty
|
||||
// read of data that never durably existed.
|
||||
page 50349 "WS ReadCommitted Bad"
|
||||
{
|
||||
PageType = API;
|
||||
APIPublisher = 'contoso';
|
||||
APIGroup = 'sales';
|
||||
APIVersion = 'v1.0';
|
||||
EntityName = 'customer';
|
||||
EntitySetName = 'customers';
|
||||
ODataKeyFields = SystemId;
|
||||
SourceTable = Customer;
|
||||
Editable = false;
|
||||
InsertAllowed = false;
|
||||
ModifyAllowed = false;
|
||||
DeleteAllowed = false;
|
||||
|
||||
layout
|
||||
{
|
||||
area(content)
|
||||
{
|
||||
repeater(records)
|
||||
{
|
||||
field(id; Rec.SystemId)
|
||||
{
|
||||
Caption = 'id';
|
||||
Editable = false;
|
||||
}
|
||||
field(displayName; Rec.Name)
|
||||
{
|
||||
Caption = 'displayName';
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -0,0 +1,41 @@
|
|||
page 50348 "WS ReadCommitted Good"
|
||||
{
|
||||
PageType = API;
|
||||
Caption = 'customer';
|
||||
APIPublisher = 'contoso';
|
||||
APIGroup = 'sales';
|
||||
APIVersion = 'v1.0';
|
||||
EntityName = 'customer';
|
||||
EntitySetName = 'customers';
|
||||
ODataKeyFields = SystemId;
|
||||
SourceTable = Customer;
|
||||
Editable = false;
|
||||
InsertAllowed = false;
|
||||
ModifyAllowed = false;
|
||||
DeleteAllowed = false;
|
||||
|
||||
layout
|
||||
{
|
||||
area(content)
|
||||
{
|
||||
repeater(records)
|
||||
{
|
||||
field(id; Rec.SystemId)
|
||||
{
|
||||
Caption = 'id';
|
||||
Editable = false;
|
||||
}
|
||||
field(displayName; Rec.Name)
|
||||
{
|
||||
Caption = 'displayName';
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
trigger OnOpenPage()
|
||||
begin
|
||||
// Return only durably committed rows; ignore concurrent uncommitted writes.
|
||||
Rec.ReadIsolation := IsolationLevel::ReadCommitted;
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [22..]
|
||||
domain: web-services
|
||||
keywords: [api-page, readisolation, isolationlevel, readcommitted, onopenpage, dirty-read, committed-data]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Read only committed data from APIs that must not expose in-flight writes
|
||||
|
||||
## Description
|
||||
|
||||
This is about the data-consistency contract of an API endpoint: what a consumer receives when it reads. By default an API read can return in-flight rows that a concurrent, still-open transaction has written but not yet committed. For an endpoint whose contract is "return only data that is durably committed," that is wrong — a consumer could fetch a row that the writing transaction later rolls back, then act on data that never really existed. From runtime 22.0 (BC 2023 release wave 1) an API page can pin the isolation level its reads use: setting `Rec.ReadIsolation := IsolationLevel::ReadCommitted;` in the page's `OnOpenPage` trigger makes the endpoint expose only committed rows. LLMs rarely set this on an API page because the platform default "just works" for ordinary UI; this file is remedial because the committed-only endpoint contract requires an explicit opt-in the model would not add on its own.
|
||||
|
||||
## Best Practice
|
||||
|
||||
For an API page that must expose only committed data, set the endpoint's read isolation once as the page opens: in the `OnOpenPage` trigger write `Rec.ReadIsolation := IsolationLevel::ReadCommitted;`. Every read the endpoint then serves ignores uncommitted writes from concurrent transactions, so a consumer never receives a row that another transaction might still roll back.
|
||||
|
||||
See sample: `expose-only-committed-data-from-api-reads.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
An API intended to return committed-only data that sets no isolation level, leaving reads at the default that can observe in-flight, uncommitted writes. A consumer can fetch a row created by a concurrent transaction that is later rolled back — a dirty read that surfaces data which never durably existed. The detection signal: a committed-only read API with no `Rec.ReadIsolation := IsolationLevel::ReadCommitted` in `OnOpenPage`.
|
||||
|
||||
See sample: `expose-only-committed-data-from-api-reads.bad.al`.
|
||||
|
|
@ -0,0 +1,60 @@
|
|||
// Side effect hidden behind a writable flag: PATCHing "posted" to true silently
|
||||
// triggers posting through OnValidate. The operation is indistinguishable from
|
||||
// an ordinary data edit and is not discoverable as an action. Expose a
|
||||
// [ServiceEnabled] bound action instead.
|
||||
page 50351 "WS Bound Action Bad"
|
||||
{
|
||||
PageType = API;
|
||||
APIPublisher = 'contoso';
|
||||
APIGroup = 'sales';
|
||||
APIVersion = 'v1.0';
|
||||
EntityName = 'salesOrder';
|
||||
EntitySetName = 'salesOrders';
|
||||
ODataKeyFields = SystemId;
|
||||
SourceTable = "Sales Header";
|
||||
DelayedInsert = true;
|
||||
|
||||
layout
|
||||
{
|
||||
area(content)
|
||||
{
|
||||
repeater(records)
|
||||
{
|
||||
field(id; Rec.SystemId)
|
||||
{
|
||||
Caption = 'id';
|
||||
Editable = false;
|
||||
}
|
||||
field(number; Rec."No.")
|
||||
{
|
||||
Caption = 'number';
|
||||
}
|
||||
field(posted; IsPosted)
|
||||
{
|
||||
Caption = 'posted';
|
||||
|
||||
trigger OnValidate()
|
||||
var
|
||||
PostHelper: Codeunit "WS Bound Action Bad Helper";
|
||||
begin
|
||||
if IsPosted then
|
||||
PostHelper.PostOrder(Rec);
|
||||
end;
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
var
|
||||
IsPosted: Boolean;
|
||||
}
|
||||
|
||||
codeunit 50353 "WS Bound Action Bad Helper"
|
||||
{
|
||||
procedure PostOrder(var SalesHeader: Record "Sales Header")
|
||||
var
|
||||
SalesPost: Codeunit "Sales-Post";
|
||||
begin
|
||||
SalesPost.Run(SalesHeader);
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,59 @@
|
|||
page 50350 "WS Bound Action Good"
|
||||
{
|
||||
PageType = API;
|
||||
Caption = 'salesOrder';
|
||||
APIPublisher = 'contoso';
|
||||
APIGroup = 'sales';
|
||||
APIVersion = 'v1.0';
|
||||
EntityName = 'salesOrder';
|
||||
EntitySetName = 'salesOrders';
|
||||
ODataKeyFields = SystemId;
|
||||
SourceTable = "Sales Header";
|
||||
DelayedInsert = true;
|
||||
|
||||
layout
|
||||
{
|
||||
area(content)
|
||||
{
|
||||
repeater(records)
|
||||
{
|
||||
field(id; Rec.SystemId)
|
||||
{
|
||||
Caption = 'id';
|
||||
Editable = false;
|
||||
}
|
||||
field(number; Rec."No.")
|
||||
{
|
||||
Caption = 'number';
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
[ServiceEnabled]
|
||||
procedure Post(var ActionContext: WebServiceActionContext)
|
||||
var
|
||||
PostHelper: Codeunit "WS Bound Action Helper";
|
||||
begin
|
||||
PostHelper.PostOrder(Rec);
|
||||
SetActionResponse(ActionContext, Rec.SystemId);
|
||||
end;
|
||||
|
||||
local procedure SetActionResponse(var ActionContext: WebServiceActionContext; CreatedId: Guid)
|
||||
begin
|
||||
ActionContext.SetObjectType(ObjectType::Page);
|
||||
ActionContext.SetObjectId(Page::"WS Bound Action Good");
|
||||
ActionContext.AddEntityKey(Rec.FieldNo(SystemId), CreatedId);
|
||||
ActionContext.SetResultCode(WebServiceActionResultCode::Updated);
|
||||
end;
|
||||
}
|
||||
|
||||
codeunit 50352 "WS Bound Action Helper"
|
||||
{
|
||||
procedure PostOrder(var SalesHeader: Record "Sales Header")
|
||||
var
|
||||
SalesPost: Codeunit "Sales-Post";
|
||||
begin
|
||||
SalesPost.Run(SalesHeader);
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: web-services
|
||||
keywords: [api-page, serviceenabled, bound-action, webserviceactioncontext, setactionresponse, side-effect, patch]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Expose business operations as bound actions, not as writable status flags
|
||||
|
||||
## Description
|
||||
|
||||
An API consumer that needs to *do* something to a record — post it, ship it, release it — should call an explicit operation, not mutate a field and hope a side effect fires. AL models this with a bound action: a `[ServiceEnabled] procedure` that takes `var ActionContext: WebServiceActionContext`, performs the work, and reports the result through the action context (typically a `SetActionResponse` helper that returns the affected record's id). The endpoint then exposes a callable action — `.../salesOrders(<id>)/Microsoft.NAV.post` — with a clear contract. The anti-pattern is to expose a writable Boolean or status field whose `OnValidate` quietly performs the operation: a routine PATCH that looks like a data edit silently triggers posting, with no discoverable action and surprising, hard-to-audit behaviour. LLMs reach for the flag-field approach because it is less code; this file is remedial because the platform-idiomatic, contract-safe choice (a bound action) is not the model's default.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Declare the operation as `[ServiceEnabled] procedure Post(var ActionContext: WebServiceActionContext)` on the API page. Inside, perform the operation against `Rec`, then call a `SetActionResponse` helper that writes the result — the bound record and its id — back into the `WebServiceActionContext` so the caller receives a well-formed response. The operation is now an explicit, named endpoint action separate from ordinary field writes.
|
||||
|
||||
See sample: `expose-operations-as-bound-actions.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Exposing a writable Boolean (for example `posted`) whose `OnValidate` performs the posting. A client that PATCHes the field to `true` — an action indistinguishable from any other data edit — silently triggers a side-effecting business operation. The detection signal: an API page field whose `OnValidate` posts, ships, or releases, instead of a `[ServiceEnabled]` bound action.
|
||||
|
||||
See sample: `expose-operations-as-bound-actions.bad.al`.
|
||||
|
|
@ -0,0 +1,33 @@
|
|||
// Unstable key: the endpoint addresses records by the business field "No.".
|
||||
// When a user renames a customer's number, every external reference built on
|
||||
// the old value dangles. ODataKeyFields should be SystemId instead.
|
||||
page 50345 "WS SystemId Key Bad"
|
||||
{
|
||||
PageType = API;
|
||||
APIPublisher = 'contoso';
|
||||
APIGroup = 'sales';
|
||||
APIVersion = 'v1.0';
|
||||
EntityName = 'customer';
|
||||
EntitySetName = 'customers';
|
||||
ODataKeyFields = "No.";
|
||||
SourceTable = Customer;
|
||||
DelayedInsert = true;
|
||||
|
||||
layout
|
||||
{
|
||||
area(content)
|
||||
{
|
||||
repeater(records)
|
||||
{
|
||||
field(number; Rec."No.")
|
||||
{
|
||||
Caption = 'number';
|
||||
}
|
||||
field(displayName; Rec.Name)
|
||||
{
|
||||
Caption = 'displayName';
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -0,0 +1,36 @@
|
|||
page 50344 "WS SystemId Key Good"
|
||||
{
|
||||
PageType = API;
|
||||
Caption = 'customer';
|
||||
APIPublisher = 'contoso';
|
||||
APIGroup = 'sales';
|
||||
APIVersion = 'v1.0';
|
||||
EntityName = 'customer';
|
||||
EntitySetName = 'customers';
|
||||
ODataKeyFields = SystemId;
|
||||
SourceTable = Customer;
|
||||
DelayedInsert = true;
|
||||
|
||||
layout
|
||||
{
|
||||
area(content)
|
||||
{
|
||||
repeater(records)
|
||||
{
|
||||
field(id; Rec.SystemId)
|
||||
{
|
||||
Caption = 'id';
|
||||
Editable = false;
|
||||
}
|
||||
field(number; Rec."No.")
|
||||
{
|
||||
Caption = 'number';
|
||||
}
|
||||
field(displayName; Rec.Name)
|
||||
{
|
||||
Caption = 'displayName';
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: web-services
|
||||
keywords: [api-page, odatakeyfields, systemid, stable-key, guid, business-key, editable-false]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Address API records by SystemId, not by a renamable business key
|
||||
|
||||
## Description
|
||||
|
||||
Every BC table carries a `SystemId` — an immutable GUID assigned at insert and never reused. API consumers must address a record through a key that does not change, otherwise a previously stored URL or `@odata.id` reference breaks the moment a user renames the underlying business key. The convention is to set `ODataKeyFields = SystemId` on the API page and expose the GUID as a non-editable `field(id; Rec.SystemId)`. An LLM left to its own devices often reaches for the human-readable primary key (a customer `No.`, an item code) as the OData key, because that is what a developer types when filtering in AL. That choice is wrong for an external contract: business keys are renamable and the API caller's stored references would dangle. This file is remedial because the correct key (`SystemId`) is rarely the one the model would pick by analogy with ordinary AL code.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Set `ODataKeyFields = SystemId` so OData routes records by the stable GUID, and expose it as `field(id; Rec.SystemId)` marked `Editable = false`. Clients then address a record at `.../customers(<guid>)`, an identity that survives any rename of the business key. Keep the business key (for example `No.`) as an ordinary exposed field, not as the OData key.
|
||||
|
||||
See sample: `expose-systemid-as-the-api-key.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Setting `ODataKeyFields = "No."` so the endpoint addresses records by a renamable business field. As soon as a user changes that `No.`, every external reference built on the old value points at nothing, silently breaking integrations. The detection signal: `ODataKeyFields` set to a business field rather than `SystemId`, or an API page that exposes no `id` field bound to `Rec.SystemId`.
|
||||
|
||||
See sample: `expose-systemid-as-the-api-key.bad.al`.
|
||||
|
|
@ -0,0 +1,24 @@
|
|||
// Malformed API endpoint: APIPublisher and APIGroup are missing, and there is
|
||||
// no SourceTable. The page compiles but the route cannot be composed, so the
|
||||
// entity is never published where an integration expects it.
|
||||
page 50341 "WS Required Props Bad"
|
||||
{
|
||||
PageType = API;
|
||||
APIVersion = 'v1.0';
|
||||
EntityName = 'customer';
|
||||
EntitySetName = 'customers';
|
||||
|
||||
layout
|
||||
{
|
||||
area(content)
|
||||
{
|
||||
repeater(records)
|
||||
{
|
||||
field(displayName; Rec.Name)
|
||||
{
|
||||
Caption = 'displayName';
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -0,0 +1,36 @@
|
|||
page 50340 "WS Required Props Good"
|
||||
{
|
||||
PageType = API;
|
||||
Caption = 'customer';
|
||||
APIPublisher = 'contoso';
|
||||
APIGroup = 'sales';
|
||||
APIVersion = 'v1.0';
|
||||
EntityName = 'customer';
|
||||
EntitySetName = 'customers';
|
||||
ODataKeyFields = SystemId;
|
||||
SourceTable = Customer;
|
||||
DelayedInsert = true;
|
||||
|
||||
layout
|
||||
{
|
||||
area(content)
|
||||
{
|
||||
repeater(records)
|
||||
{
|
||||
field(id; Rec.SystemId)
|
||||
{
|
||||
Caption = 'id';
|
||||
Editable = false;
|
||||
}
|
||||
field(number; Rec."No.")
|
||||
{
|
||||
Caption = 'number';
|
||||
}
|
||||
field(displayName; Rec.Name)
|
||||
{
|
||||
Caption = 'displayName';
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: web-services
|
||||
keywords: [api-page, pagetype-api, apipublisher, apigroup, apiversion, entityname, entitysetname, sourcetable]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Declare every required property on a PageType = API page
|
||||
|
||||
## Description
|
||||
|
||||
An API page projects a table as an OData v4 / API v2 endpoint, but the platform only publishes that endpoint when the page carries the full set of identifying properties: `APIPublisher`, `APIGroup`, `APIVersion`, `EntityName`, `EntitySetName`, and a backing `SourceTable`. These properties are what compose the route — `/api/<publisher>/<group>/<version>/<entitySet>` — so omitting any one of them yields a page that compiles yet never surfaces as a usable endpoint, or surfaces at an unexpected address. An LLM that has mostly seen ordinary list/card pages tends to treat `PageType = API` as a cosmetic switch and forgets the identifying metadata, because a normal page needs none of it. This file is remedial precisely because the missing-property failure is silent: there is no runtime error, only an endpoint that clients cannot reach.
|
||||
|
||||
## Best Practice
|
||||
|
||||
On every `PageType = API` page set all six properties explicitly: `APIPublisher` (your publisher tag), `APIGroup` (the logical grouping for related entities), `APIVersion` (a `vX.Y` value such as `'v1.0'`), `EntityName` (singular), `EntitySetName` (plural), and `SourceTable` (the projected table). Expose the record's fields inside a single `field(...)` repeater under `area(content)`. Treat the six properties as a mandatory checklist that travels with the `PageType = API` declaration itself.
|
||||
|
||||
See sample: `set-required-api-page-properties.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Writing a page with `PageType = API` and a `SourceTable` but leaving out `APIPublisher` and `APIGroup` (and, worse, omitting `SourceTable` entirely). The page compiles, so it looks finished, but the endpoint is malformed: with no publisher and group the route cannot be composed, and the entity is never published where an integration expects it. The detection signal: a `PageType = API` page missing one or more of the six identifying properties.
|
||||
|
||||
See sample: `set-required-api-page-properties.bad.al`.
|
||||
|
|
@ -0,0 +1,35 @@
|
|||
// Breaking change in place: the published v1.0 is edited rather than versioned.
|
||||
// EntityName was renamed from 'customer' to 'client' and the displayName field
|
||||
// was removed, so the single declared version now serves a different contract
|
||||
// than the one clients integrated against. Every existing consumer breaks.
|
||||
page 50355 "WS API Versioning Bad"
|
||||
{
|
||||
PageType = API;
|
||||
APIPublisher = 'contoso';
|
||||
APIGroup = 'sales';
|
||||
APIVersion = 'v1.0';
|
||||
EntityName = 'client';
|
||||
EntitySetName = 'clients';
|
||||
ODataKeyFields = SystemId;
|
||||
SourceTable = Customer;
|
||||
DelayedInsert = true;
|
||||
|
||||
layout
|
||||
{
|
||||
area(content)
|
||||
{
|
||||
repeater(records)
|
||||
{
|
||||
field(id; Rec.SystemId)
|
||||
{
|
||||
Caption = 'id';
|
||||
Editable = false;
|
||||
}
|
||||
field(number; Rec."No.")
|
||||
{
|
||||
Caption = 'number';
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -0,0 +1,39 @@
|
|||
// Additive versioning: v2.0 carries the new shape while v1.0 stays published and
|
||||
// unchanged. APIVersion accepts a list, so both contracts are served and
|
||||
// existing clients keep working while new clients adopt v2.0.
|
||||
page 50354 "WS API Versioning Good"
|
||||
{
|
||||
PageType = API;
|
||||
Caption = 'customer';
|
||||
APIPublisher = 'contoso';
|
||||
APIGroup = 'sales';
|
||||
APIVersion = 'v2.0', 'v1.0';
|
||||
EntityName = 'customer';
|
||||
EntitySetName = 'customers';
|
||||
ODataKeyFields = SystemId;
|
||||
SourceTable = Customer;
|
||||
DelayedInsert = true;
|
||||
|
||||
layout
|
||||
{
|
||||
area(content)
|
||||
{
|
||||
repeater(records)
|
||||
{
|
||||
field(id; Rec.SystemId)
|
||||
{
|
||||
Caption = 'id';
|
||||
Editable = false;
|
||||
}
|
||||
field(number; Rec."No.")
|
||||
{
|
||||
Caption = 'number';
|
||||
}
|
||||
field(displayName; Rec.Name)
|
||||
{
|
||||
Caption = 'displayName';
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: web-services
|
||||
keywords: [api-page, apiversion, versioning, published-contract, breaking-change, backward-compatibility]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Version APIs by adding a new APIVersion, not by mutating a published one
|
||||
|
||||
## Description
|
||||
|
||||
Once an API version is published, external clients depend on its exact shape — the entity name, the set of exposed fields, the key — as a frozen contract. Changing any of that on the already-published version is a breaking change delivered silently: integrations that worked yesterday fail today with no warning. The platform gives you a clean way to evolve without breaking anyone, because `APIVersion` accepts a *list* of versions on one page. The correct way to change a published API is to add the new version (`'v2.0'`) alongside the existing one (`'v1.0'`) — or publish a new API page for it — so both contracts are served side by side and clients migrate on their own schedule. LLMs tend to "fix" an API by editing the live version in place, because in ordinary code you just change what's wrong; this file is remedial because a published API version is an immutable contract in a way ordinary internal code is not.
|
||||
|
||||
## Best Practice
|
||||
|
||||
When a published API must change shape, keep the old version's contract intact and add the new one to the `APIVersion` list — `APIVersion = 'v2.0', 'v1.0';`. The page now serves both `v1.0` (unchanged) and `v2.0` (carrying the new shape), so existing clients keep working while new clients adopt `v2.0`. Retire the old version only after consumers have migrated.
|
||||
|
||||
See sample: `version-apis-by-adding-not-mutating-published-versions.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Editing the published `v1.0` page in place — renaming its `EntityName` or removing an exposed field — so the single declared version now serves a different contract than the one clients integrated against. Every consumer of the old shape breaks without notice. The detection signal: a change that renames the entity or removes a field on an existing published `APIVersion` instead of adding a new version to the list.
|
||||
|
||||
See sample: `version-apis-by-adding-not-mutating-published-versions.bad.al`.
|
||||
|
|
@ -3,7 +3,7 @@ kind: action-skill
|
|||
id: al-code-review
|
||||
version: 1
|
||||
title: AL code review
|
||||
description: Reviews AL source changes by composing the AL review leaf skills (performance, security, privacy, upgrade, style, UI, error handling, events, interfaces, breaking changes).
|
||||
description: Reviews AL source changes by composing the AL review leaf skills (performance, security, privacy, upgrade, style, UI, error handling, events, interfaces, breaking changes, web services).
|
||||
inputs: [pr-diff, file-path]
|
||||
outputs: [findings-report]
|
||||
bc-version: [all]
|
||||
|
|
@ -21,6 +21,7 @@ sub-skills:
|
|||
- microsoft/skills/review/al-events-review.md
|
||||
- microsoft/skills/review/al-interfaces-review.md
|
||||
- microsoft/skills/review/al-breaking-changes-review.md
|
||||
- microsoft/skills/review/al-web-services-review.md
|
||||
---
|
||||
|
||||
# AL code review
|
||||
|
|
@ -45,6 +46,7 @@ The sub-skills invoked by this skill are those listed in frontmatter `sub-skills
|
|||
- `microsoft/skills/review/al-events-review.md`
|
||||
- `microsoft/skills/review/al-interfaces-review.md`
|
||||
- `microsoft/skills/review/al-breaking-changes-review.md`
|
||||
- `microsoft/skills/review/al-web-services-review.md`
|
||||
|
||||
Additional leaf skills (for example, telemetry, testing) are added by updating the `sub-skills` list. The skill does not discover sub-skills implicitly.
|
||||
|
||||
|
|
|
|||
136
microsoft/skills/review/al-web-services-review.md
Normal file
136
microsoft/skills/review/al-web-services-review.md
Normal file
|
|
@ -0,0 +1,136 @@
|
|||
---
|
||||
kind: action-skill
|
||||
id: al-web-services-review
|
||||
version: 1
|
||||
title: AL web services review
|
||||
description: Reviews AL source changes against web-services (API page) guidance from BCQuality.
|
||||
inputs: [pr-diff, file-path]
|
||||
outputs: [findings-report]
|
||||
bc-version: [all]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# AL web services review
|
||||
|
||||
Reviews AL source changes against the `web-services` 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 either a `pr-diff` (the standard PR-review entry point) or a `file-path` (single-file review). The skill produces a single JSON document conforming to the DO output contract.
|
||||
|
||||
## Source
|
||||
|
||||
Read the BCQuality knowledge index once — the `knowledge-index.json` BCQuality builds at the root of the knowledge checkout (Entry's preparation step regenerates it over the live, already-filtered clone — see `skills/entry.md`). It lists every article that survived layer and allow/deny filtering and carries, per article, its `path`, `layer`, `domain`, frontmatter dimensions, `keywords`, `title`, and a one-line `description` hint — exactly the fields Relevance and Worklist consume. Take the index entries whose `domain` is `web-services` as this skill's candidate set across every enabled layer; do not open the individual article files at this step. Open an article's full body only once it enters the Worklist below, so a review reads the index plus the handful of worklisted articles instead of every file under `*/knowledge/web-services/**`.
|
||||
|
||||
## Relevance
|
||||
|
||||
Apply the frontmatter matching rules defined in READ (*Frontmatter matching semantics*) against the task context:
|
||||
|
||||
- `bc-version` — the target BC version from the PR branch's `app.json` or the orchestrator-supplied version. If unavailable, the dimension is `unknown`.
|
||||
- `technologies` — `[al]`.
|
||||
- `countries` — the countries declared in the consuming app's `app.json`. Default to the orchestrator's configured context; if absent, `unknown`.
|
||||
- `application-area` — the union of application areas declared by the changed objects. Pass the actual set; do not substitute `[all]`. If the area cannot be determined from the changes, the dimension is `unknown`.
|
||||
|
||||
Discard files that are not applicable. Retain conditionally applicable files (any dimension `unknown`) only when the orchestrator's configuration permits them; findings derived from those files MUST have `confidence` no higher than `medium`, AND the finding's `message` MUST name the dimension or dimensions that were unknown.
|
||||
|
||||
## Worklist
|
||||
|
||||
Narrow the relevant files to the subset that applies to the changes under review. For each relevant file, compute overlap against:
|
||||
|
||||
- The changed AL object names and types — especially page objects declared with `PageType = API`, and any procedure on such a page that exposes a bound action.
|
||||
- The changed properties and triggers, weighted toward API page metadata (`APIPublisher`, `APIGroup`, `APIVersion`, `EntityName`, `EntitySetName`, `ODataKeyFields`, `SourceTable`), CRUD guards (`InsertAllowed`, `ModifyAllowed`, `DeleteAllowed`, `Editable`), the `OnOpenPage` trigger, and `OnValidate` triggers on exposed fields.
|
||||
- Tokens extracted from the diff that relate to API surface and behaviour (`PageType`, `API`, `APIPublisher`, `APIGroup`, `APIVersion`, `EntityName`, `EntitySetName`, `ODataKeyFields`, `SystemId`, `ServiceEnabled`, `WebServiceActionContext`, `SetActionResponse`, `ReadIsolation`, `IsolationLevel`, `ReadCommitted`, `InsertAllowed`, `ModifyAllowed`, `DeleteAllowed`, `Editable`, `SourceTable`).
|
||||
|
||||
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.
|
||||
|
||||
Once the candidate worklist is known, resolve layer-precedence conflicts per READ. Drop lower-precedence files whose normative guidance (`## Best Practice` or `## Anti Pattern`) directly contradicts a higher-precedence candidate, and record each dropped file in `suppressed` with `reason: "layer-precedence"`. Files that would have been candidates but are hidden because their layer is disabled in consumer configuration are recorded with `reason: "configuration"`. Files that never became candidates are NOT recorded in `suppressed`.
|
||||
|
||||
When the post-conflict worklist is empty because no applicable web-services knowledge exists, or because configuration suppressed every candidate, emit `outcome: "no-knowledge"`. When the worklist is empty because no applicable web-services knowledge matched the changes, emit `outcome: "completed"` with an empty `findings` array.
|
||||
|
||||
## Action
|
||||
|
||||
For each worklist entry, evaluate the diff against the file's `## Best Practice` and `## Anti Pattern` sections. Emit findings as follows:
|
||||
|
||||
- When the diff contains a clear match for an Anti Pattern, emit a finding with severity `major` or `blocker`, a message summarizing the anti-pattern, `location` pointing to the offending line or range, and a `references` entry pointing to the knowledge file. Use `blocker` only when the knowledge file states the anti-pattern violates a platform-level guarantee. When the file does not make such a claim, the ceiling is `major`.
|
||||
- When the diff contains code that contradicts a Best Practice without being a full anti-pattern, emit `minor` with the same reference shape.
|
||||
- When the skill cannot detect a violation but the file is clearly applicable to the change, emit `info` citing the file. Repository-wide observations MAY omit `location`.
|
||||
|
||||
Set `confidence` to:
|
||||
|
||||
- `high` when the detection is based on an unambiguous pattern match (identifier, syntax, object type).
|
||||
- `medium` when detection relies on heuristics or when any frontmatter dimension was `unknown`.
|
||||
- `low` when the finding is an advisory derived only from applicability.
|
||||
|
||||
After evaluating each worklist entry, also consider whether the diff exhibits a web-services defect the agent recognises from its general AL knowledge that no knowledge file in the worklist covers. Such candidates are agent findings within this skill's domain — emit them with `references: []`, an `id` slug prefixed with `agent:`, `confidence` capped at `medium`, `severity` capped at `minor` (agent findings are advisory and non-gating), and a `message` that is self-contained (describing both the issue and a concrete recommendation, since there is no knowledge-file footer for the consumer to fall back on). Hold every candidate to the precision bar in `skills/do.md` (*Agent findings*): emit only a concrete, material web-services defect a knowledgeable BC reviewer would agree is wrong — steelman it first and drop anything stylistic, speculative, dependent on code outside the diff, or merely a valid alternative; when in doubt, omit. The scope is strictly API pages and web-service surfaces; defects outside this domain belong to other leaves and MUST NOT be emitted here. Before emitting, check the worklist for a knowledge file that matches the candidate — if one exists, upgrade the candidate to a knowledge-backed finding instead. See `skills/do.md` for the full contract.
|
||||
|
||||
For every emitted finding, decide whether the fix is mechanical. A fix is mechanical when it is small, local, and unambiguous from the diff context (for example: set `ODataKeyFields = SystemId`; add the three `*Allowed = false` guards to a read-only page; add the missing `OnOpenPage` isolation assignment). For mechanical findings, emit `findings[].suggested-code` with the literal replacement for the source lines indicated by `location`. The payload must be a verbatim replacement — no diff markers, no fences, no commentary — that the consumer can render as a one-click suggestion. When a `.good.al` companion exists and the diff context matches the `.bad.al` shape, adapt the `.good.al` replacement into `suggested-code`.
|
||||
|
||||
Omit `suggested-code` only when the appropriate fix depends on context the skill cannot determine, when multiple defensible replacements exist, or when the fix spans non-contiguous code. If a finding is mechanical-looking but you omit `suggested-code`, set `findings[].suggested-code-omission-reason` to a short explanation. See `skills/do.md` for the full contract.
|
||||
|
||||
Outcome selection:
|
||||
|
||||
- `completed` — the skill evaluated every worklist item; default when the skill finishes normally, including when the resulting `findings` array is empty.
|
||||
- `no-knowledge` — no applicable web-services knowledge survived Source, Relevance, configuration filtering, and conflict resolution. `findings` is empty.
|
||||
- `not-applicable` — the task context lacks an AL dimension (no AL changes in the diff, or `technologies` filter rejected the task).
|
||||
- `partial` — a time or token budget was hit before the worklist was exhausted. `summary.coverage` reflects the evaluated subset; `outcome-reason` explains the cause.
|
||||
- `failed` — an unrecoverable error occurred. `outcome-reason` is required.
|
||||
|
||||
## Output
|
||||
|
||||
Output conforms to the DO output contract. A populated example:
|
||||
|
||||
```json
|
||||
{
|
||||
"skill": { "id": "al-web-services-review", "version": 1 },
|
||||
"outcome": "completed",
|
||||
"summary": {
|
||||
"counts": { "blocker": 0, "major": 1, "minor": 1, "info": 0 },
|
||||
"coverage": { "worklist-size": 2, "items-evaluated": 2 }
|
||||
},
|
||||
"findings": [
|
||||
{
|
||||
"id": "microsoft/knowledge/web-services/set-required-api-page-properties.md",
|
||||
"severity": "major",
|
||||
"message": "This PageType = API page declares a SourceTable but omits APIPublisher and APIGroup, so the endpoint route cannot be composed and the entity is never published. Declare all six required API page properties.",
|
||||
"location": {
|
||||
"file": "src/Api/CustomerApi.Page.al",
|
||||
"line": 3,
|
||||
"range": { "start-line": 1, "end-line": 8 }
|
||||
},
|
||||
"references": [
|
||||
{ "path": "microsoft/knowledge/web-services/set-required-api-page-properties.md" }
|
||||
],
|
||||
"confidence": "high"
|
||||
},
|
||||
{
|
||||
"id": "microsoft/knowledge/web-services/expose-systemid-as-the-api-key.md",
|
||||
"severity": "minor",
|
||||
"message": "This API page sets ODataKeyFields to a renamable business field instead of SystemId, so stored references break when the business key changes. Set ODataKeyFields = SystemId and expose field(id; Rec.SystemId).",
|
||||
"location": {
|
||||
"file": "src/Api/CustomerApi.Page.al",
|
||||
"line": 9
|
||||
},
|
||||
"references": [
|
||||
{ "path": "microsoft/knowledge/web-services/expose-systemid-as-the-api-key.md" }
|
||||
],
|
||||
"confidence": "high"
|
||||
}
|
||||
],
|
||||
"suppressed": []
|
||||
}
|
||||
```
|
||||
|
||||
The empty-corpus case — when no web-services knowledge survives filtering — produces:
|
||||
|
||||
```json
|
||||
{
|
||||
"skill": { "id": "al-web-services-review", "version": 1 },
|
||||
"outcome": "no-knowledge",
|
||||
"summary": {
|
||||
"counts": { "blocker": 0, "major": 0, "minor": 0, "info": 0 },
|
||||
"coverage": { "worklist-size": 0, "items-evaluated": 0 }
|
||||
},
|
||||
"findings": [],
|
||||
"suppressed": []
|
||||
}
|
||||
```
|
||||
Loading…
Add table
Add a link
Reference in a new issue