mirror of
https://github.com/microsoft/BCQuality.git
synced 2026-10-05 06:36:55 +01:00
Strengthen review contracts and add AL reliability guidance (#196)
* Strengthen review contracts and HTTP guidance - add outbound HttpClient transport and HTTP status review rules with paired fixtures`n- resolve layered action-skill overrides deterministically across enabled layers`n- validate findings reports and enforce measurable changed-fixture coverage * Add data handling and test isolation guidance - add SCM guidance for deriving base quantities through line unit-of-measure validation`n- add security guidance for parameterizing SetFilter with external text`n- add test isolation guidance for resetting per-test state before initialization guards`n- add web-service guidance for JSON null handling and invariant standard format 9`n- route and cover all five rules with paired evaluation fixtures * Fix findings report rollup validation * Validate findings report rollups * Enforce merged finding identity * Fix locationless finding deduplication * Reject conflicting merged corrections * Detect conflicting leaf corrections * Route HTTP error checks to canonical web-services knowledge Let the Error Handling leaf conditionally retrieve the existing HTTP owner articles, preserving applicability and exact-path provenance. Add deterministic source-contract and retrieval regressions without duplicating knowledge rules. Copilot-Session-Id: a92a7788-103e-4651-9b84-19e34caffb94 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: wenjiefan <wenjiefan@microsoft.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
This commit is contained in:
Stefano Demiliani
GitHub
parent
130d5de6c4
commit
4287233f80
41 changed files with 1974 additions and 38 deletions
|
|
@ -0,0 +1,21 @@
|
|||
codeunit 50100 "HTTP Status Handling Bad"
|
||||
{
|
||||
procedure GetCustomer(CustomerId: Guid): JsonObject
|
||||
var
|
||||
Client: HttpClient;
|
||||
Response: HttpResponseMessage;
|
||||
CustomerJson: JsonObject;
|
||||
ResponseText: Text;
|
||||
begin
|
||||
if not Client.Get(
|
||||
StrSubstNo('https://api.example.com/customers/%1', CustomerId),
|
||||
Response)
|
||||
then
|
||||
Error('The customer service could not be reached.');
|
||||
|
||||
// A completed request can still contain a 4xx or 5xx error document.
|
||||
Response.Content().ReadAs(ResponseText);
|
||||
CustomerJson.ReadFrom(ResponseText);
|
||||
exit(CustomerJson);
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,23 @@
|
|||
codeunit 50100 "HTTP Status Handling Good"
|
||||
{
|
||||
procedure GetCustomer(CustomerId: Guid): JsonObject
|
||||
var
|
||||
Client: HttpClient;
|
||||
Response: HttpResponseMessage;
|
||||
CustomerJson: JsonObject;
|
||||
ResponseText: Text;
|
||||
begin
|
||||
if not Client.Get(
|
||||
StrSubstNo('https://api.example.com/customers/%1', CustomerId),
|
||||
Response)
|
||||
then
|
||||
Error('The customer service could not be reached.');
|
||||
|
||||
if not Response.IsSuccessStatusCode() then
|
||||
Error('The customer service returned HTTP status %1.', Response.HttpStatusCode());
|
||||
|
||||
Response.Content().ReadAs(ResponseText);
|
||||
CustomerJson.ReadFrom(ResponseText);
|
||||
exit(CustomerJson);
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,31 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: web-services
|
||||
keywords: [httpclient, httpresponsemessage, issuccessstatuscode, httpstatuscode, response-body, json]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Check HTTP status before consuming the response body
|
||||
|
||||
## Description
|
||||
|
||||
A successful AL `HttpClient` call only confirms that the platform completed the HTTP exchange. The server can still return `4xx` or `5xx`, often with an error document whose shape differs from the expected success payload. Parsing that body as business data can produce misleading parse errors, incomplete records, or decisions based on an error response.
|
||||
|
||||
## Best Practice
|
||||
|
||||
After handling any platform or transport failure, check `HttpResponseMessage.IsSuccessStatusCode()` or the expected `HttpStatusCode()` before interpreting the response body as a success payload. Handle non-success status explicitly and include safe diagnostic context when appropriate. A bounded error body may be read for diagnostics, but it must not enter the success parsing path.
|
||||
|
||||
See sample: [`check-http-status-before-consuming-response-body.good.al`](check-http-status-before-consuming-response-body.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Checking only the Boolean result of `Get`, `Post`, `Put`, `Delete`, or `Send` and then parsing `Response.Content()` as the expected payload. The Boolean can be `true` for any HTTP status, including authentication failures, throttling, validation errors, and server failures.
|
||||
|
||||
See sample: [`check-http-status-before-consuming-response-body.bad.al`](check-http-status-before-consuming-response-body.bad.al).
|
||||
|
||||
## References
|
||||
|
||||
- [HttpResponseMessage.IsSuccessStatusCode method](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/methods-auto/httpresponsemessage/httpresponsemessage-issuccessstatuscode-method)
|
||||
- [HttpClient data type](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/methods-auto/httpclient/httpclient-data-type)
|
||||
|
|
@ -0,0 +1,11 @@
|
|||
codeunit 50173 "Contact Payload Reader Bad"
|
||||
{
|
||||
procedure ApplyPayload(var Contact: Record Contact; Payload: JsonObject)
|
||||
var
|
||||
Token: JsonToken;
|
||||
begin
|
||||
if Payload.Get('email', Token) then
|
||||
Contact.Validate("E-Mail", CopyStr(Token.AsValue().AsText(), 1, MaxStrLen(Contact."E-Mail")));
|
||||
Contact.Modify(true);
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,28 @@
|
|||
codeunit 50172 "Contact Payload Reader Good"
|
||||
{
|
||||
procedure ApplyPayload(var Contact: Record Contact; Payload: JsonObject)
|
||||
var
|
||||
EmailAddress: Text;
|
||||
begin
|
||||
if TryGetText(Payload, 'email', EmailAddress) then
|
||||
Contact.Validate("E-Mail", CopyStr(EmailAddress, 1, MaxStrLen(Contact."E-Mail")));
|
||||
Contact.Modify(true);
|
||||
end;
|
||||
|
||||
local procedure TryGetText(Payload: JsonObject; PropertyName: Text; var Value: Text): Boolean
|
||||
var
|
||||
Token: JsonToken;
|
||||
begin
|
||||
if not Payload.Get(PropertyName, Token) then
|
||||
exit(false);
|
||||
if not Token.IsValue() then
|
||||
Error(NotAValueErr, PropertyName);
|
||||
if Token.AsValue().IsNull() then
|
||||
exit(false);
|
||||
Value := Token.AsValue().AsText();
|
||||
exit(true);
|
||||
end;
|
||||
|
||||
var
|
||||
NotAValueErr: Label 'The property %1 must contain a single value.', Comment = '%1 = JSON property name';
|
||||
}
|
||||
|
|
@ -0,0 +1,35 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: web-services
|
||||
keywords: [jsonobject, jsontoken, jsonvalue, isnull, asvalue, astext, asdecimal, optional-property, json-null, payload-parsing]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Check for JSON null before converting a value
|
||||
|
||||
## Description
|
||||
|
||||
An optional property in a JSON payload can be missing or present with the value `null`, and the two cases behave differently in AL. When the Boolean result is captured, `JsonObject.Get` returns `false` for a missing key but `true` for `"email": null`, because the key exists. The conversion methods on `JsonValue` (`AsText`, `AsCode`, `AsDecimal`, `AsInteger`, `AsDate`, `AsBoolean`, and the others) fail with a runtime error when the value is `NULL` or `UNDEFINED`. Code that guards only with `Get` therefore passes its tests with the property omitted and fails in production when the sender serializes an empty field as `null`, which many services do by default.
|
||||
|
||||
## Best Practice
|
||||
|
||||
For every property that the contract allows to be optional or nullable, check three things before converting: that `Get` (or `SelectToken`) returned `true`, that the token `IsValue()` rather than an object or array, and that `AsValue().IsNull()` is `false`. Put this in one small helper per target type and decide explicitly what a missing or null property means: a default, leaving the field unchanged, or a validation error that names the property.
|
||||
|
||||
Required properties can still fail fast, but with an error that states the missing or null property instead of a generic conversion error. Keep a numeric or date conversion strict when the contract says the value must be a number or a date; `IsNull` covers only `null`, not a value of the wrong type.
|
||||
|
||||
See sample: [`check-json-null-before-converting-values.good.al`](check-json-null-before-converting-values.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
`if Json.Get('email', Token) then Email := Token.AsValue().AsText();` or an unguarded `Token.AsValue().AsDecimal()` on a property that the external contract allows to be `null`. The `Get` check makes the code look defensive, but it doesn't handle a present `null`. Detection signal: an `As<Type>()` call on a `JsonValue` obtained from an external payload with no preceding `IsNull()` check on the same token, where the property isn't documented as always non-null.
|
||||
|
||||
See sample: [`check-json-null-before-converting-values.bad.al`](check-json-null-before-converting-values.bad.al).
|
||||
|
||||
## References
|
||||
|
||||
- [JsonObject.Get method](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/methods-auto/jsonobject/jsonobject-get-method)
|
||||
- [JsonValue.AsText method: fails on NULL or UNDEFINED](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/methods-auto/jsonvalue/jsonvalue-astext-method)
|
||||
- [JsonValue.IsNull method](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/methods-auto/jsonvalue/jsonvalue-isnull-method)
|
||||
- [JsonToken data type](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/methods-auto/jsontoken/jsontoken-data-type)
|
||||
|
|
@ -0,0 +1,27 @@
|
|||
codeunit 50171 "Exchange Rate Export Bad"
|
||||
{
|
||||
procedure SendRate(CurrencyCode: Code[10]; StartingDate: Date; ExchangeRate: Decimal)
|
||||
var
|
||||
Client: HttpClient;
|
||||
Response: HttpResponseMessage;
|
||||
RequestUrl: Text;
|
||||
begin
|
||||
RequestUrl := StrSubstNo(RateUrlTok, CurrencyCode, Format(StartingDate), Format(ExchangeRate));
|
||||
if not Client.Get(RequestUrl, Response) then
|
||||
Error(RequestFailedErr);
|
||||
if not Response.IsSuccessStatusCode() then
|
||||
Error(RateRejectedErr, Response.HttpStatusCode());
|
||||
end;
|
||||
|
||||
procedure ReadRate(RateText: Text) ExchangeRate: Decimal
|
||||
begin
|
||||
if not Evaluate(ExchangeRate, RateText) then
|
||||
Error(InvalidRateErr, RateText);
|
||||
end;
|
||||
|
||||
var
|
||||
RateUrlTok: Label 'https://rates.example.com/rates?currency=%1&date=%2&rate=%3', Locked = true;
|
||||
RequestFailedErr: Label 'The exchange rate service could not be reached.';
|
||||
RateRejectedErr: Label 'The exchange rate service rejected the rate. Status code: %1.', Comment = '%1 = HTTP status code';
|
||||
InvalidRateErr: Label 'The exchange rate %1 is not a valid decimal number.', Comment = '%1 = received value';
|
||||
}
|
||||
|
|
@ -0,0 +1,27 @@
|
|||
codeunit 50170 "Exchange Rate Export Good"
|
||||
{
|
||||
procedure SendRate(CurrencyCode: Code[10]; StartingDate: Date; ExchangeRate: Decimal)
|
||||
var
|
||||
Client: HttpClient;
|
||||
Response: HttpResponseMessage;
|
||||
RequestUrl: Text;
|
||||
begin
|
||||
RequestUrl := StrSubstNo(RateUrlTok, CurrencyCode, Format(StartingDate, 0, 9), Format(ExchangeRate, 0, 9));
|
||||
if not Client.Get(RequestUrl, Response) then
|
||||
Error(RequestFailedErr);
|
||||
if not Response.IsSuccessStatusCode() then
|
||||
Error(RateRejectedErr, Response.HttpStatusCode());
|
||||
end;
|
||||
|
||||
procedure ReadRate(RateText: Text) ExchangeRate: Decimal
|
||||
begin
|
||||
if not Evaluate(ExchangeRate, RateText, 9) then
|
||||
Error(InvalidRateErr, RateText);
|
||||
end;
|
||||
|
||||
var
|
||||
RateUrlTok: Label 'https://rates.example.com/rates?currency=%1&date=%2&rate=%3', Locked = true;
|
||||
RequestFailedErr: Label 'The exchange rate service could not be reached.';
|
||||
RateRejectedErr: Label 'The exchange rate service rejected the rate. Status code: %1.', Comment = '%1 = HTTP status code';
|
||||
InvalidRateErr: Label 'The exchange rate %1 is not a valid decimal number.', Comment = '%1 = received value';
|
||||
}
|
||||
|
|
@ -0,0 +1,39 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: web-services
|
||||
keywords: [format, evaluate, standard-format-9, xml-format, locale, regional-settings, decimal-separator, data-exchange, integration]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Format exchanged values with standard format 9
|
||||
|
||||
## Description
|
||||
|
||||
`Format(Value)` uses standard format 0, the display format, and follows the current user's regional settings. The same decimal renders as `-76.543,21` for a European region and `-76,543.21` for English (US); the same date renders as `05-04-21` or `04/05/21`. Text built this way and sent outside Business Central (an HTTP query string or body, an XML or CSV file, a signature or hash input, or an external key) changes with the user or job queue session that produces it. The receiver can reject it or, worse, misread it. `Evaluate` without a format number has the same dependency when it parses machine-generated text.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Use `Format(Value, 0, 9)` for machine-readable text. Standard format 9 is the XML format and doesn't depend on the region: `-76543.21` for a decimal, `2021-04-05` for a date, `04:35:55.553` for a time, `true`/`false` for a Boolean, and a UTC `DateTime` such as `2021-04-05T03:35:55.553Z`. Parse such text with `Evaluate(Variable, Text, 9)`.
|
||||
|
||||
Prefer typed APIs when they exist. `JsonObject.Add` and `JsonValue.SetValue` with a `Decimal`, `Date`, or `Boolean` argument write a JSON value without going through display text. An XMLport handles this with `FormatEvaluate = Xml`.
|
||||
|
||||
For `Enum` and `Option` values, format 9 produces the ordinal number, not the name. When the external contract exchanges names, map them explicitly; see [`api-enum-values-are-a-contract-by-name-not-ordinal.md`](api-enum-values-are-a-contract-by-name-not-ordinal.md).
|
||||
|
||||
Text shown to a person (messages, captions, report columns, notifications) should keep the regional display format. `Code`, `Text`, and `Guid` values don't need format 9 because their standard formats don't vary by region.
|
||||
|
||||
See sample: [`format-exchanged-values-with-standard-format-9.good.al`](format-exchanged-values-with-standard-format-9.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
`Format(Amount)`, `Format(PostingDate)`, or `Format(SomeDateTime)` concatenated into a URL, request body, XML or CSV line, file name, or hash input. Passing the `Decimal` or `Date` itself to `StrSubstNo` for such text has the same effect, because `StrSubstNo` formats it with the display format. Also `Evaluate(DecimalOrDateVariable, ExternalText)` without format number 9 on text received from another system. The code usually works for the developer's own region and fails for users or job queue sessions in another one. Detection signal: `Format` with one argument, or with a format number other than 9, applied to a `Decimal`, `Date`, `Time`, `DateTime`, or `Boolean` on a path that writes to an `HttpContent`, `HttpRequestMessage`, `OutStream`, `XmlDocument`, or file.
|
||||
|
||||
See sample: [`format-exchanged-values-with-standard-format-9.bad.al`](format-exchanged-values-with-standard-format-9.bad.al).
|
||||
|
||||
## References
|
||||
|
||||
- [Formatting values, dates, and time: standard formats by region and format 9](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/devenv-format-property)
|
||||
- [System.Format method](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/methods-auto/system/system-format-joker-integer-integer-method)
|
||||
- [System.Evaluate method and format number 9](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/methods-auto/system/system-evaluate-method)
|
||||
- [FormatEvaluate property for XMLports](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/properties/devenv-formatevaluate-property)
|
||||
|
|
@ -0,0 +1,18 @@
|
|||
codeunit 50100 "Http Platform Failure Bad"
|
||||
{
|
||||
procedure GetCustomer(CustomerId: Guid): Text
|
||||
var
|
||||
Client: HttpClient;
|
||||
Response: HttpResponseMessage;
|
||||
ResponseText: Text;
|
||||
RequestSucceeded: Boolean;
|
||||
begin
|
||||
RequestSucceeded := Client.Get(
|
||||
StrSubstNo('https://api.example.com/customers/%1', CustomerId),
|
||||
Response);
|
||||
|
||||
// Response content is unavailable when the platform call failed.
|
||||
Response.Content().ReadAs(ResponseText);
|
||||
exit(ResponseText);
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,18 @@
|
|||
codeunit 50100 "Http Platform Failure Good"
|
||||
{
|
||||
procedure GetCustomer(CustomerId: Guid): Text
|
||||
var
|
||||
Client: HttpClient;
|
||||
Response: HttpResponseMessage;
|
||||
ResponseText: Text;
|
||||
begin
|
||||
if not Client.Get(
|
||||
StrSubstNo('https://api.example.com/customers/%1', CustomerId),
|
||||
Response)
|
||||
then
|
||||
Error('The customer service could not be reached.');
|
||||
|
||||
Response.Content().ReadAs(ResponseText);
|
||||
exit(ResponseText);
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,31 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: web-services
|
||||
keywords: [httpclient, transport-failure, boolean-return, httpresponsemessage, content, runtime-error]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Handle HttpClient platform failure before accessing the response
|
||||
|
||||
## Description
|
||||
|
||||
AL `HttpClient` methods can fail before a usable HTTP response exists because of an invalid request, DNS or network failure, certificate validation, timeout, a disabled extension setting, or the response-size limit. When code captures the optional Boolean return value, `false` reports this platform or transport failure. The accompanying `HttpResponseMessage` is not safe to consume; accessing its content after the failed call can raise another error and obscure the original failure.
|
||||
|
||||
## Best Practice
|
||||
|
||||
When capturing the Boolean return value from `Get`, `Post`, `Put`, `Delete`, or `Send`, stop the current response-processing path immediately when it is `false`. Report or propagate the transport failure without reading status, headers, or content. Omitting the optional Boolean is also valid when fail-fast behavior is intended: the runtime then raises an error if the operation cannot execute.
|
||||
|
||||
See sample: [`handle-httpclient-platform-failure-before-response-access.good.al`](handle-httpclient-platform-failure-before-response-access.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Capturing a failed call in a Boolean and then reading `Response.Content()`, parsing the body, or otherwise treating `Response` as usable. Do not report omission of the Boolean by itself; that form deliberately delegates failure propagation to the runtime.
|
||||
|
||||
See sample: [`handle-httpclient-platform-failure-before-response-access.bad.al`](handle-httpclient-platform-failure-before-response-access.bad.al).
|
||||
|
||||
## References
|
||||
|
||||
- [HttpClient.Send method](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/methods-auto/httpclient/httpclient-send-method)
|
||||
- [Call external services with HttpClient](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/devenv-httpclient)
|
||||
Loading…
Add table
Add a link
Reference in a new issue