diff --git a/.claude/agents/reviewer.md b/.claude/agents/reviewer.md index e26e604..4ba1542 100644 --- a/.claude/agents/reviewer.md +++ b/.claude/agents/reviewer.md @@ -16,14 +16,14 @@ First read `CLAUDE.md` and every **accepted** ADR in `docs/adr/` that relates to **ADR compliance** - ADR-0002: no `catch (Exception)` or bare `catch`. Expected failures (HTTP status, payload, network, timeout) are returned as `OpenUrzednikResult` errors. Only caller cancellation, argument errors and fatal errors are thrown; in provider client methods, a `null` argument or an undefined enum value throws synchronously, before the task is returned (flag an `async` public client method that checks them, or a `ValidationError` for them), while a non-null value of the right type that fails a format or range check is a returned `ValidationError`. Timeouts are distinguished from caller cancellation. Errors implement `CreateException()` returning `OpenUrzednikException` subtypes; exceptions keep their error in `Error`/`Errors`. Several validation errors are thrown as one `OpenUrzednikValidationException` listing all of them, never `AggregateException`; several errors of other kinds as the first error's exception with all of them in `Errors`. Resilience-handler rejections are translated in DI packages, not caught in providers. -- ADR-0003: no `ILogger`/`ActivitySource` in Core or provider packages, except Core's internal `Telemetry/Activities` (the .NET fallback trace source, compiled on .NET only). Members of those types that other packages call through `InternalsVisibleTo` are a binary contract: flag a change or removal outside a major. Span and tag naming; HTTP tags use the OpenTelemetry names (`http.request.method`, `url.path`, `http.response.status_code`); errors set `error.type`. Activity sources are named `OpenUrzednik.`. Every log call passes an event id; a new log event has a new id in `RestLogEvents` (HTTP, 1xxx) or the provider's `LogEvents` (2xxx); flag a renumbered, reused or missing id, and a `RestLogEvents` entry not mirrored in `LogEvents`. Flag any change to `IOpenUrzednikLogger`, `IOpenUrzednikTraceSource` or `IOpenUrzednikSpan` after 1.0 (frozen; new capabilities are separate interfaces), and any change to a stable name (source, span, documented tag, `error.type`, logger category, event id) outside a major. `IsEnabled` guards. No secrets or personal identifiers in logs or tags. +- ADR-0003: no `ILogger`/`ActivitySource` in Core or provider packages, except Core's internal `Telemetry/Activities` (the .NET fallback trace source, compiled on .NET only). Members of those types that other packages call through `InternalsVisibleTo` are a binary contract: flag a change or removal outside a major. Span and tag naming; HTTP tags use the OpenTelemetry names (`http.request.method`, `url.path`, `http.response.status_code`); errors set `error.type`. Activity sources are named `OpenUrzednik.`. Every log call passes an event id; a new log event has a new id in `RestLogEvents` (HTTP, 1xxx) or the provider's `LogEvents` (2xxx); flag a renumbered, reused or missing id, and a `RestLogEvents` entry not mirrored in `LogEvents`. Flag any change to `IOpenUrzednikLogger`, `IOpenUrzednikTraceSource` or `IOpenUrzednikSpan` after 1.0 (frozen; new capabilities are separate interfaces), and any change to a stable name (source, span, documented tag, `error.type`, logger category, event id) outside a major. `IsEnabled` guards. No secrets (API keys, session ids, tokens) or personal identifiers (PESEL, NIP, REGON, bank account numbers) in logs, span names or tags, error and exception messages, error metadata or `Exception.Data`; at most the last 4 characters. - ADR-0004: no new `PackageReference` in Core or provider projects, apart from netstandard2.0-only official Microsoft BCL packages (currently `System.Text.Json`, `System.Net.Http.Json`, `Microsoft.Bcl.TimeProvider`, `Microsoft.Bcl.HashCode`; any other one needs approval). `System.Diagnostics.DiagnosticSource` is allowed only in `OpenUrzednik.Diagnostics`. No `Version=` on package references (central package management). A dependency of a published library moves only by patch unless the PR says why a minor or major raise is needed; the DI package's `Microsoft.Extensions.*` keep the `net8.0` override (8.0.x), and no package gets two conditional `Include` rows. DI packages don't add a resilience handler themselves (the app chains `AddStandardResilienceHandler()`), and they translate its rejection exceptions into errors. - ADR-0005: code builds for netstandard2.0 and passes `test (net472)`. Package validation stays on with no suppressions: flag a `CompatibilitySuppressions.xml`, a `CPxxxx` in `NoWarn`, or `EnablePackageValidation` turned off (the only exception is ADR-0011's CP0006 for a new client-interface method). `OpenUrzednik.snk`, `SignAssembly`, `PublicKey` and `assemblyVersion.precision` don't change; a project doesn't turn signing off; a third-party `InternalsVisibleTo` friend has its own `Key`. `#if` kept inside helpers (polyfills in `src/Polyfills`, `src/Http/Polyfills`, `Nbp/Polyfills`, `Extensions.Logging/Polyfills`). Every build has the same public signatures for what it shares: flag a public `DateOnly` (parameter, property or return type) and any interface member that exists only on .NET; public dates are `DateTime`, with `DateOnly` overloads only as .NET-only extension methods. Internally, dates are written as `DateOnly` (an alias for `DateTime` on netstandard2.0) and compared by `DayNumber`. A .NET target that isn't the lowest one may be removed in a minor version once Microsoft ends its support; the lowest .NET target and netstandard2.0 are removed only in a major. Targets are netstandard2.0 plus the .NET LTS releases in support. -- ADR-0006: provider HTTP goes through `RestRequestExecutor` (shared source in `src/Http`); providers never call `HttpClient.SendAsync`, and no public signature uses a type from `OpenUrzednik.Http.Infrastructure`. Request and response are disposed. -- ADR-0007: public types follow the namespace layout (clients, interfaces and options in the provider root namespace, models in area namespaces, no public `.Extensions`/`.Options` namespaces in Core or providers; `OpenUrzednik.Extensions.Logging` and internal namespaces are exempt); the default constructor stays simple; nothing that shapes requests (URL builders, path enums) is public or a public constructor parameter: customization goes through `NbpOptions` and `DelegatingHandler`s; `CancellationToken cancellationToken = default` comes last in both interface and implementation. +- ADR-0006: REST/JSON provider HTTP goes through `RestRequestExecutor` (shared source in `src/Http`); those providers never call `HttpClient.SendAsync` (a SOAP or other transport follows its provider ADR), and a request URI (path or query) carrying a personal identifier must not reach `url.path`, logs, error messages or `ErrorResponseContext.RequestPath` (display path; a 400's server message must be masked or dropped), and no public signature uses a type from `OpenUrzednik.Http.Infrastructure`. Request and response are disposed. +- ADR-0007: public types follow the namespace layout (clients, interfaces and options in the provider root namespace, models in area namespaces, no public `.Extensions`/`.Options` namespaces in Core or providers; `OpenUrzednik.Extensions.Logging` and internal namespaces are exempt); the default constructor stays simple; nothing that shapes requests (URL builders, path enums) is public or a public constructor parameter: customization goes through `NbpOptions` and `DelegatingHandler`s (auth that needs protocol calls, or credentials passed as API arguments, belongs in the provider, per its provider ADR); `CancellationToken cancellationToken = default` comes last in both interface and implementation. - ADR-0008: all packages share the version of the root `version.json`; no project has a `version.json` of its own. The `paths` filter of `nuget-pre-release.yml` and `pathFilters` in `version.json` list the same package inputs. - ADR-0009: docs are in English, and English wins over Polish. If a PR changes an informational part of a README (description, "unofficial" notice, installation, quick start), `README.pl.md` changes too. A `*.pl.md` file gets no contract or reference content (API limits, data availability, errors, telemetry, configuration rules, dependencies, versioning, support), and no `*.en.md` file comes back. Renaming or removing a section of an English README updates the links to it in `README.pl.md`. -- ADR-0010: no work on GUS/KRS/MF beyond what that ADR allows. +- ADR-0010: no work on GUS/KRS/MF beyond what that ADR allows; a provider ADR answers the ADR-0010 readiness checklist, and package and namespace names follow its casing rule. - If a change contradicts an ADR and has no new ADR, report it as **blocking**. **Performance** (CLAUDE.md "Performance") @@ -36,7 +36,7 @@ First read `CLAUDE.md` and every **accepted** ADR in `docs/adr/` that relates to - Dependencies of the published (`src/`) libraries: abstractions stay at the lowest supported version, implementations at the latest patch of the chosen minor (a minor or major raise needs a stated reason). Test and tooling packages always take the latest version. A change that raises an abstraction is **blocking** until the maintainer has explicitly approved it. **Correctness** -- DTO `required` properties match real API responses. Hand-written WireMock bodies are suspect: suggest `/verify-api`. +- DTO `required` properties match real API responses. Hand-written WireMock bodies are suspect: suggest `/verify-api` (allowed only for cases the real API can't produce, such as an empty `[]` or malformed JSON). Flag committed fixtures that contain real personal identifiers, API keys, session ids or tokens. - Validators match the documented API limits. Messages use invariant formatting. - Disposal, cancellation-token propagation, `ConfigureAwait(false)` in library code. - Public behaviour changes are covered by unit tests and WireMock tests, including every error path in ADR-0002's Confirmation. diff --git a/.claude/skills/new-endpoint/SKILL.md b/.claude/skills/new-endpoint/SKILL.md index 093e607..801a91e 100644 --- a/.claude/skills/new-endpoint/SKILL.md +++ b/.claude/skills/new-endpoint/SKILL.md @@ -54,7 +54,7 @@ Work through every step. Do not skip tests. If a step conflicts with an ADR, sto - Use `Faker().WithConstantSeed()`, the DTO fakers, `StubHttpMessageHandler`, `FakeTimeProvider`. - **Mapper** tests in `MapperTest..cs`; a **validator** test per new validator. - **WireMock** (`tests/OpenUrzednik.IntegrationTests//WireMock/`): - - success with the captured real body + - success with the captured real body; if responses can contain personal data, capture from the provider's test environment or from legal-person records, replace remaining personal fields with fake values of the same format, and never commit API keys, session ids or tokens. Hand-written bodies are only for cases the real API can't produce (an empty answer `[]`, malformed JSON) - every error path in the canonical list in [ADR-0002](../../../docs/adr/0002-result-pattern-and-error-handling.md)'s Confirmation - Assert the request path and `Accept` header. - **Real API** test with `[ManualFact]` (explicit: runs with `--explicit on` or when `OPEN_URZEDNIK_INTEGRATION_TEST_ENABLED` is set). diff --git a/.claude/skills/verify-api/SKILL.md b/.claude/skills/verify-api/SKILL.md index 437027d..b9b8d96 100644 --- a/.claude/skills/verify-api/SKILL.md +++ b/.claude/skills/verify-api/SKILL.md @@ -22,7 +22,7 @@ Goal: our DTOs and test payloads must match what the API really returns, so that - A `required` property missing from any real response is a **bug**: deserialization will fail. - Report extra fields we ignore, type mismatches (number vs string, date formats) and casing. 4. **Compare with the WireMock tests** in `tests/OpenUrzednik.IntegrationTests//WireMock/`. Flag every hand-written body that differs in shape from the real one. -5. **Capture fixtures** when asked, or when fixing a mismatch: save real bodies under `tests/OpenUrzednik.IntegrationTests//Fixtures/.json` (trim long arrays to 2–3 items, keep the real structure) and load them in the WireMock tests instead of inline JSON. +5. **Capture fixtures** when asked, or when fixing a mismatch: save real bodies under `tests/OpenUrzednik.IntegrationTests//Fixtures/.json` (trim long arrays to 2–3 items, keep the real structure) and load them in the WireMock tests instead of inline JSON. If responses can contain personal data, capture from the provider's test environment or from legal-person records, replace any remaining personal fields with fake values of the same format, and never commit API keys, session ids or tokens. Hand-written bodies are only for cases the real API can't produce (an empty answer `[]`, malformed JSON). 6. **Report** a table: endpoint | status | fields in response | DTO verdict (OK / missing required / extra / type mismatch) | test payload verdict. List the limits you observed (max `last/{n}`, max date range, earliest date) next to the values used in `Validation/`. Don't change validators based on one observation without saying so: point out differences from the documented limits and let the maintainer decide. diff --git a/.github/ISSUE_TEMPLATE/provider_proposal.yml b/.github/ISSUE_TEMPLATE/provider_proposal.yml index e9f4036..c16f04d 100644 --- a/.github/ISSUE_TEMPLATE/provider_proposal.yml +++ b/.github/ISSUE_TEMPLATE/provider_proposal.yml @@ -49,3 +49,31 @@ body: description: Do czego potrzebujesz tego API? / What do you need it for? validations: required: true + - type: dropdown + id: quota + attributes: + label: Rodzaj limitu / Quota type + description: Opcjonalne. / Optional. + options: [ "Brak limitu / No limit", "Ograniczanie tempa / Throttling", "Limit dzienny lub na adres IP / Daily or per-IP quota", "Nie wiem / Unknown" ] + - type: dropdown + id: session + attributes: + label: Sesja lub logowanie / Stateful session or login + description: Opcjonalne. Czy trzeba się zalogować osobnym wywołaniem? / Optional. Is a separate login call needed? + options: [ "Tak / Yes", "Nie / No", "Nie wiem / Unknown" ] + - type: dropdown + id: personal-data + attributes: + label: Dane osobowe w odpowiedziach / Personal data in responses + description: Opcjonalne. / Optional. + options: [ "Tak / Yes", "Nie / No", "Nie wiem / Unknown" ] + - type: textarea + id: test-environment + attributes: + label: Środowisko testowe / Test environment + description: Opcjonalne. Adres lub opis. / Optional. URL or description. + - type: textarea + id: dependencies + attributes: + label: Wymagane zależności / Required dependencies + description: Opcjonalne. Np. biblioteka do archiwów, certyfikaty. / Optional. E.g. an archive library, certificates. diff --git a/CLAUDE.md b/CLAUDE.md index 0439f11..2b8a55b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -64,7 +64,7 @@ Before you say a task is done, run `dotnet build`, `dotnet test -f net10.0` and - Every log call passes an `OpenUrzednikEventId`. The shared HTTP layer's events are in `src/Http/Infrastructure/RestLogEvents.cs` (1xxx); a provider's own are 2xxx and listed, with the HTTP ones, as `public const int` in its `LogEvents` class (`NbpLogEvents`; the event name is the constant's name). A new log event gets a new id: never renumber or reuse one. The `{provider}` log property is the profile's short name (`nbp`), as in span names. - Stable within a major: source names, span names, the documented tags, `error.type`, logger categories and event ids. Log message text and properties aren't. - Adapters: `OpenUrzednik.Extensions.Logging` (logger category = the client's full type name; the event id becomes `EventId`) and `OpenUrzednik.Diagnostics` (source `OpenUrzednik.`, e.g. `NbpTelemetry.SourceName`; every span is `ActivityKind.Internal`). -- Guard `Debug` logs with `IsEnabled`, use message templates (no interpolation), and never log secrets or full personal identifiers. +- Guard `Debug` logs with `IsEnabled`, use message templates (no interpolation), and never put secrets (API keys, session ids, tokens) or personal identifiers (PESEL, NIP, REGON, bank account numbers) in log messages or properties, span names or tags, error and exception messages, error metadata or `Exception.Data`: show at most the last 4 characters, or use a template or display value. A provider ADR may require stricter masking. **Target frameworks ([ADR-0005](docs/adr/0005-target-frameworks.md))** - Every assembly, tests included, is strong-named with `OpenUrzednik.snk` (root `Directory.Build.props`; the key pair is checked in on purpose, Microsoft's guidance for OSS). Never change, replace or remove the key. A new project is signed automatically; a new `InternalsVisibleTo` gets the key from the `PublicKey` property, except a third-party friend such as `DynamicProxyGenAssembly2`, which needs its own `Key`. If test code references a new test dependency directly, that dependency must be strong-named (CS8002); if it isn't, stop and ask. The assembly version changes only with the major (NBGV `assemblyVersion.precision: major`). @@ -75,7 +75,7 @@ Before you say a task is done, run `dotnet build`, `dotnet test -f net10.0` and - .NET Framework differences seen so far: `HttpResponseMessage.Content` can be `null`; `HttpClient.Timeout` has no inner `TimeoutException`; time zones have Windows ids only (`Central European Standard Time`); `Activity` ids are hierarchical unless the app (or OpenTelemetry) switches to W3C. **HTTP ([ADR-0006](docs/adr/0006-shared-http-layer.md))** -- All provider HTTP goes through `RestRequestExecutor.GetAsync` in the shared HTTP layer (`src/Http`); providers never call `HttpClient.SendAsync`. A REST provider imports `src/Http/Http.props`, and its types stay `internal`: no public signature uses them. A provider describes itself with a `RestProviderProfile` (span name `.http.get`, message prefix, optional `MapErrorAsync` override). NBP creates its executor in `Nbp/Common/NbpConnection.cs`. Clients take an `HttpClient` and optional `NbpOptions`; they never change the `HttpClient`. +- All REST/JSON provider HTTP goes through `RestRequestExecutor` (`GetAsync`) in the shared HTTP layer (`src/Http`); those providers never call `HttpClient.SendAsync`; a SOAP or other transport follows its provider ADR. A REST provider whose request paths or queries carry personal identifiers gives the executor a display path or route template ([ADR-0006](docs/adr/0006-shared-http-layer.md)). A REST provider imports `src/Http/Http.props`, and its types stay `internal`: no public signature uses them. A provider describes itself with a `RestProviderProfile` (span name `.http.get`, message prefix, optional `MapErrorAsync` override). NBP creates its executor in `Nbp/Common/NbpConnection.cs`. Clients take an `HttpClient` and optional `NbpOptions`; they never change the `HttpClient`. - Dispose `HttpRequestMessage` and `HttpResponseMessage`. - Deserialize with source-generated `JsonTypeInfo` (`NbpJsonContext`); no reflection-based serialization. @@ -90,7 +90,7 @@ Before you say a task is done, run `dotnet build`, `dotnet test -f net10.0` and **Client API ([ADR-0007](docs/adr/0007-client-api-and-extensibility.md))** - The default case is one line (`new NbpGoldPriceClient(httpClient)`). - Parts that shape requests (the URL builders, `NbpTable`) are internal; tests replace them through an internal constructor. Each client has two public constructors: `(HttpClient)` and `(HttpClient, NbpOptions?, TimeProvider?, logger, traceSource)`. Gateways use `NbpOptions.ApiUrl` and a `DelegatingHandler`. -- Cross-cutting HTTP concerns belong in `DelegatingHandler`s. +- Cross-cutting HTTP concerns belong in `DelegatingHandler`s. Auth that needs protocol calls (a session login, token acquisition) or credentials passed as API arguments belongs in the provider, designed in its provider ADR (ADR-0010). - `CancellationToken cancellationToken = default` is the last parameter of every async method, in both interfaces and implementations. ## Performance @@ -121,7 +121,7 @@ Before you say a task is done, run `dotnet build`, `dotnet test -f net10.0` and - Test names: `Method_Condition_ExpectedResult`. Use `// Arrange / // Act / // Assert` sections. - Use deterministic data: `new Faker().WithConstantSeed()`, the DTO fakers in `tests/OpenUrzednik.Nbp.Tests/Fakes`, and `FakeTimeProvider`. - Fake HTTP in unit tests with `StubHttpMessageHandler`; mock interfaces with NSubstitute. Pass `TestContext.Current.CancellationToken` to async calls. -- WireMock tests must use **response bodies captured from the real API**, not hand-written JSON. Each error path in [ADR-0002](docs/adr/0002-result-pattern-and-error-handling.md)'s Confirmation needs a WireMock test for each client: 400 with the server's message, 401 and 403, 404, 429 with `Retry-After`, 5xx, a 200 with an empty array (`NotFoundError`, for endpoints that return an array), malformed JSON, a timeout and a connection failure. +- WireMock tests must use **response bodies captured from the real API**, not hand-written JSON. Each error path in [ADR-0002](docs/adr/0002-result-pattern-and-error-handling.md)'s Confirmation needs a WireMock test for each client: 400 with the server's message, 401 and 403, 404, 429 with `Retry-After`, 5xx, a 200 with an empty array (`NotFoundError`, for endpoints that return an array), malformed JSON, a timeout and a connection failure. If responses can contain personal data, capture them from the provider's test environment or from legal-person records, replace any remaining personal fields with fake values of the same format, and never commit API keys, session ids or tokens. Hand-written bodies are only for cases the real API can't produce (an empty answer `[]`, malformed JSON). - Tests against the real API use `[ManualFact]`/`[ManualTheory]` (explicit tests) and must never run in default CI. - Tests run on Microsoft.Testing.Platform (`global.json`), so filters are xUnit's (`--filter-class`, `--filter-method`, `--filter-trait`), not VSTest's `--filter`. Filter one project: on the whole solution, projects with no matching test make the run exit with code 8 (add `--ignore-exit-code 8`). diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index d5e46ca..f802f0e 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -20,7 +20,7 @@ Binding architecture decisions are in [`docs/adr/`](docs/adr/README.md), and cur - **`OpenUrzednikError`** is the base type for expected errors. A new error is a `sealed` class ([ADR-0011](docs/adr/0011-compatibility-policy.md)) that derives from it ([ADR-0002](docs/adr/0002-result-pattern-and-error-handling.md)), defines `public const string ErrorCode`, passes it to the base constructor and overrides `CreateException()` to return an exception derived from `OpenUrzednikException` (see `NotFoundError`). - **Provider packages** follow the patterns in `OpenUrzednik.Core`: type names, error naming and the way results are returned stay consistent. - **Document the public API** with `///` comments. `GenerateDocumentationFile` is on, so they show up in the consumer's IntelliSense. -- **Tests** must not depend on real government servers in the default CI run. Test HTTP scenarios with stubs (`StubHttpMessageHandler`) and WireMock, using responses captured from the real API. Mark tests that call the real API with `[ManualFact]` / `[ManualTheory]`. They run only on request: `dotnet test tests/OpenUrzednik.IntegrationTests --explicit on`, or with the `OPEN_URZEDNIK_INTEGRATION_TEST_ENABLED` variable set. +- **Tests** must not depend on real government servers in the default CI run. Test HTTP scenarios with stubs (`StubHttpMessageHandler`) and WireMock, using responses captured from the real API (if they can contain personal data, capture them from a test environment or from legal-person records and replace remaining personal fields with fake values of the same format; never commit API keys, session ids or tokens). Mark tests that call the real API with `[ManualFact]` / `[ManualTheory]`. They run only on request: `dotnet test tests/OpenUrzednik.IntegrationTests --explicit on`, or with the `OPEN_URZEDNIK_INTEGRATION_TEST_ENABLED` variable set. ## Reporting security vulnerabilities diff --git a/docs/adr/0002-result-pattern-and-error-handling.md b/docs/adr/0002-result-pattern-and-error-handling.md index b3c893d..9bd625e 100644 --- a/docs/adr/0002-result-pattern-and-error-handling.md +++ b/docs/adr/0002-result-pattern-and-error-handling.md @@ -1,6 +1,6 @@ --- status: accepted -date: 2026-10-29 +date: 2026-10-30 decision-makers: milten89 --- @@ -46,7 +46,7 @@ Chosen option: "All failures that can happen during normal execution are returne | Timeout: an `OperationCanceledException` while the **caller's** token is not cancelled (the request deadline, `HttpClient.Timeout`, a resilience handler's timeout) | `RequestTimeoutError`, with `Timeout` when the limit is known | `requestTimeout` | none | | `default(OpenUrzednikResult)` | `UnknownError` | `unknown` | none | -The metadata keys are public constants next to the errors that set them: `OpenUrzednikError.StatusCodeMetadataKey`, `RateLimitExceededError.RetryAfterMetadataKey`, `ValidationError.RuleNameMetadataKey`, `NameMetadataKey` and `ValueMetadataKey`. Other metadata, if any appears, is informational. +The metadata keys are public constants next to the errors that set them: `OpenUrzednikError.StatusCodeMetadataKey`, `RateLimitExceededError.RetryAfterMetadataKey`, `ValidationError.RuleNameMetadataKey`, `NameMetadataKey` and `ValueMetadataKey`. Other metadata, if any appears, is informational. A validator for a personal identifier (PESEL, NIP, REGON, bank account number) passes a masked value to `GetValidationErrorResult`, which puts it into the error's metadata ([ADR-0003](0003-telemetry-abstractions.md)). **Compatibility of the error contract** (from the first stable release): @@ -111,3 +111,5 @@ Supersedes the informal rule in `CONTRIBUTING.md`. Shared HTTP behaviour is impl * `Value` on a failure named no error; its message now lists them. These are breaking changes for preview users. + +**2026-10-30 change: masked values in validation errors** ([#109](https://github.com/milten89/OpenUrzednik.NET/issues/109)). `ValueValidator.GetValidationErrorResult` stores the validated value under `ValueMetadataKey`, so a validator for a personal identifier would copy the identifier into every error, exception and log that shows the metadata. A validator for a personal identifier must pass a masked value, under the rule in [ADR-0003](0003-telemetry-abstractions.md). NBP validates no such value, so nothing changes there. diff --git a/docs/adr/0003-telemetry-abstractions.md b/docs/adr/0003-telemetry-abstractions.md index 0b2fe5a..f1e35b4 100644 --- a/docs/adr/0003-telemetry-abstractions.md +++ b/docs/adr/0003-telemetry-abstractions.md @@ -1,6 +1,6 @@ --- status: accepted -date: 2026-10-02 +date: 2026-10-30 decision-makers: milten89 --- @@ -40,7 +40,7 @@ Chosen option: "Keep custom abstractions and ship official adapter packages". * Errors are recorded via `RecordError`/`RecordErrors` extensions, which set OpenTelemetry's `error.type` tag to `OpenUrzednikError.Code`. The `error` events `RecordErrors` adds are best-effort. * Every log entry has an `OpenUrzednikEventId`: the shared HTTP layer's events are 1xxx, a provider's own 2xxx, and each provider lists all of them as constants (`NbpLogEvents`). The `{provider}` log property is the provider's short name (`nbp`), as in span names. * Log calls at `Debug`/`Trace` are guarded with `IsEnabled`; messages use templates with named placeholders, never string interpolation. -* Never log secrets (API keys, session ids) or full personal identifiers (PESEL, NIP of natural persons) – mask them. +* Never put secrets (API keys, session ids, tokens) or personal identifiers (PESEL, NIP, REGON, bank account numbers, whoever they belong to) in log messages or properties, span names or tags, error and exception messages, error metadata or `Exception.Data`. Show at most the last 4 characters (e.g. `*******1234`), or use a template or display value instead. A provider ADR may require stricter masking. ### Consequences @@ -77,3 +77,5 @@ Rules from 1.0 on: * **Cross-package internals:** on .NET, Diagnostics and the providers call Core's internal `SharedActivityTraceSource` and `ActivitySpan` through `InternalsVisibleTo`. Package dependencies are minimums ([ADR-0008](0008-branching-versioning-and-release.md)), so an app may pair a newer Core with older packages: within a major, those members are only added to, never changed or removed. * **Sources are unversioned:** `ActivitySource` gets no version; the stability rule above takes its place. * **Metrics are out of scope.** The library records no metrics; `HttpClient`'s own metrics cover requests. Adding metrics later needs a new decision. + +**2026-10-30 change: no secrets or personal identifiers in telemetry or errors** ([#109](https://github.com/milten89/OpenUrzednik.NET/issues/109)). The rule only named logs and exempted the NIP of a legal person. NBP handles no such data, but the MF and GUS providers will, and the shared executor already writes request paths into `url.path`, logs and the messages of the errors it returns (timeout, connection, serialization and status errors all carry the request path), so an identifier in a path would leak through every channel. The rule now covers all of them and every holder of an identifier, and allows at most the last 4 characters. A provider whose paths carry identifiers also needs a display path ([ADR-0006](0006-shared-http-layer.md)). diff --git a/docs/adr/0006-shared-http-layer.md b/docs/adr/0006-shared-http-layer.md index b603d21..376f218 100644 --- a/docs/adr/0006-shared-http-layer.md +++ b/docs/adr/0006-shared-http-layer.md @@ -1,6 +1,6 @@ --- status: accepted -date: 2026-10-29 +date: 2026-10-30 decision-makers: milten89 --- @@ -36,6 +36,7 @@ Chosen option: "Shared source files compiled into each provider", because the la ### Provider-author reference * A provider describes itself with a `RestProviderProfile(name, displayName, mapErrorAsync)`. `name` builds the span name `.http.get` and is the `{provider}` log property; `displayName` goes into error messages. Every log call of the layer has an event id from `RestLogEvents` (1xxx), which the provider's public log-events class mirrors ([ADR-0003](0003-telemetry-abstractions.md)). The provider passes its `ActivitySource` name to `OpenUrzednikTelemetry`, which uses it when the app gives no trace source (on the .NET targets); for that, the provider is added to Core's .NET-only `InternalsVisibleTo`. +* A REST provider whose request paths or queries carry personal identifiers ([ADR-0003](0003-telemetry-abstractions.md)) must give the executor a display path or route template (e.g. `/api/search/nip/{nip}`). The executor uses the absolute `RequestUri` in every status-error message, in the error log and in `ErrorResponseContext.RequestPath` (passed to `MapErrorAsync`), so the display value must replace the whole request URI (path and query) in `url.path`, logs, error messages and `ErrorResponseContext.RequestPath`. The executor API for it is added with the first such provider (an additive change). Such a provider must also mask or drop a 400's server message, which may echo the identifier. * `RestRequestExecutor` resolves request paths against an absolute base address and never changes the `HttpClient`, so one `HttpClient` can serve several providers. The request deadline (the timeout passed to it, or `HttpClient.Timeout` when none is set) covers reading the body too. * `GetAsync` **returns** as errors: every non-success status, an empty, malformed or undecodable JSON body, network failures (`HttpRequestException`, or an `IOException` while reading the body) and timeouts while sending or reading a success body (an `OperationCanceledException` while the caller's token isn't cancelled). When the error body can't be read before the deadline, `ReadMessageAsync()` returns `null` and the error is still returned. * **Thrown**: @@ -53,7 +54,7 @@ Chosen option: "Shared source files compiled into each provider", because the la ### Confirmation -Provider packages contain no direct `HttpClient.SendAsync` calls; all HTTP goes through `RestRequestExecutor`. No public signature of a provider uses a type from `OpenUrzednik.Http.Infrastructure`. The `reviewer` agent checks both. +REST/JSON provider packages contain no direct `HttpClient.SendAsync` calls; all their HTTP goes through `RestRequestExecutor`. A SOAP or other transport follows its provider ADR ([ADR-0010](0010-provider-readiness-gate.md)). No public signature of a provider uses a type from `OpenUrzednik.Http.Infrastructure`. The `reviewer` agent checks both. ## Pros and Cons of the Options @@ -80,3 +81,5 @@ Provider packages contain no direct `HttpClient.SendAsync` calls; all HTTP goes **2026-10-17 change: one version for all packages.** All packages now share one version from the root `version.json` ([ADR-0008](0008-branching-versioning-and-release.md)), so a provider no longer path-filters `OpenUrzednik.Http`, and an HTTP change no longer re-versions only the REST providers. The driver about needless version bumps, the matching consequence and the `pathFilters` check in Confirmation were removed. The version arguments under "Pros and Cons of the Options" no longer tell the options apart. **2026-10-29 change: shared source instead of a package** ([#85](https://github.com/milten89/OpenUrzednik.NET/issues/85)). The maintainer chose shared source before 1.0. The `OpenUrzednik.Http` package would have frozen four public types meant only for provider authors, while no application needs them, and with one version for all packages (2026-10-17) a separate package no longer saved releases. The code moved from `src/OpenUrzednik.Http` to `src/Http`, the types became `internal`, and `OpenUrzednik.Nbp` compiles them in and gained the netstandard2.0 `System.Net.Http.Json` reference. The package's README was removed; its provider-author reference is now the section above, which also corrects its claim that only caller cancellation throws. The package is no longer published and gets deprecated on nuget.org ([#116](https://github.com/milten89/OpenUrzednik.NET/issues/116)). The Decision Outcome, drivers, consequences, Confirmation and the pros and cons were rewritten for the chosen option. + +**2026-10-30 change: scope of the executor rule and display paths** ([#109](https://github.com/milten89/OpenUrzednik.NET/issues/109)). The Confirmation said all provider HTTP goes through the executor, which cannot hold for SOAP (GUS), so it now covers REST/JSON providers and leaves other transports to their provider ADR. The ADR also says that a request URI (path or query) carrying a personal identifier must not reach `url.path`, logs, error messages or `ErrorResponseContext.RequestPath`, because the executor uses the absolute `RequestUri` there (`RestRequestExecutor.cs:277`); a 400's server message may echo the identifier too. The executor gets a display-path parameter with the first provider that needs one. diff --git a/docs/adr/0007-client-api-and-extensibility.md b/docs/adr/0007-client-api-and-extensibility.md index d260d61..0dd7d1b 100644 --- a/docs/adr/0007-client-api-and-extensibility.md +++ b/docs/adr/0007-client-api-and-extensibility.md @@ -1,6 +1,6 @@ --- status: accepted -date: 2026-10-29 +date: 2026-10-30 decision-makers: milten89 --- @@ -30,7 +30,7 @@ Chosen option: "Simple constructors; request shaping internal, HTTP customizatio * **Default path:** `new NbpGoldPriceClient(httpClient)` works with defaults (base URL from `NbpOptions.DefaultApiUrl` when `HttpClient.BaseAddress` is not set). DI packages give `services.AddOpenUrzednikNbp()`. * **Configuration:** a plain options object (`NbpOptions`: base URL, timeout, …) – no `IOptions` in provider packages ([ADR-0004](0004-dependency-policy.md)). * **Request shaping is internal:** the URL builders (`INbpUrlBuilderFactory`, `INbpUrlBuilder`, their defaults and `NbpTable`) are internal. Tests replace them through an internal constructor, and NSubstitute mocks them through `InternalsVisibleTo("DynamicProxyGenAssembly2")`. A gateway's base address goes into `NbpOptions.ApiUrl`; renamed paths and extra query parameters are rewritten in a `DelegatingHandler` (the NBP README shows one). -* **Cross-cutting HTTP concerns** (headers, auth, proxies, retries, path rewriting) are done with `DelegatingHandler`s / `HttpClient` configuration, not with library-specific hooks. +* **Cross-cutting HTTP concerns** (headers, auth, proxies, retries, path rewriting) are done with `DelegatingHandler`s / `HttpClient` configuration, not with library-specific hooks. Auth that needs protocol calls (a session login such as GUS `Zaloguj` returning a session id, token acquisition) or credentials passed as API arguments belongs in the provider and is designed in its provider ADR ([ADR-0010](0010-provider-readiness-gate.md)); whether a static API key is a handler or a provider option is decided there too. * Separate clients per API area (currency, tables, gold) remain; a facade may be added later without breaking changes. * **Namespace layout:** what an application uses to call a provider is in the provider's root namespace (`OpenUrzednik.Nbp`: the clients, their interfaces, `NbpOptions`, `NbpTelemetry`), so one `using` is enough for the common case; the clients' files live in the project root. Models and enums stay in the namespace of the area that defines them (`OpenUrzednik.Nbp.Currency`, `.Table`, `.Gold`; e.g. `TableType` is in `.Table`, also for the currency client). In Core, the result types and their extensions on `Task<…>` (`EnsureSuccessAsync`, `MapAsync`, `BindAsync`) are in `OpenUrzednik.Core`, and operations on a result are instance methods (`EnsureSuccess`, `And`, `Map`, `Bind`, `Match`); errors, exceptions, telemetry and validation have their own namespaces (`.Errors`, `.Exceptions`, `.Telemetry`, `.Validation`), with extension methods next to the type they extend (`RecordError` in `.Telemetry`). No public namespace of Core or a provider is named after a kind of member (`.Extensions`, `.Options`); package names modelled on `Microsoft.Extensions.*` (`OpenUrzednik.Extensions.Logging`) and internal namespaces are exempt. DI registration stays in `Microsoft.Extensions.DependencyInjection`, as is usual for `IServiceCollection` extensions. * **Constructors:** each client has two public constructors, one with only the `HttpClient` and one with the optional `NbpOptions`, `TimeProvider`, logger and trace source. DI packages create the clients through the second one in a factory; a bare `AddHttpClient()` isn't supported, because the typed-client activator finds no constructor it can use. @@ -51,3 +51,5 @@ README shows the one-line default usage and the gateway handler, with only the n **2026-10-29 change: request shaping is internal** ([#86](https://github.com/milten89/OpenUrzednik.NET/issues/86)). Before, components that shaped requests were public interfaces with public defaults, passed as optional constructor parameters (`INbpUrlBuilderFactory` for custom base URLs, paths or extra query parameters), and the DI package used a registered `INbpUrlBuilderFactory`. The maintainer made them internal before 1.0: every such interface would have been a long-term compatibility commitment, and a `DelegatingHandler` plus `NbpOptions.ApiUrl` covers the same gateway cases. The `urlBuilderFactory` constructor parameter and the DI pickup were removed, and the title ("simple defaults, replaceable internals"), the chosen option, drivers, consequences and Confirmation were rewritten. Both public constructors stay. **2026-10-29 change: namespace layout** ([#87](https://github.com/milten89/OpenUrzednik.NET/issues/87)). Before, the clients, their interfaces and `NbpOptions` were in the area namespaces (`OpenUrzednik.Nbp.Currency`, `.Table`, `.Gold`, `.Options`), so the one-line default needed a `using` per area, and Core's `EnsureSuccess`, `And`, `EnsureSuccessAsync`, `MapAsync`, `BindAsync`, `RecordError` and `RecordErrors` were extension methods in `OpenUrzednik.Core.Extensions`. The maintainer settled the layout above before 1.0, when moving types is still allowed. The clients' logger category (their full type name) changed with them, e.g. to `OpenUrzednik.Nbp.NbpGoldPriceClient`; a filter on the `OpenUrzednik.Nbp` prefix still matches, but filters on `OpenUrzednik.Nbp.Currency`, `.Table` or `.Gold` no longer match the clients and change their log level silently; use the type name or `OpenUrzednik.Nbp`. + +**2026-10-30 change: auth that a handler can't do** ([#109](https://github.com/milten89/OpenUrzednik.NET/issues/109)). The rule named auth as a handler concern, which fits a static header but not a session login that is itself an API call, or credentials that are arguments of the API (GUS BIR). Those stay in the provider, and each provider ADR decides how, including for a static API key. diff --git a/docs/adr/0010-provider-readiness-gate.md b/docs/adr/0010-provider-readiness-gate.md index b4b82c3..f9fd902 100644 --- a/docs/adr/0010-provider-readiness-gate.md +++ b/docs/adr/0010-provider-readiness-gate.md @@ -1,6 +1,6 @@ --- status: accepted -date: 2026-10-17 +date: 2026-10-30 decision-makers: milten89 --- @@ -15,6 +15,18 @@ The repository contains skeletons for GUS, KRS and MF, but the shared framework * No work on new providers (including the existing GUS/KRS/MF skeletons) until the gate opens: the first stable release of `OpenUrzednik.Nbp`, with no open P0 or P1 bug. An open P0 or P1 bug is an open issue labelled `bug` with Priority P0 or P1 in the [project](https://github.com/users/milten89/projects/2). * NBP is the reference implementation. New providers copy its structure, conventions and test approach. * The choice and order of the next providers is decided afterwards, in a separate ADR. +* Every provider ADR answers this readiness checklist: + * protocol and HTTP methods; + * authentication and sessions; + * quota type (throttling, or a daily or per-IP quota) and retry advice; + * personal data in requests and responses, and its masking ([ADR-0003](0003-telemetry-abstractions.md), [ADR-0006](0006-shared-http-layer.md)); + * fixtures, and whether a test environment exists; + * terms of use and attribution; + * the legal meaning of outputs; + * partial results of batch calls; + * extra dependencies ([ADR-0004](0004-dependency-policy.md)); + * the package and namespace name. +* Package and namespace names use .NET casing: two-letter acronyms in capitals (`MF`, not `Mf`), longer ones in Pascal case. Whether a package covers one API (e.g. `OpenUrzednik.BialaLista`) or an institution and dataset (e.g. `OpenUrzednik.MF.WhiteList`) is decided in each provider ADR. Renaming a released package changes its `ActivitySource` name, which is part of the telemetry contract ([ADR-0003](0003-telemetry-abstractions.md)), so the name is decided before the first release. The `Gus`, `Krs` and `Mf` skeletons keep their names until their provider ADR. * Skeleton packages are not published (`IsPackable=false`) until they contain a usable client. The empty previews of Gus, Krs and Mf published before that rule (up to `1.0.13-preview`) are deprecated and unlisted on nuget.org. A provider's first real version must sort above `1.0.13-preview.gde7b6f8597`, the highest of them. Lockstep versions from `1.1.0-preview` on do; `1.0.0` to `1.0.13` don't. A new provider is a feature, so it ships in a minor release, 1.1.0 or later ([ADR-0008](0008-branching-versioning-and-release.md)). ### Consequences @@ -29,3 +41,5 @@ PRs touching `src/OpenUrzednik.Gus|Krs|Mf` are rejected unless they implement th ## More Information **2026-10-17 change: an objective gate** ([#75](https://github.com/milten89/OpenUrzednik.NET/issues/75)). The gate said "until `OpenUrzednik.Nbp` is complete", measured by the design issues in `docs/BACKLOG.md`. That file is now a closed record (work is tracked in issues), and its last open P0–P2 item was a documentation sentence, so the condition no longer said anything. The maintainer replaced it with an event anyone can check: the first stable release of `OpenUrzednik.Nbp`, with no open P0 or P1 bug. The skeletons' empty previews (52 versions each, published before `IsPackable=false`) were deprecated and unlisted, and the READMEs, the Core README and CONTRIBUTING now say the providers are planned, not started. + +**2026-10-30 change: readiness checklist and naming** ([#109](https://github.com/milten89/OpenUrzednik.NET/issues/109)). The audit of the documentation found that the first real providers would hit questions no ADR answers. The checklist above makes each provider ADR answer them before any code. The MF White List shows what the checklist surfaces (to verify in its provider ADR): a daily quota that blocks the caller until midnight, not a throttle; `requestId` and `requestDateTime` in the answer as the proof of a check; partial results in batch calls; a flat file in 7z format; a test host (`wl-test.mf.gov.pl`). Each of these touches the error model, the executor, the fixtures or the dependencies. The naming rule was decided at the same time, because the skeleton `Mf` breaks the .NET acronym rule and a rename after release would change the `ActivitySource` name.