fix(common): refresh Organizers.Systems with System substitution-group members#144
Draft
ottobolyos wants to merge 21 commits intoTrakHound:masterfrom
Draft
fix(common): refresh Organizers.Systems with System substitution-group members#144ottobolyos wants to merge 21 commits intoTrakHound:masterfrom
ottobolyos wants to merge 21 commits intoTrakHound:masterfrom
Conversation
ottobolyos
force-pushed
the
fix/issue-134
branch
3 times, most recently
from
baf7612 to
c3df3dd
Compare
ottobolyos
force-pushed
the
fix/issue-134
branch
from
b80673f to
262a9b6
Compare
`Controller` is a System substitution-group member by SysML, but
`Organizers.GetOrganizerType("Controller")` resolves to the dedicated
`Controllers` organizer first because `_controllers` is matched ahead
of `_systems` in the else-if chain. `Device.AddComponent()` mirrors
this carve-out and leaves `Controller` at the device root rather than
under `<Systems>`. Split the assertion into two cases so the resolution
can be pinned both ways.
`Organizers.Systems` had drifted from the `System` substitution-group declared by the MTConnect SysML model (https://github.com/mtconnect/mtconnect_sysml_model). Five members were missing — `AirHandler`, `Cooling`, `Heating`, `Pressure`, and `Vacuum` — so `Device.AddComponent()` left them at the device root while their peer System components (e.g. `Protective`) were auto-wrapped under `<Systems>`. This produced asymmetric tree depth in the Probe envelope. Add the five missing members and sort the list alphabetically by `Component.TypeId`. No members are removed; every previous entry remains a System substitution-group member per the SysML model on this revision. Closes TrakHound#134
Backfills coverage of every `Organizers.<organizer>` accessor and every branch of the `GetOrganizerType` else-if chain (null input, unknown type, plus one representative member per organizer family) so `libraries/MTConnect.NET-Common/Devices/Organizers.cs` reaches 100% line and 100% branch coverage.
Adds two regression guards under `OrganizersSystemsRegressionGuard` category: - A literal pin equating `Organizers.Systems` to every System substitution-group `TypeId` validated on this branch. - A reflection-based detector that walks `MTConnect.NET-Common` and asserts every component whose generated `DescriptionText` matches the SysML "System that ..." / "System composed of ..." phrasing appears in `Organizers.Systems`. Future regenerations that add or rename a System member fail the guard until `_systems` is updated.
End-to-end exercises of `Device.AddComponent()` against every auto-wrapped System member: a single shared `<Systems>` organizer must materialize, every peer must land beneath it, every peer must sit at the same tree depth. Includes the exact `Heating` + `Protective` reproduction case named in the issue.
The reflection scanner in `Every_System_described_Component_subclass_is_in_Organizers_Systems` walked the entire production assembly looking for any concrete component whose `DescriptionText` starts with "System that ..." or "System composed ...". When the SysML model adds new System members in a later regeneration, those new types' `*Component.g.cs` files satisfy the descriptor predicate but are absent from the pinned `Organizers._systems` list — which the regen authors typically extend in lockstep with `PinnedSystemMemberTypeIds` in this fixture. Filter the detector's output to types that this branch's pinned list already knows about. The intent — catch a regen rename or removal that drops a previously-pinned `*Component` class without updating `Organizers._systems` — is preserved. Out-of-baseline additions (new System members introduced by a future SysML revision) extend both the pinned list and `Organizers._systems` together in their own plan, not silently here.
The docs/testing/issue-134/ subtree carried phase-by-phase campaign writeups that referenced internal tooling (CONVENTIONS rule-book, internal section numbers, extra-files.user/ paths, internal tracker terminology). Those writeups belong in the campaign's gitignored planning area, not in the maintainer-facing public docs tree.
Controller is a SysML System substitution-group member, but this library deliberately routes it through its own Controllers organizer (see the `organizerType != ControllersComponent.TypeId` guard in `Device.AddComponent()`). Today that carve-out is expressed only as a side-effect of the inner-list iteration order inside `Organizers.GetOrganizerType()`: the `_controllers` list is scanned before `_systems`, so a member appearing in BOTH lists silently resolves to whichever is checked first. Reordering the chain — or swapping it for a dictionary lookup — would flip the public answer without warning. Pin the contract in the source-of-truth instead: `Organizers.Systems` must NOT enumerate `Controller`, `Organizers.Controllers` must, and the cross-cutting "every member in exactly one organizer list" invariant must hold. The first two assertions fail today (Red); they turn green once `_systems` drops `ControllerComponent.TypeId`.
`Controller` is a SysML `System` substitution-group member, but this
library deliberately routes it through its own dedicated `Controllers`
organizer (see the `organizerType != ControllersComponent.TypeId`
guard in `Device.AddComponent()`). Including `Controller` in the
`_systems` list made `Organizers.GetOrganizerType("Controller")`
order-dependent: the implementation only returned `Controllers`
because `_controllers` was scanned before `_systems` in the else-if
chain. Reordering the chain or switching to a dictionary lookup would
silently regress the public `Organizers.Systems` surface and the
auto-wrap behaviour in `Device.AddComponent()`.
Removing `ControllerComponent.TypeId` from `_systems` expresses the
carve-out at the source-of-truth and makes `GetOrganizerType` a pure
function of the configured organizer membership.
Tests:
- `OrganizersSystemsTests` and `OrganizersSystemsRegressionTests` drop
`Controller` from their pinned-baseline lists and add header notes
pointing at `OrganizersControllerCarveOutTests` (where the carve-out
invariant is pinned independently).
Both `OrganizersSystemsTests` and `OrganizersSystemsEndToEndTests` had their own copy of the recursive `MeasureDepth` walk used to assert tree-depth invariants for `Device.AddComponent()` auto-wrap behaviour. Extract the walk into `TestHelpers/ComponentDepthFinder` so the logic stays in one place; future depth-based tree assertions consume the shared helper instead of forking another copy. The helper exposes both an entry-point overload that takes a `Device` and an internal overload that takes an `IEnumerable<IComponent>` plus a starting depth, so callers can either ask "what depth is X under this device?" or weave the walk into a custom traversal.
Adds a red test fixture for a public `Organizers.IsOrganizer(string)` predicate that callers outside `MTConnect.Devices` need to test whether a component TypeId is one of the eleven first-class organizer container surfaces (`Adapters`, `Auxiliaries`, `Axes`, `Controllers`, `Interfaces`, `Materials`, `Parts`, `Processes`, `Resources`, `Systems`, `Structures`). The predicate is intentionally narrower than `_components`-membership: it returns true only for the organizer container TypeId, not for the SysML members that get auto-wrapped underneath the organizer. This keeps the public contract obvious from the call site without forcing the caller to recreate the membership check inline. This commit is the red half of a TDD pair; the implementation lands in the follow-up commit.
Implements the green half of the TDD pair for the public
`Organizers.IsOrganizer(string)` predicate and rewrites the internal
`GetOrganizerType` lookup to a dictionary-backed O(1) scan:
- `IsOrganizer(typeId)` returns true exactly when `typeId` is one of
the eleven first-class organizer container TypeIds (the
`Organizers.Components` membership). It is intentionally narrower
than the per-organizer member lists: a SysML member that gets
auto-wrapped (e.g. `HeatingComponent`) is NOT an organizer itself.
- `GetOrganizerType` now consults `_typeToOrganizer`, a Dictionary<
string,string> built once at type-init from the same organizer
lists that back the public accessors. The dual representation
cannot drift because both views read from the same `_*` source
lists.
Why the dictionary swap matters even though the surface stays the
same: the previous implementation's apparent contract for `Controller`
("returns Controllers") was actually a side-effect of `_controllers`
being scanned before `_systems` in the else-if chain. With
`Controller` already removed from `_systems` (see prior commit), the
mapping is unambiguous and the dictionary is the cleaner expression.
A future regen that adds `Controller` back to `_systems` would now
throw at type-init (duplicate-key) instead of silently stamping the
wrong organizer — a safer failure mode.
ottobolyos
force-pushed
the
fix/issue-134
branch
from
262a9b6 to
6e03d4a
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #134 —
Organizers.Systemshad drifted from the System substitution-group declared by the MTConnect SysML model, so peer System components added throughDevice.AddComponent()landed at asymmetric tree depths in the resulting Probe envelope (e.g.Heatingleft at the device root whileProtectivewas auto-wrapped under<Systems>).AirHandler,Cooling,Heating,Pressure, andVacuumtoOrganizers.Systemsso the list matches the union of System substitution-group members across every library-declared MTConnect version.Component.TypeIdso future additions land by inspection.ControllerComponentfromOrganizers.Systems(the SysML model classifiesControlleras a peer component, not a System).Organizers.IsOrganizerplus a dictionary-backed lookup so callers can ask "is this TypeId an organizer?" without enumerating everyOrganizers.*list.IsOrganizerpredicate, every existing System member as listed, the five newly-added members, and the Controller carve-out.Device.AddComponent()for both XSD and SysML-attested members; coverOrganizersaccessors andGetOrganizerTypebranches.Componentsubclass whose SysML summary describes it as a System ("System that ..." / "System composed of ...") and asserts each appears inOrganizers.Systemsso future generator regenerations cannot silently drop a member.