refactor(sql): .Browser references .Driver — the same layering violation #512 removed from MQTT
#526
Assignees
dohertj2 (dohertj2)
Clear assignees
dohertj2 (dohertj2)
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: dohertj2/lmxopcua#526
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser.csprojcarries aProjectReferenceto the runtimeDriver.Sqlproject in addition to.Contracts— the same layering violation Gitea #512 just removedfrom the MQTT browser, in a second driver.
Found while closing #512, not by a sweep. #512's
.Browsercsproj comment claimed the MQTT browserwas why
ZB.MOM.WW.OtOpcUa.Core.dllreached the AdminUI's build/deploy graph. After the MQTT fix,Driver.Mqtt.dll,Polly.CoreandSerilogall disappeared fromAdminUI/bin— butCore.dlldidnot, because
Driver.Sql.Browserstill pulls it in. So the old comment's attribution was wrong, andthis is the remaining cause.
Worse than #512 in one respect, better in another. Unlike the MQTT case it carries no comment at
all justifying the exception — a reader has no way to know whether it is deliberate. But the shared
surface is smaller: MQTT's exception existed to avoid forking security-relevant TLS/CA-pin logic, and it
is not yet established that Sql's has an equivalent justification.
Do this first: find out what
Sql.Browseractually uses fromDriver.Sql. The fix follows fromthat and only then:
.Contracts,which is the cheap fix and needs no new project;
⇒ a leaf project mirroring
ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Transport;Guard against the wrong fix, exactly as #512 warned: do not resolve this by copying the shared code
into the browser. If it touches credentials or TLS, a second copy is a silent security divergence, and
that is the hazard the exception was taken to avoid in the first place.
Acceptance.
Driver.Sql.Browserreferences.Contracts(plus any new leaf) only; the Sql browsesuite still passes;
ZB.MOM.WW.OtOpcUa.Core.dllis absent from the AdminUI's output. ExtendMqttBrowserLayeringTests' shape to Sql — or generalise it — so every bespoke browser is guarded ratherthan just the one that had an issue filed.
Done — master
803f4e6e(merged9fa701b1).ISqlDialect+SqlServerDialectmoved to a new leaf projectZB.MOM.WW.OtOpcUa.Driver.Sql.Dialect, referenced by the runtime driver and the browser alike. Assembly name is.Dialect; the namespace staysZB.MOM.WW.OtOpcUa.Driver.Sql, matching theDriver.Mqtt.Transportprecedent — the extraction cost zerousingchanges anywhere.Acceptance, measured in both directions on a clean AdminUI build (this issue exists because #512 asserted the same thing without measuring):
Two corrections to the issue as written.
It says the reference "carries no comment at all justifying the exception". It carried a detailed one from the original scaffold commit (
3b75e1ac), naming the design doc and asking that nobody "fix" it without reading that context. That doesn't make the reference right, but the "a reader has no way to know whether it is deliberate" argument doesn't hold, and the design doc's project table recorded it as deliberate too — both are now corrected.Second, it was the
.Transportshape, not the cheap one.ISqlDialectexposesDbProviderFactoryandSqlServerDialectreturnsSqlClientFactory.Instance, so.Contracts— deliberately provider-free — cannot hold it. The old comment's premise was sound and survives: the dialect's catalog SQL is the browse engine, so it must be shared rather than duplicated. What was wrong was only the conclusion that sharing required a reference to the runtime driver.Two dependencies the cut edge surfaced as the implicit ones they were. The AdminUI itself read
SqlDriver.DriverTypeNamein three places (tag-editor map, tag-config validator, Sql address picker); all now readDriverTypeNames.Sql, which is exactly what that constant resolves to and exactly the repointSqlDriverBrowser's own comment had deferred until a factory existed for the shared constant. AndSqlDriverFormContractTestsgenuinely needs the runtime driver — it asserts the AdminUI form's emitted JSON against the factory that reads it — soAdminUI.Testsnow references it explicitly. A test project referencing a runtime driver is fine; tests are not in the AdminUI's deploy graph, which is the whole point.Guard: generalised, as asked.
DriverBrowserLayeringTests(Commons.Tests) walks the csproj graph of every*.Browserproject in the tree, transitively. It reads csproj files rather than loaded assemblies because the cost being prevented is a build-graph cost — the compiler emits no assembly reference for an unusedProjectReference, soMqttBrowserLayeringTests' assembly-reference approach cannot see that case. The two are complementary and now point at each other.Falsified three ways, each red then green again: the direct reference re-added to the browser; the same reference one hop out via
.Dialect(proving the walk is really transitive); and both controls — browsers-discovered and references-parsed — which exist so an empty discovery cannot pass the absence assertions vacuously.