Skip to content

fix(data): model convert silently drops relationships and picklist columns - #190

Draft
david-hudec-networg wants to merge 2 commits into
TALXIS:masterfrom
david-hudec-networg:fix/data-model-convert-loses-model-content
Draft

fix(data): model convert silently drops relationships and picklist columns#190
david-hudec-networg wants to merge 2 commits into
TALXIS:masterfrom
david-hudec-networg:fix/data-model-convert-loses-model-content

Conversation

@david-hudec-networg

@david-hudec-networg david-hudec-networg commented Aug 31, 2026

Copy link
Copy Markdown

Two defects in DataModelConverter. Both are silent: output stays well-formed while losing model content, so nothing surfaces until you diff against the source declarations. Found while building a layered ERD across a product base and three customer solutions.

1. N:1 relationships deduped on the table pair, not the column

DataModelConverterService.cs:492 compares only LeftSideTable and RighSideTable, so every relationship after the first between the same pair is dropped.

talxis_project has four separate lookups to account — building authority, city, city district, architectural studio. One edge was emitted. All four columns still rendered, because columns are emitted separately from relationships, so the diagram understated the model without looking broken.

Fix: include LeftSideRow in the key. Table.GetOrCreateRow caches by name, so two relationships through the same column share one TableRow instance and reference equality holds. That is also what dbdiagram constrains — the endpoint pair, not the table pair — so genuinely duplicated declarations still collapse, which is presumably why the guard exists.

2. A picklist column is deleted when its option set will not resolve

The filter at :366 removes Picklist/Multiselectoptionset/State/Status/Bit rows whose OptionSetName is not among the resolved option sets. Three triggers observed:

  • the global option set file exists but declares <options />
  • the option set is declared in a different module from the attribute using it
  • the option set is platform-owned (activitypointer_*, socialprofile_*) with no file in the repo

Required, form-visible business columns disappear with no warning, and a module whose only contribution is such an attribute reads as a non-contributor.

Fix: clear OptionSetName instead of deleting the row. That is the minimum the problem needs — ToDbDiagramNotation prefers OptionSetName over RowType, so a stale name would reference an Enum that was never emitted. RowType is deliberately left alone so each translator keeps its own handling: sql falls back to its existing nvarchar(255) for an unresolvable picklist, edmx keeps Edm.Boolean for a bit, and dbml renders the bare kind. An earlier revision of this PR also forced RowType.Int; that reached past DBML into every target sharing ParseModules and imposed a DBML-driven choice on sql and edmx, so it was reverted.

Measured

Built from source, baseline vs patched, on three real declaration folders:

folder refs columns
customer Model layer 9 → 14 214 → 215
Service/Project/Model 87 → 99 914 → 918
Environment/Bootstrap/Model 87 → 100 1344 → 1349

Table and Enum counts unchanged in all three. No duplicate endpoint pairs introduced. No column left typed as an undeclared Enum. All three targets (dbml, sql, edmx) exercised.

Notes

  • Two changed sites, no refactoring, no new types.
  • The many-to-many path (:439-455) is untouched and correct for the non-self-referential case. A separate defect exists there — a self-referencing N:N emits the intersect column twice and the Ref: line twice — which this PR does not address.
  • Tests: not included. DataModelConvertTests.cs is entirely [Fact(Skip = ...)] integration tests needing a real solution on disk. ParseModules is public static and both defects are unit-testable from in-memory XDocument fixtures; happy to add them wherever you would want them, but I did not want to guess at placement in a draft.
  • Two further issues in the same area are not in this PR: table colours are randomised per run (Module seeds Colorhex from new Random(), so the same unchanged solution converts to a different file every time), and the synthesised <entity>id primary key is wrong for activity entities — the latter is not fixable here, since the only layer that triggers key synthesis is also the layer that omits IsActivity.

🤖 Generated with Claude Code

david-hudec-networg and others added 2 commits August 31, 2026 15:33
…convert

Two defects in DataModelConverter, both silent — the output stays well-formed
while losing model content, so nothing surfaces until you diff against source.

1. Relationship dedup keyed on (LeftSideTable, RightSideTable), ignoring the
   column. Every N:1 relationship after the first between the same table pair
   was discarded. talxis_project has four separate lookups to account (building
   authority, city, city district, architectural studio); only one got an edge,
   while all four columns still rendered — so the diagram understated the model
   without looking broken. Fixed by including LeftSideRow in the key, which is
   also what dbdiagram actually constrains (the endpoint pair, not the table
   pair), so genuine duplicates still collapse.

2. Picklist/state/status/bit rows whose OptionSetName could not be resolved were
   deleted outright. Three triggers seen in the wild: an option set declared with
   <options />, an option set owned by a different module, and platform-owned sets
   (activitypointer_*). Required, form-visible business columns disappeared, and a
   module whose only contribution was such an attribute looked like a
   non-contributor. Now the row is downgraded to Int instead of removed.
   OptionSetName is cleared too, because ToDbDiagramNotation prefers it over
   RowType and the column would otherwise reference an Enum never emitted.

Measured on two unrelated solutions:
  Project Model layer   refs  9 -> 14   (the 5 lost account lookups return)
  Service/Project/Model refs 87 -> 99, columns 911 -> 918
  table and Enum counts unchanged; no duplicate endpoint pairs introduced;
  no column left typed as an undeclared Enum.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Clearing OptionSetName is sufficient and is all the DBML problem requires — it is
what ToDbDiagramNotation prefers over RowType. Also forcing RowType.Int reached
past DBML into every other target sharing ParseModules, overriding each
translator's own handling of the kind:

  target  column                      Int version      this version
  sql     activitytypecode            int              nvarchar(255)
  dbml    activitytypecode            Int              Picklist
  edmx    isregularactivity           Edm.Boolean      Edm.Boolean (unchanged)

nvarchar(255) is SQLTranslator's own existing fallback for a picklist it cannot
resolve, so leaving RowType alone keeps the tool self-consistent instead of
imposing a DBML-driven choice on sql and edmx. One line instead of two.

Verified on three declaration folders, all three targets, no dangling enum
references in the DBML output.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant