From 8dd74121c345273c6eb4519c8c7f01ae138a1f35 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sun, 17 May 2026 00:04:56 -0400 Subject: [PATCH] =?UTF-8?q?fix(inbound-api):=20resolve=20InboundAPI-012=20?= =?UTF-8?q?=E2=80=94=20move=20ParameterDefinition=20POCO=20to=20ScadaLink.?= =?UTF-8?q?Commons=20(Types/InboundApi)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- code-reviews/InboundAPI/findings.md | 37 ++++++++++++------- .../Types/InboundApi/ParameterDefinition.cs | 15 ++++++++ .../ParameterValidator.cs | 11 +----- 3 files changed, 39 insertions(+), 24 deletions(-) create mode 100644 src/ScadaLink.Commons/Types/InboundApi/ParameterDefinition.cs diff --git a/code-reviews/InboundAPI/findings.md b/code-reviews/InboundAPI/findings.md index 92ff844..1109a47 100644 --- a/code-reviews/InboundAPI/findings.md +++ b/code-reviews/InboundAPI/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-16 | | Reviewer | claude-agent | | Commit reviewed | `9c60592` | -| Open findings | 1 | +| Open findings | 0 | ## Summary @@ -500,7 +500,7 @@ indistinguishable contract. |--|--| | Severity | Low | | Category | Code organization & conventions | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.InboundAPI/ParameterValidator.cs:128-133` | **Description** @@ -521,18 +521,27 @@ components that work with method definitions. **Resolution** -_Unresolved — left Open; the fix crosses this module's editable boundary._ -Re-triage 2026-05-16: confirmed against the source — `ParameterDefinition` -(`ParameterValidator.cs:128-133`) is indeed an API-contract-shaped POCO declared in -the component project. However the recommended fix is to **create the type in -`ScadaLink.Commons`** (and update the validator plus any other consumers to reference -it), which edits files outside this module's editable scope -(`src/ScadaLink.InboundAPI`, `tests/`, this file only). It also touches a shared -contract type: a Commons namespace placement and a likely-paired return-definition -type are a cross-component code-organization decision. **Surface to the design/Commons -owner:** add `ParameterDefinition` (and a return-definition counterpart) under a -`ScadaLink.Commons` InboundApi types namespace, then repoint `ParameterValidator` and -any other consumers at it. +Resolved 2026-05-16 (commit ``): root cause confirmed against the source — +`ParameterDefinition` was a persistence-ignorant, API-contract-shaped POCO (the +deserialized form of the `ApiMethod.ParameterDefinitions` configuration-database +column) declared inside the component project, contrary to CLAUDE.md's +code-organization rule that such shared contract types live in `ScadaLink.Commons`. +The type was moved to `src/ScadaLink.Commons/Types/InboundApi/ParameterDefinition.cs` +(namespace `ScadaLink.Commons.Types.InboundApi`) — placed under `Types/` with an +`InboundApi` domain subfolder, matching the existing `Types/Scripts/` precedent, since +the column itself is the persisted form and this type is its deserialized contract +shape (not an EF-mapped entity). It remains a pure POCO with no EF attributes and no +behaviour. `ParameterValidator` now imports the moved type via a `using +ScadaLink.Commons.Types.InboundApi;` directive; a tree-wide search confirmed +`ParameterValidator.cs` was the type's only declaration and only direct consumer (all +other `ParameterDefinition*` matches are the unrelated `ParameterDefinitions` string +property). No return-definition type exists in the codebase — only a `ReturnDefinition` +string column — so none was invented. No behavioural change, so no new runtime +regression test: this is a compile-level type move, and the existing 52 +`ScadaLink.InboundAPI.Tests` (including the `ParameterValidator` suite) act as the +regression guard. `dotnet test` for `ScadaLink.InboundAPI.Tests` (52 passed) and +`ScadaLink.Commons.Tests` (226 passed) are green; `dotnet build ScadaLink.slnx` +succeeds with 0 warnings / 0 errors. ### InboundAPI-013 — `ApiKeyValidationResult.NotFound` factory returns HTTP 400, contradicting its name diff --git a/src/ScadaLink.Commons/Types/InboundApi/ParameterDefinition.cs b/src/ScadaLink.Commons/Types/InboundApi/ParameterDefinition.cs new file mode 100644 index 0000000..0353a92 --- /dev/null +++ b/src/ScadaLink.Commons/Types/InboundApi/ParameterDefinition.cs @@ -0,0 +1,15 @@ +namespace ScadaLink.Commons.Types.InboundApi; + +/// +/// Defines a single parameter in an inbound API method's parameter definitions. +/// This is the deserialized, persistence-ignorant form of the JSON stored in +/// ApiMethod.ParameterDefinitions and describes the public API contract, +/// so it is shared by every component that reads or produces method parameter +/// definitions (Inbound API, Central UI method editor, CLI, Management Service). +/// +public class ParameterDefinition +{ + public string Name { get; set; } = string.Empty; + public string Type { get; set; } = "String"; + public bool Required { get; set; } = true; +} diff --git a/src/ScadaLink.InboundAPI/ParameterValidator.cs b/src/ScadaLink.InboundAPI/ParameterValidator.cs index dc6269c..3f85631 100644 --- a/src/ScadaLink.InboundAPI/ParameterValidator.cs +++ b/src/ScadaLink.InboundAPI/ParameterValidator.cs @@ -1,4 +1,5 @@ using System.Text.Json; +using ScadaLink.Commons.Types.InboundApi; namespace ScadaLink.InboundAPI; @@ -142,16 +143,6 @@ public static class ParameterValidator } } -/// -/// Defines a parameter in a method's parameter definitions. -/// -public class ParameterDefinition -{ - public string Name { get; set; } = string.Empty; - public string Type { get; set; } = "String"; - public bool Required { get; set; } = true; -} - /// /// Result of parameter validation. ///