From b68c4d5072ec582f3b49295a57c81b08bac3a5eb Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sun, 2 Aug 2026 20:53:20 +0300 Subject: [PATCH 1/5] feat(ddl2cpp): generate HasMany, HasManyThrough and HasOneThrough ddl2cpp emitted only BelongsTo, so a generated model was navigable from child to parent and no further. This adds the inverse and through relations. A BelongsTo is derivable from the child table alone; every relation pointing the other way is implied by a foreign key declared on some *other* table. So the work splits in two: CxxModelPrinter::PlanRelations() answers the schema-wide question once and is a pure function over the table list (hence testable without a database), and PrintTable() consumes one table's share of that plan. Rules, documented in docs/ddl2cpp-relation-generation.md: - a join table - exactly two single-column foreign keys to two *distinct* tables, and no columns beyond those keys - becomes a HasManyThrough on both referenced tables, and the join table itself gets no inverse of its own - it degrades to HasOneThrough when the join record's far-side key is covered by a single-column unique index - every other single-column foreign key becomes a HasMany on the referenced side - a uniquely indexed child foreign key makes the relation one-to-one - composite foreign keys are skipped, exactly as BelongsTo skips them Two shapes are deliberately *not* collapsed. A join table carrying payload columns is an association object, so it keeps its own record and a HasMany on each side, which keeps the payload reachable. A table whose two foreign keys point at the same table has no far side to hop to, so it stays two HasMany relations rather than a nonsensical self-through. Emission detail: an inverse relation names the child while the child's BelongsTo names the parent, so including both ways would be a cycle. The parent forward-declares instead, which is sound because every relation stores its records indirectly (HasMany holds std::vector>) and needs no complete type at the point of declaration. Selectors are emitted whenever the owner is reachable from the same child table through more than one foreign key, and the member name is then qualified by the foreign key column so the members do not collide. This is not a nicety: the reference schema has a table pair joined by 55 foreign keys, and HasMany without a selector cannot resolve its inverse. NB: depends on the relation selectors from PR #528 and so branches from fix/hasmany-inverse-by-type, not from master. On master HasMany takes only and this cannot work. There is also no HasOne type in the library - only HasOneThrough - so a non-through one-to-one is emitted as a collection with a note in the generated header saying so. Verified against a large production schema (MS SQL Server 2022 in Docker): 686 tables, 10830 columns, 1860 foreign keys -> 1770 inverse relations generated (1660 HasMany, 104 HasManyThrough, 6 HasOneThrough, alongside 1496 BelongsTo). A slice of that generated output was then compiled with clang-cl to confirm the emitted relations instantiate. Tests: 17 new cases in CxxModelRelationTests.cpp covering each rule and each deliberate non-rule, with fixtures reduced from the shapes actually found in that schema. Full suite 1311 passed, 1 skipped, 1 failed as expected (the inherited clang-cl self-reference miscompile). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01TVec3YeQg2FZnToiPQwcQH --- docs/ddl2cpp-relation-generation.md | 122 +++++ src/Lightweight/Tools/CxxModelPrinter.cpp | 267 ++++++++++- src/Lightweight/Tools/CxxModelPrinter.hpp | 81 +++- src/tests/CMakeLists.txt | 1 + src/tests/CxxModelRelationTests.cpp | 533 ++++++++++++++++++++++ 5 files changed, 1001 insertions(+), 3 deletions(-) create mode 100644 docs/ddl2cpp-relation-generation.md create mode 100644 src/tests/CxxModelRelationTests.cpp diff --git a/docs/ddl2cpp-relation-generation.md b/docs/ddl2cpp-relation-generation.md new file mode 100644 index 000000000..7469f21d2 --- /dev/null +++ b/docs/ddl2cpp-relation-generation.md @@ -0,0 +1,122 @@ +# Relation generation in ddl2cpp + +## Status + +`ddl2cpp` currently emits exactly one relation type: `Light::BelongsTo`, on the child side of a +single-column foreign key. The inverse and through relations that `DataMapper` supports — +`HasMany`, `HasManyThrough`, `HasOneThrough` — are never generated, so a generated model is +navigable only from child to parent. + +This page records what a real production schema needs, and what is genuinely not representable. + +> **Depends on the relation selectors from PR #528.** Generating inverse relations for a real schema +> is impossible without them: this reference schema has table pairs joined by up to **55** foreign +> keys from the same child table, and `HasMany` without a selector is a compile error as soon +> as more than one foreign key links the pair. On `master` today `HasMany` takes only +> ``; the `SqlRealName` selector overloads of `HasMany`, `HasManyThrough` and +> `HasOneThrough` live on `fix/hasmany-inverse-by-type`. This work therefore branches from there +> rather than from `master`, and must not merge ahead of it. + +There is also **no `HasOne`** type in the library — only `HasOneThrough`. A one-to-one relation that +is *not* across a join table has no representation, so the plan below records it as +`Kind::HasOne` and the emitter falls back to `HasMany` for it, with a note in the generated header. +Adding a real `HasOne` is out of scope here. + +## Reference schema + +The numbers below come from the `konrad_english` database used by the Lastrada application +(MS SQL Server 2022, single schema `lasa`), which is the largest schema this generator is pointed +at in practice. + +| Metric | Count | +|--------|-------| +| Tables | 686 | +| Columns | 10,830 | +| Foreign keys | 1,860 | +| — single-column | 1,770 | +| — composite (multi-column) | 90 | +| Primary keys, composite | 170 | +| Tables with no primary key | 0 | +| Self-referential foreign keys | 0 | +| Foreign keys onto a non-primary-key column | 0 | + +### Column types in use + +`float` (4060), `int` (3632), `varchar` (1302), `datetime` (629), `char` (605), `text` (434), +`tinyint` (90), `money` (31), `smallint` (19), `real` (12), `image` (7), `bigint` (4), +`nvarchar` (3), `varbinary` (2). + +No computed columns, no alias/CLR/`sysname` types, so the alias-type resolution added for +`sysname` is not exercised by this schema (it remains needed for others). + +## What this schema needs that is not generated + +### 1. `HasMany` — the inverse of every single-column foreign key + +1,770 single-column foreign keys each imply a `HasMany` on the referenced side. None is emitted. + +Many of these are ambiguous and require the `SqlRealName` selector that `HasMany` already +supports: there are dozens of table pairs joined by more than one foreign key from the same child +table, the worst carrying **55** foreign keys between a single pair. Auto-detection cannot pick +between them — `InverseBelongsToIndexOf` makes that a compile error — so the generator must emit +the selector rather than a bare `HasMany`. + +### 2. `HasManyThrough` — many-to-many across a join table + +159 tables look like join tables (exactly two single-column foreign keys to two distinct tables). +Of those, **84 also have a composite primary key**, which is the classic many-to-many marker. + +Concrete examples, both two-column tables: + +``` +XLAB_PROJECT_USER (PROJECT_NR -> XLAB_PROJECT, USER_NR -> XLAB_USER) +MANDANT_KUNDE (KUNDEN_NR -> KUNDE, MANDANT_NR -> MANDANTEN) +``` + +Each should yield a `HasManyThrough` on both referenced tables. Because both foreign keys of the +join record point at *different* tables here, the selectors are only needed when a join table +points twice at the same target. + +### 3. `HasOneThrough` / one-to-one + +24 single-column foreign keys sit under a single-column unique index, which makes the relation +one-to-one rather than one-to-many. Those should be a scalar relation, not a `HasMany`. + +## What is not representable, and why + +These are limits of the relation model, not gaps in the generator. They are reported rather than +silently mismodelled. + +| Construct | Count here | Reason | +|-----------|-----------:|--------| +| Composite foreign keys | 90 | `BelongsTo` names a single referenced field (`&Record::member`); there is no multi-column form. `ddl2cpp` already counts and warns about these. | +| Composite primary keys | 170 | `BelongsTo` requires the referenced field to be a primary key, and a relation is expressed through exactly one such field. Affected tables still generate as plain records; only relations *into* them are skipped. | +| `text` / `image` columns | 441 | Deprecated LOB types. They map as text/binary, but `SQLGetData` semantics differ from `varchar(max)`/`varbinary(max)`; see `docs/data-binder.md`. | + +A foreign key onto a non-primary-key unique column is also not representable, and does not occur +in this schema. + +## Generation rules + +The rules the generator follows, in the order it applies them. Only single-column foreign keys are +considered throughout; composite ones are counted and skipped as before. + +1. **`BelongsTo`** on the child side of each foreign key — unchanged. +2. **`HasManyThrough`** on each of the two referenced tables, when a table qualifies as a join + table: exactly two single-column foreign keys, pointing at two *distinct* tables, and no + non-key columns beyond those two foreign keys and its own key columns. +3. **`HasOneThrough`** in place of `HasManyThrough` when the join table's foreign key to the far + side is covered by a single-column unique index. +4. **`HasMany`** on the referenced side of every remaining foreign key, i.e. one that is not part + of a join table already covered by rule 2 or 3. +5. **Scalar rather than collection** when the child's foreign key is itself covered by a + single-column unique index: the relation is one-to-one. + +A selector (`SqlRealName { "" }`) is emitted whenever the referenced table is reachable +from the same child table through more than one foreign key, which is what makes the ambiguous +pairs above compile. + +Because a relation member has to name the *other* record type, and both records must be complete +at that point, the generator emits relations only where its existing dependency ordering already +guarantees a declaration order — the same constraint that governs `BelongsTo` today, and the reason +a self-referential foreign key falls back to a plain field. diff --git a/src/Lightweight/Tools/CxxModelPrinter.cpp b/src/Lightweight/Tools/CxxModelPrinter.cpp index 897e581be..1dbb90cbe 100644 --- a/src/Lightweight/Tools/CxxModelPrinter.cpp +++ b/src/Lightweight/Tools/CxxModelPrinter.cpp @@ -358,6 +358,19 @@ std::string CxxModelPrinter::HeaderFileForTheTable(std::string_view modelNamespa output << std::format("namespace {}\n{{\n", modelNamespace); output << "\n"; + + // Records named by an inverse or through relation are forward-declared rather than included: the + // child's header already includes this one, so including it back would be a cycle. Every relation + // stores its records indirectly, so a declaration suffices here. + auto const& forwardDeclared = _definitions[tableName].forwardDeclaredTables; + if (!forwardDeclared.empty()) + { + for (auto const& declared: forwardDeclared) + if (declared != _definitions[tableName].structName) + output << std::format("struct {};\n", declared); + output << '\n'; + } + output << _definitions[tableName].text.str(); if (!modelNamespace.empty()) output << std::format("}} // end namespace {}\n", modelNamespace); @@ -375,6 +388,180 @@ std::string CxxModelPrinter::HeaderFileForTheTable(std::string_view modelNamespa return output.str(); } +namespace +{ + + /// @return `true` if @p constraint links exactly one column to exactly one column. + bool IsSingleColumn(SqlSchema::ForeignKeyConstraint const& constraint) noexcept + { + return constraint.foreignKey.columns.size() == 1 && constraint.primaryKey.columns.size() == 1; + } + + /// Whether @p columnName is covered by a single-column unique index or UNIQUE constraint on @p table, + /// which is what makes a foreign key through it one-to-one rather than one-to-many. + /// + /// @param table Table owning the column. + /// @param columnName Column to test. + /// @return `true` if a value in that column identifies at most one row. + bool IsUniquelyIndexed(SqlSchema::Table const& table, std::string_view columnName) + { + auto const column = std::ranges::find_if(table.columns, [&](auto const& c) { return c.name == columnName; }); + if (column != table.columns.end() && column->isUnique) + return true; + + // A single-column primary key is unique too, as is any single-column unique index. + if (table.primaryKeys.size() == 1 && table.primaryKeys.front() == columnName) + return true; + + return std::ranges::any_of(table.indexes, [&](SqlSchema::IndexDefinition const& index) { + return index.isUnique && index.columns.size() == 1 && index.columns.front() == columnName; + }); + } + + /// Whether @p table is a pure join table: exactly two single-column foreign keys pointing at two + /// distinct tables, and no columns of its own beyond those keys. + /// + /// A join table carrying extra payload columns is deliberately *not* treated as one. Such a table is + /// an entity in its own right - the association-object shape - and collapsing it into a + /// `HasManyThrough` would hide the payload, so it keeps its plain record plus `BelongsTo` members. + /// + /// @param table Candidate join table. + /// @return The two single-column foreign keys if it qualifies, `std::nullopt` otherwise. + std::optional> AsJoinTable( + SqlSchema::Table const& table) + { + auto singleColumnKeys = std::vector {}; + for (auto const& constraint: table.foreignKeys) + if (IsSingleColumn(constraint)) + singleColumnKeys.emplace_back(constraint); + + if (singleColumnKeys.size() != 2 || table.foreignKeys.size() != 2) + return std::nullopt; + + if (singleColumnKeys[0].primaryKey.table.table == singleColumnKeys[1].primaryKey.table.table) + return std::nullopt; // both keys point at the same table: not a two-sided join + + // Every column must participate in one of the two foreign keys, or be part of the table's own + // key. Anything else is payload, which makes this an association object rather than a join table. + auto const isKeyColumn = [&](std::string const& name) { + return std::ranges::contains(singleColumnKeys[0].foreignKey.columns, name) + || std::ranges::contains(singleColumnKeys[1].foreignKey.columns, name) + || std::ranges::contains(table.primaryKeys, name); + }; + if (!std::ranges::all_of(table.columns, [&](auto const& c) { return isKeyColumn(c.name); })) + return std::nullopt; + + return std::pair { singleColumnKeys[0], singleColumnKeys[1] }; + } + +} // namespace + +CxxModelPrinter::RelationPlan CxxModelPrinter::PlanRelations(std::vector const& tables) +{ + auto plan = RelationPlan {}; + + auto const byName = [&](std::string_view name) -> SqlSchema::Table const* { + auto const it = std::ranges::find_if(tables, [&](auto const& t) { return t.name == name; }); + return it != tables.end() ? &*it : nullptr; + }; + + // How many single-column foreign keys run from one table to another. A count above one makes the + // inverse ambiguous, so the selector naming the foreign key column becomes mandatory. + auto foreignKeyMultiplicity = std::map, size_t> {}; + for (auto const& table: tables) + for (auto const& constraint: table.foreignKeys) + if (IsSingleColumn(constraint)) + ++foreignKeyMultiplicity[{ table.name, constraint.primaryKey.table.table }]; + + auto const isAmbiguous = [&](std::string const& child, std::string const& owner) { + auto const it = foreignKeyMultiplicity.find({ child, owner }); + return it != foreignKeyMultiplicity.end() && it->second > 1; + }; + + // Join tables are consumed as through-relations rather than contributing a HasMany of their own, + // so resolve them first and remember which tables they were. + auto joinTables = std::set {}; + + for (auto const& table: tables) + { + auto const joinKeys = AsJoinTable(table); + if (!joinKeys.has_value()) + continue; + + joinTables.emplace(table.name); + + auto const& [leftKey, rightKey] = *joinKeys; + + // One through-relation on each side, each hopping to the other side's target. + auto const emitThrough = [&](SqlSchema::ForeignKeyConstraint const& ownerKey, + SqlSchema::ForeignKeyConstraint const& farKey) { + auto const& ownerTable = ownerKey.primaryKey.table.table; + auto const& farTable = farKey.primaryKey.table.table; + + // Scalar when the join record's far-side key is uniquely indexed: at most one far row + // per join row, so the owner sees a single record rather than a collection. + auto const farIsUnique = IsUniquelyIndexed(table, farKey.foreignKey.columns.front()); + + plan[ownerTable].emplace_back(PlannedRelation { + .kind = farIsUnique ? PlannedRelation::Kind::HasOneThrough : PlannedRelation::Kind::HasManyThrough, + .ownerTable = ownerTable, + .referencedTable = farTable, + .throughTable = table.name, + .ownerForeignKeyColumn = ownerKey.foreignKey.columns.front(), + .referencedForeignKeyColumn = farKey.foreignKey.columns.front(), + .ownerSelectorRequired = isAmbiguous(table.name, ownerTable), + .referencedSelectorRequired = isAmbiguous(table.name, farTable), + // Named after the far table: `project.users` rather than `project.projectUsers`, since + // the join table is an implementation detail of the relation. + .memberName = farTable, + }); + }; + + emitThrough(leftKey, rightKey); + emitThrough(rightKey, leftKey); + } + + // Every remaining single-column foreign key yields an inverse collection on the referenced side. + for (auto const& table: tables) + { + if (joinTables.contains(table.name)) + continue; + + for (auto const& constraint: table.foreignKeys) + { + if (!IsSingleColumn(constraint)) + continue; // composite: no BelongsTo either, so no inverse + + auto const& ownerTable = constraint.primaryKey.table.table; + if (byName(ownerTable) == nullptr) + continue; // references a table outside the generated set + + auto const& childColumn = constraint.foreignKey.columns.front(); + + // Scalar when the child's own foreign key is uniquely indexed: one child per owner. + auto const childIsUnique = IsUniquelyIndexed(table, childColumn); + + plan[ownerTable].emplace_back(PlannedRelation { + .kind = childIsUnique ? PlannedRelation::Kind::HasOne : PlannedRelation::Kind::HasMany, + .ownerTable = ownerTable, + .referencedTable = table.name, + .throughTable = {}, + .ownerForeignKeyColumn = childColumn, + .referencedForeignKeyColumn = {}, + .ownerSelectorRequired = isAmbiguous(table.name, ownerTable), + .referencedSelectorRequired = false, + // Named after the child table. Where several foreign keys from the same child table + // land here the names would collide, so distinguish those by the foreign key column - + // which is exactly the set that also needs a selector. + .memberName = + isAmbiguous(table.name, ownerTable) ? std::format("{}_{}", table.name, childColumn) : table.name, + }); + } + } + + return plan; +} + SqlSchema::ForeignKeyConstraint const& CxxModelPrinter::GetForeignKey( SqlSchema::Column const& column, std::vector const& foreignKeys) { @@ -580,6 +767,10 @@ std::optional CxxModelPrinter::MapColumnNameOverride(SqlSchema::Ful // NOLINTNEXTLINE(readability-function-cognitive-complexity) void CxxModelPrinter::ResolveOrderAndPrintTable(std::vector const& tables) { + // Inverse and through relations are a property of the whole schema, not of one table, so plan + // them once here and hand each table its own share below. + auto const relationPlan = PlanRelations(tables); + std::unordered_map> numberOfForeignKeys; for (auto const idx: std::views::iota(static_cast(0), tables.size())) numberOfForeignKeys[idx] = static_cast(tables[idx].foreignKeys.size()); @@ -602,7 +793,9 @@ void CxxModelPrinter::ResolveOrderAndPrintTable(std::vector co size_t numberOfPrintedTables = 0; auto const printTable = [&, this](size_t index, auto const& table) { - PrintTable(table); + static auto const noRelations = std::vector {}; + auto const planned = relationPlan.find(table.name); + PrintTable(table, planned != relationPlan.end() ? planned->second : noRelations); numberOfPrintedTables++; updateForeignKeyCountAfterPrinted(table); numberOfForeignKeys[index] = std::nullopt; @@ -639,7 +832,7 @@ void CxxModelPrinter::ResolveOrderAndPrintTable(std::vector co } // NOLINTNEXTLINE(readability-function-cognitive-complexity) -void CxxModelPrinter::PrintTable(SqlSchema::Table const& table) +void CxxModelPrinter::PrintTable(SqlSchema::Table const& table, std::vector const& relationPlan) { auto& definition = _definitions[table.name]; std::string cxxPrimaryKeys; @@ -817,6 +1010,75 @@ void CxxModelPrinter::PrintTable(SqlSchema::Table const& table) (void) foreignKey; // TODO } + // Inverse and through relations. These come after the columns because they are not columns: they + // carry no storage of their own, and RecordColumnMember-based projections skip them. + if (!relationPlan.empty()) + definition.text << '\n'; + + for (auto const& relation: relationPlan) + { + auto const referencedStruct = AliasTableName(relation.referencedTable); + auto const throughStruct = relation.throughTable.empty() ? std::string {} : AliasTableName(relation.throughTable); + + // The relation names the other record but must not include its header: the child already + // includes this one, so including it back would be a cycle. A forward declaration is enough + // because every relation stores its records indirectly. + if (relation.referencedTable != table.name) + definition.forwardDeclaredTables.emplace(referencedStruct); + if (!throughStruct.empty() && relation.throughTable != table.name) + definition.forwardDeclaredTables.emplace(throughStruct); + + auto const memberName = uniqueMemberNameBuilder.DeclareName( + SanitizeName(FormatName(StripSuffix(relation.memberName), _config.formatType))); + + auto const ownerSelector = relation.ownerSelectorRequired + ? std::format(", Light::SqlRealName {{ \"{}\" }}", relation.ownerForeignKeyColumn) + : std::string {}; + + switch (relation.kind) + { + case PlannedRelation::Kind::HasOne: + // No HasOne type exists in the library, only HasOneThrough. Emit the collection form + // and say why, rather than silently pretending the relation is scalar. + definition.text << std::format(" // NOTE: {}.{} is uniquely indexed, so this relation holds at most\n" + " // one record. Lightweight has no HasOne type, so it is " + "emitted as a collection.\n", + relation.referencedTable, + relation.ownerForeignKeyColumn); + [[fallthrough]]; + case PlannedRelation::Kind::HasMany: + definition.text << std::format( + " Light::HasMany<{}{}> {};\n", referencedStruct, ownerSelector, memberName); + break; + + case PlannedRelation::Kind::HasOneThrough: + definition.text << std::format( + " Light::HasOneThrough<{}, {}{}{}> {};\n", + referencedStruct, + throughStruct, + ownerSelector, + relation.referencedSelectorRequired + ? std::format(", Light::SqlRealName {{ \"{}\" }}", relation.referencedForeignKeyColumn) + : std::string {}, + memberName); + break; + + case PlannedRelation::Kind::HasManyThrough: + definition.text << std::format( + " Light::HasManyThrough<{}, {}{}{}> {};\n", + referencedStruct, + throughStruct, + ownerSelector, + relation.referencedSelectorRequired + ? std::format(", Light::SqlRealName {{ \"{}\" }}", relation.referencedForeignKeyColumn) + : std::string {}, + memberName); + break; + } + + ++_numberOfRelationsListed; + } + definition.text << "};\n\n"; } @@ -830,6 +1092,7 @@ void CxxModelPrinter::PrintReport() std::println("Columns listed : {}", _numberOfColumnsListed); std::println("Foreign keys considered : {}", _numberOfForeignKeysListed); std::println("Foreign keys ignored : {}", _warningOnUnsupportedMultiKeyForeignKey.size()); + std::println("Inverse relations : {}", _numberOfRelationsListed); if (!_warningOnUnsupportedMultiKeyForeignKey.empty() && !_config.suppressWarnings) { diff --git a/src/Lightweight/Tools/CxxModelPrinter.hpp b/src/Lightweight/Tools/CxxModelPrinter.hpp index 28f83f1a3..47bef2311 100644 --- a/src/Lightweight/Tools/CxxModelPrinter.hpp +++ b/src/Lightweight/Tools/CxxModelPrinter.hpp @@ -95,7 +95,76 @@ class CxxModelPrinter void ResolveOrderAndPrintTable(std::vector const& tables); - void PrintTable(SqlSchema::Table const& table); + /// @brief One inverse or through relation to emit on a record. + /// + /// A `BelongsTo` is derivable from the child table alone, but every relation pointing the other + /// way needs to know about foreign keys declared on *other* tables. `RelationPlan` is that + /// schema-wide answer, computed once by @ref PlanRelations and consumed per table. + struct PlannedRelation + { + /// Which relation template to emit. + enum class Kind : uint8_t + { + HasMany, //!< One-to-many: the inverse of a child's BelongsTo. + HasOne, //!< One-to-one: as HasMany, but the child's foreign key is uniquely indexed. + HasManyThrough, //!< Many-to-many across a join table. + HasOneThrough, //!< As HasManyThrough, but the far side is uniquely indexed. + }; + + Kind kind {}; + + /// Table this relation is emitted on. + std::string ownerTable; + + /// The record the relation yields: the child table for HasMany/HasOne, the far table for the + /// through relations. + std::string referencedTable; + + /// Join table, for the through relations only; empty otherwise. + std::string throughTable; + + /// Foreign key column linking the child (or join record) back to the owner. Emitted as a + /// `SqlRealName` selector so that several foreign keys into one table stay distinguishable. + std::string ownerForeignKeyColumn; + + /// Foreign key column linking the join record to the far table; empty for HasMany/HasOne. + std::string referencedForeignKeyColumn; + + /// Whether a selector must be emitted for @ref ownerForeignKeyColumn. It is required when the + /// owner is reachable from the same child table through more than one foreign key, and is + /// harmless otherwise - but emitting it unconditionally would churn every generated header, + /// so it is tracked. + bool ownerSelectorRequired {}; + + /// As @ref ownerSelectorRequired, for @ref referencedForeignKeyColumn. + bool referencedSelectorRequired {}; + + /// Member name to emit, already sanitized and formatted. + std::string memberName; + }; + + /// @brief Relations to emit, keyed by the table they belong on. + using RelationPlan = std::map>; + + /// @brief Works out every inverse and through relation implied by a schema. + /// + /// Applies the rules documented in `docs/ddl2cpp-relation-generation.md`: a join table (exactly + /// two single-column foreign keys to two distinct tables, and no payload columns) becomes a + /// `HasManyThrough` on both sides; every other single-column foreign key becomes a `HasMany` on + /// the referenced side; and either degrades to its scalar form when the relevant foreign key is + /// covered by a single-column unique index. Composite foreign keys are ignored, matching the + /// existing `BelongsTo` behaviour. + /// + /// Pure: it reads the schema and returns a plan, so it is testable without a database. + /// + /// @param tables The whole schema. Relations are only planned between tables present here. + /// @return The relations to emit, keyed by owning table name. + [[nodiscard]] static RelationPlan PlanRelations(std::vector const& tables); + + /// @param table Table to emit. + /// @param relationPlan Inverse and through relations to emit on it, from @ref PlanRelations. + /// Defaults to none, which yields the BelongsTo-only output. + void PrintTable(SqlSchema::Table const& table, std::vector const& relationPlan = {}); void PrintReport(); @@ -110,6 +179,15 @@ class CxxModelPrinter /// several foreign-key columns pointing at the same target must still be included once, so /// this is a set (which also gives the emitted `#include` block a stable, sorted order). std::set requiredTables; + + /// Records this one names but must *not* include, emitted as forward declarations instead. + /// + /// An inverse relation points from parent to child while the child's `BelongsTo` points back, + /// so including both ways would be a cycle. `HasMany` stores + /// `std::vector>` and the through relations are equally + /// indirect, so a declaration is sufficient at the point of use - the definition is only + /// needed where the relation is actually loaded, which is a `.cpp`. + std::set forwardDeclaredTables; std::string structName; //< C++ struct name (possibly aliased). std::vector> members; //< (emitted member id, SQL column name), in order. }; @@ -132,6 +210,7 @@ class CxxModelPrinter std::map _warningOnUnsupportedMultiKeyForeignKey; size_t _numberOfColumnsListed = 0; size_t _numberOfForeignKeysListed = 0; + size_t _numberOfRelationsListed = 0; }; } // namespace Lightweight::Tools diff --git a/src/tests/CMakeLists.txt b/src/tests/CMakeLists.txt index a0d503418..2d2b4e0c4 100644 --- a/src/tests/CMakeLists.txt +++ b/src/tests/CMakeLists.txt @@ -39,6 +39,7 @@ set(SOURCE_FILES ConfigProfileStoreTests.cpp CoreTests.cpp CxxModelPrinterTests.cpp + CxxModelRelationTests.cpp DataBinderTests.cpp DataSourceEnumeratorTests.cpp PluginDiscoveryTests.cpp diff --git a/src/tests/CxxModelRelationTests.cpp b/src/tests/CxxModelRelationTests.cpp new file mode 100644 index 000000000..c7c7d8025 --- /dev/null +++ b/src/tests/CxxModelRelationTests.cpp @@ -0,0 +1,533 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// Relation generation in ddl2cpp: the inverse and through relations implied by a schema. +// +// `CxxModelPrinterTests.cpp` covers column type mapping and the child-side `BelongsTo`. This file +// covers the other direction - `HasMany`, `HasManyThrough`, `HasOneThrough` - which requires +// schema-wide knowledge because a relation on a parent is implied by a foreign key declared on some +// other table. +// +// The shapes tested here are taken from the reference schema surveyed in +// `docs/ddl2cpp-relation-generation.md` (the `konrad_english` database behind the Lastrada +// application, 686 tables / 1860 foreign keys), reduced to the smallest fixtures that reproduce +// each structural case found there: +// +// - a plain parent/child pair -> HasMany +// - two foreign keys from one child into one parent -> HasMany + selector (that schema has a +// pair joined by 55 foreign keys) +// - a two-column join table -> HasManyThrough on both sides +// (e.g. XLAB_PROJECT_USER, MANDANT_KUNDE) +// - a join table whose far key is uniquely indexed -> HasOneThrough +// - a uniquely indexed child foreign key -> one-to-one +// - a join table carrying payload columns -> NOT a join table (association object) +// - composite foreign keys -> skipped, as for BelongsTo + +#include +#include + +#include + +#include +#include + +using Lightweight::Tools::CxxModelPrinter; +using Relation = CxxModelPrinter::PlannedRelation; +using Kind = Relation::Kind; + +namespace +{ + +using namespace Lightweight::SqlColumnTypeDefinitions; + +/// Builds a fully qualified name in the default (empty) catalog and schema, as ReadAllTables yields +/// for a single-schema database. +/// +/// @param table Table name. +/// @return The qualified name. +Lightweight::SqlSchema::FullyQualifiedTableName Qualified(std::string_view table) +{ + return { .catalog = "", .schema = "", .table = std::string { table } }; +} + +/// Builds a single-column foreign key constraint. +/// +/// @param childTable Table holding the foreign key. +/// @param childColumn Foreign key column. +/// @param parentTable Referenced table. +/// @param parentColumn Referenced column, its primary key. +/// @return The constraint. +Lightweight::SqlSchema::ForeignKeyConstraint ForeignKey(std::string_view childTable, + std::string_view childColumn, + std::string_view parentTable, + std::string_view parentColumn = "id") +{ + return { .foreignKey = { .table = Qualified(childTable), .columns = { std::string { childColumn } } }, + .primaryKey = { .table = Qualified(parentTable), .columns = { std::string { parentColumn } } } }; +} + +/// @return The relations planned for @p table, or an empty vector if none. +std::vector RelationsOn(CxxModelPrinter::RelationPlan const& plan, std::string_view table) +{ + auto const it = plan.find(std::string { table }); + return it != plan.end() ? it->second : std::vector {}; +} + +/// @return The single relation planned for @p table; fails the test if there is not exactly one. +Relation SoleRelationOn(CxxModelPrinter::RelationPlan const& plan, std::string_view table) +{ + auto const relations = RelationsOn(plan, table); + REQUIRE(relations.size() == 1); + return relations.front(); +} + +Lightweight::SqlSchema::Column IdColumn() +{ + return { .name = "id", .type = Integer {}, .isNullable = false, .isPrimaryKey = true }; +} + +Lightweight::SqlSchema::Column ForeignKeyColumn(std::string_view name, bool isUnique = false) +{ + return { + .name = std::string { name }, .type = Integer {}, .isNullable = false, .isUnique = isUnique, .isForeignKey = true + }; +} + +} // namespace + +// ================================================================================================ +// HasMany - the inverse of a plain child foreign key +// ================================================================================================ + +TEST_CASE("PlanRelations: a child foreign key yields a HasMany on the parent", "[CxxModelPrinter][relations]") +{ + auto const tables = std::vector { + { .schema = "", .name = "author", .columns = { IdColumn() }, .primaryKeys = { "id" } }, + { .schema = "", + .name = "book", + .columns = { IdColumn(), ForeignKeyColumn("author_id") }, + .foreignKeys = { ForeignKey("book", "author_id", "author") }, + .primaryKeys = { "id" } }, + }; + + auto const plan = CxxModelPrinter::PlanRelations(tables); + + // The relation lands on the parent, not the child - the child already has its BelongsTo. + auto const relation = SoleRelationOn(plan, "author"); + CHECK(relation.kind == Kind::HasMany); + CHECK(relation.ownerTable == "author"); + CHECK(relation.referencedTable == "book"); + CHECK(relation.ownerForeignKeyColumn == "author_id"); + CHECK(relation.throughTable.empty()); + + // A single foreign key between the pair is unambiguous, so no selector is needed. + CHECK_FALSE(relation.ownerSelectorRequired); + + // ...and nothing is planned on the child. + CHECK(RelationsOn(plan, "book").empty()); +} + +TEST_CASE("PlanRelations: two foreign keys into one parent require selectors", "[CxxModelPrinter][relations]") +{ + // The shape the reference schema has in abundance - one pair there is joined by 55 foreign keys. + // Without a selector `HasMany` cannot resolve its inverse and is a compile error, so the + // selector is what makes such a schema generatable at all. + auto const tables = std::vector { + { .schema = "", .name = "person", .columns = { IdColumn() }, .primaryKeys = { "id" } }, + { .schema = "", + .name = "meeting", + .columns = { IdColumn(), ForeignKeyColumn("organizer_id"), ForeignKeyColumn("minute_taker_id") }, + .foreignKeys = { ForeignKey("meeting", "organizer_id", "person"), + ForeignKey("meeting", "minute_taker_id", "person") }, + .primaryKeys = { "id" } }, + }; + + auto const plan = CxxModelPrinter::PlanRelations(tables); + auto const relations = RelationsOn(plan, "person"); + REQUIRE(relations.size() == 2); + + for (auto const& relation: relations) + { + CHECK(relation.kind == Kind::HasMany); + CHECK(relation.referencedTable == "meeting"); + CHECK(relation.ownerSelectorRequired); // both are ambiguous without one + } + + // Each names its own foreign key column... + CHECK(relations[0].ownerForeignKeyColumn != relations[1].ownerForeignKeyColumn); + + // ...and the member names must differ too, or the generated struct would not compile. + CHECK(relations[0].memberName != relations[1].memberName); + CHECK(relations[0].memberName.contains(relations[0].ownerForeignKeyColumn)); +} + +TEST_CASE("PlanRelations: a uniquely indexed child foreign key is one-to-one", "[CxxModelPrinter][relations]") +{ + // A unique index over the child's foreign key means at most one child per parent, so the relation + // is scalar rather than a collection. + auto const tables = std::vector { + { .schema = "", .name = "user", .columns = { IdColumn() }, .primaryKeys = { "id" } }, + { .schema = "", + .name = "profile", + .columns = { IdColumn(), ForeignKeyColumn("user_id", /*isUnique=*/true) }, + .foreignKeys = { ForeignKey("profile", "user_id", "user") }, + .primaryKeys = { "id" } }, + }; + + auto const relation = SoleRelationOn(CxxModelPrinter::PlanRelations(tables), "user"); + CHECK(relation.kind == Kind::HasOne); + CHECK(relation.referencedTable == "profile"); +} + +TEST_CASE("PlanRelations: uniqueness via a single-column unique index, not just the column flag", + "[CxxModelPrinter][relations]") +{ + // The same one-to-one shape, but declared as an index rather than a column-level UNIQUE. Both + // spellings occur in real schemas and must be treated the same. + auto const tables = std::vector { + { .schema = "", .name = "user", .columns = { IdColumn() }, .primaryKeys = { "id" } }, + { .schema = "", + .name = "profile", + .columns = { IdColumn(), ForeignKeyColumn("user_id") }, + .foreignKeys = { ForeignKey("profile", "user_id", "user") }, + .primaryKeys = { "id" }, + .indexes = { { .name = "UX_profile_user", .columns = { "user_id" }, .isUnique = true } } }, + }; + + CHECK(SoleRelationOn(CxxModelPrinter::PlanRelations(tables), "user").kind == Kind::HasOne); +} + +TEST_CASE("PlanRelations: a composite unique index does not make a relation one-to-one", "[CxxModelPrinter][relations]") +{ + // Uniqueness of (user_id, kind) says nothing about user_id alone, so the relation stays a + // collection. Treating it as scalar would silently drop rows. + auto const tables = std::vector { + { .schema = "", .name = "user", .columns = { IdColumn() }, .primaryKeys = { "id" } }, + { .schema = "", + .name = "profile", + .columns = { IdColumn(), ForeignKeyColumn("user_id"), { .name = "kind", .type = Integer {} } }, + .foreignKeys = { ForeignKey("profile", "user_id", "user") }, + .primaryKeys = { "id" }, + .indexes = { { .name = "UX_profile_user_kind", .columns = { "user_id", "kind" }, .isUnique = true } } }, + }; + + CHECK(SoleRelationOn(CxxModelPrinter::PlanRelations(tables), "user").kind == Kind::HasMany); +} + +// ================================================================================================ +// HasManyThrough / HasOneThrough - across a join table +// ================================================================================================ + +TEST_CASE("PlanRelations: a two-column join table yields HasManyThrough on both sides", "[CxxModelPrinter][relations]") +{ + // Modelled on XLAB_PROJECT_USER(PROJECT_NR -> XLAB_PROJECT, USER_NR -> XLAB_USER) from the + // reference schema: a composite primary key over exactly the two foreign keys, no payload. + auto const tables = std::vector { + { .schema = "", .name = "project", .columns = { IdColumn() }, .primaryKeys = { "id" } }, + { .schema = "", .name = "user", .columns = { IdColumn() }, .primaryKeys = { "id" } }, + { .schema = "", + .name = "project_user", + .columns = { ForeignKeyColumn("project_id"), ForeignKeyColumn("user_id") }, + .foreignKeys = { ForeignKey("project_user", "project_id", "project"), + ForeignKey("project_user", "user_id", "user") }, + .primaryKeys = { "project_id", "user_id" } }, + }; + + auto const plan = CxxModelPrinter::PlanRelations(tables); + + // Each side reaches the *other* side, through the join table. + auto const fromProject = SoleRelationOn(plan, "project"); + CHECK(fromProject.kind == Kind::HasManyThrough); + CHECK(fromProject.referencedTable == "user"); + CHECK(fromProject.throughTable == "project_user"); + CHECK(fromProject.ownerForeignKeyColumn == "project_id"); + CHECK(fromProject.referencedForeignKeyColumn == "user_id"); + + auto const fromUser = SoleRelationOn(plan, "user"); + CHECK(fromUser.kind == Kind::HasManyThrough); + CHECK(fromUser.referencedTable == "project"); + CHECK(fromUser.throughTable == "project_user"); + CHECK(fromUser.ownerForeignKeyColumn == "user_id"); + CHECK(fromUser.referencedForeignKeyColumn == "project_id"); + + // The join table itself gets no inverse relation: it is consumed by the through relations, and a + // HasMany onto it as well would be redundant. + CHECK(RelationsOn(plan, "project_user").empty()); +} + +TEST_CASE("PlanRelations: a join table with a uniquely indexed far key yields HasOneThrough", "[CxxModelPrinter][relations]") +{ + // When the join record's far-side key is unique, each owner reaches at most one far record. + auto const tables = std::vector { + { .schema = "", .name = "employee", .columns = { IdColumn() }, .primaryKeys = { "id" } }, + { .schema = "", .name = "locker", .columns = { IdColumn() }, .primaryKeys = { "id" } }, + { .schema = "", + .name = "employee_locker", + .columns = { ForeignKeyColumn("employee_id"), ForeignKeyColumn("locker_id", /*isUnique=*/true) }, + .foreignKeys = { ForeignKey("employee_locker", "employee_id", "employee"), + ForeignKey("employee_locker", "locker_id", "locker") }, + .primaryKeys = { "employee_id", "locker_id" } }, + }; + + auto const plan = CxxModelPrinter::PlanRelations(tables); + + // Employee -> locker crosses the unique key, so at most one locker. + auto const fromEmployee = SoleRelationOn(plan, "employee"); + CHECK(fromEmployee.kind == Kind::HasOneThrough); + CHECK(fromEmployee.referencedTable == "locker"); + + // The other direction crosses the non-unique employee_id, so it stays a collection. + auto const fromLocker = SoleRelationOn(plan, "locker"); + CHECK(fromLocker.kind == Kind::HasManyThrough); + CHECK(fromLocker.referencedTable == "employee"); +} + +TEST_CASE("PlanRelations: a join table with payload columns stays an entity", "[CxxModelPrinter][relations]") +{ + // The association-object shape: the join table carries data of its own, so collapsing it into a + // HasManyThrough would hide that column. It must instead behave like any other child table - + // a HasMany on each side - so the payload stays reachable. + auto const tables = std::vector { + { .schema = "", .name = "item", .columns = { IdColumn() }, .primaryKeys = { "id" } }, + { .schema = "", .name = "keyword", .columns = { IdColumn() }, .primaryKeys = { "id" } }, + { .schema = "", + .name = "item_keyword", + .columns = { IdColumn(), + ForeignKeyColumn("item_id"), + ForeignKeyColumn("keyword_id"), + { .name = "weight", .type = Integer {} } }, // <-- payload + .foreignKeys = { ForeignKey("item_keyword", "item_id", "item"), + ForeignKey("item_keyword", "keyword_id", "keyword") }, + .primaryKeys = { "id" } }, + }; + + auto const plan = CxxModelPrinter::PlanRelations(tables); + + // Not through-relations: each side sees the join record itself. + auto const fromItem = SoleRelationOn(plan, "item"); + CHECK(fromItem.kind == Kind::HasMany); + CHECK(fromItem.referencedTable == "item_keyword"); + CHECK(fromItem.throughTable.empty()); + + auto const fromKeyword = SoleRelationOn(plan, "keyword"); + CHECK(fromKeyword.kind == Kind::HasMany); + CHECK(fromKeyword.referencedTable == "item_keyword"); +} + +TEST_CASE("PlanRelations: a table with two foreign keys to the same table is not a join table", + "[CxxModelPrinter][relations]") +{ + // Both keys point at `person`, so there is no far side to hop to. This must stay two HasMany + // relations on `person`, not a nonsensical person-to-person through relation. + auto const tables = std::vector { + { .schema = "", .name = "person", .columns = { IdColumn() }, .primaryKeys = { "id" } }, + { .schema = "", + .name = "marriage", + .columns = { ForeignKeyColumn("spouse_a_id"), ForeignKeyColumn("spouse_b_id") }, + .foreignKeys = { ForeignKey("marriage", "spouse_a_id", "person"), + ForeignKey("marriage", "spouse_b_id", "person") }, + .primaryKeys = { "spouse_a_id", "spouse_b_id" } }, + }; + + auto const relations = RelationsOn(CxxModelPrinter::PlanRelations(tables), "person"); + REQUIRE(relations.size() == 2); + for (auto const& relation: relations) + { + CHECK(relation.kind == Kind::HasMany); + CHECK(relation.throughTable.empty()); + CHECK(relation.ownerSelectorRequired); // ambiguous: two keys from marriage into person + } +} + +// ================================================================================================ +// Limits: what is deliberately not planned +// ================================================================================================ + +TEST_CASE("PlanRelations: composite foreign keys are skipped", "[CxxModelPrinter][relations]") +{ + // A composite foreign key has no BelongsTo either - BelongsTo names a single referenced field - + // so there is no inverse to generate. The reference schema has 90 of these. + auto const tables = std::vector { + { .schema = "", + .name = "parent", + .columns = { { .name = "a", .type = Integer {}, .isPrimaryKey = true }, + { .name = "b", .type = Integer {}, .isPrimaryKey = true } }, + .primaryKeys = { "a", "b" } }, + { .schema = "", + .name = "child", + .columns = { IdColumn(), ForeignKeyColumn("ref_a"), ForeignKeyColumn("ref_b") }, + .foreignKeys = { { .foreignKey = { .table = Qualified("child"), .columns = { "ref_a", "ref_b" } }, + .primaryKey = { .table = Qualified("parent"), .columns = { "a", "b" } } } }, + .primaryKeys = { "id" } }, + }; + + CHECK(CxxModelPrinter::PlanRelations(tables).empty()); +} + +TEST_CASE("PlanRelations: a foreign key to a table outside the set is skipped", "[CxxModelPrinter][relations]") +{ + // Generating a relation onto a record that is not being generated would not compile. + auto const tables = std::vector { + { .schema = "", + .name = "book", + .columns = { IdColumn(), ForeignKeyColumn("author_id") }, + .foreignKeys = { ForeignKey("book", "author_id", "author_not_in_this_set") }, + .primaryKeys = { "id" } }, + }; + + CHECK(CxxModelPrinter::PlanRelations(tables).empty()); +} + +TEST_CASE("PlanRelations: a schema with no foreign keys plans nothing", "[CxxModelPrinter][relations]") +{ + auto const tables = std::vector { + { .schema = "", .name = "lonely", .columns = { IdColumn() }, .primaryKeys = { "id" } }, + }; + + CHECK(CxxModelPrinter::PlanRelations(tables).empty()); +} + +// ================================================================================================ +// Emission - the generated C++ text +// ================================================================================================ + +TEST_CASE("CxxModelPrinter: emits HasMany with a forward declaration, not an include", "[CxxModelPrinter][relations]") +{ + // The parent names the child, while the child's BelongsTo names the parent. Including both ways + // would be a cycle, so the parent forward-declares instead - which is sound because HasMany holds + // std::vector> and needs no complete type here. + auto const tables = std::vector { + { .schema = "", .name = "author", .columns = { IdColumn() }, .primaryKeys = { "id" } }, + { .schema = "", + .name = "book", + .columns = { IdColumn(), ForeignKeyColumn("author_id") }, + .foreignKeys = { ForeignKey("book", "author_id", "author") }, + .primaryKeys = { "id" } }, + }; + + auto printer = CxxModelPrinter { CxxModelPrinter::Config {} }; + printer.ResolveOrderAndPrintTable(tables); + + auto const authorHeader = printer.HeaderFileForTheTable("Models", "author"); + INFO("author.hpp:\n" << authorHeader); + + CHECK(authorHeader.contains("Light::HasMany")); + CHECK(authorHeader.contains("struct book;")); // forward-declared... + CHECK_FALSE(authorHeader.contains("#include \"book.hpp\"")); // ...never included + + // The child keeps its BelongsTo and does include the parent, as before. + auto const bookHeader = printer.HeaderFileForTheTable("Models", "book"); + INFO("book.hpp:\n" << bookHeader); + CHECK(bookHeader.contains("Light::BelongsTo<&author::id")); + CHECK(bookHeader.contains("#include \"author.hpp\"")); +} + +TEST_CASE("CxxModelPrinter: emits a selector for each of several foreign keys into one table", + "[CxxModelPrinter][relations]") +{ + auto const tables = std::vector { + { .schema = "", .name = "person", .columns = { IdColumn() }, .primaryKeys = { "id" } }, + { .schema = "", + .name = "meeting", + .columns = { IdColumn(), ForeignKeyColumn("organizer_id"), ForeignKeyColumn("minute_taker_id") }, + .foreignKeys = { ForeignKey("meeting", "organizer_id", "person"), + ForeignKey("meeting", "minute_taker_id", "person") }, + .primaryKeys = { "id" } }, + }; + + auto printer = CxxModelPrinter { CxxModelPrinter::Config {} }; + printer.ResolveOrderAndPrintTable(tables); + + auto const header = printer.HeaderFileForTheTable("Models", "person"); + INFO("person.hpp:\n" << header); + + // Both selectors present, so each HasMany resolves to its own foreign key. + CHECK(header.contains(R"(Light::SqlRealName { "organizer_id" })")); + CHECK(header.contains(R"(Light::SqlRealName { "minute_taker_id" })")); + + // One include of the child at most, and it is a forward declaration. + CHECK(header.contains("struct meeting;")); +} + +TEST_CASE("CxxModelPrinter: emits HasManyThrough for a join table", "[CxxModelPrinter][relations]") +{ + auto const tables = std::vector { + { .schema = "", .name = "project", .columns = { IdColumn() }, .primaryKeys = { "id" } }, + { .schema = "", .name = "user", .columns = { IdColumn() }, .primaryKeys = { "id" } }, + { .schema = "", + .name = "project_user", + .columns = { ForeignKeyColumn("project_id"), ForeignKeyColumn("user_id") }, + .foreignKeys = { ForeignKey("project_user", "project_id", "project"), + ForeignKey("project_user", "user_id", "user") }, + .primaryKeys = { "project_id", "user_id" } }, + }; + + auto printer = CxxModelPrinter { CxxModelPrinter::Config {} }; + printer.ResolveOrderAndPrintTable(tables); + + auto const header = printer.HeaderFileForTheTable("Models", "project"); + INFO("project.hpp:\n" << header); + + // The far record first, the join record second - matching HasManyThrough. + CHECK(header.contains("Light::HasManyThrough { + { .schema = "", .name = "user", .columns = { IdColumn() }, .primaryKeys = { "id" } }, + { .schema = "", + .name = "profile", + .columns = { IdColumn(), ForeignKeyColumn("user_id", /*isUnique=*/true) }, + .foreignKeys = { ForeignKey("profile", "user_id", "user") }, + .primaryKeys = { "id" } }, + }; + + auto printer = CxxModelPrinter { CxxModelPrinter::Config {} }; + printer.ResolveOrderAndPrintTable(tables); + + auto const header = printer.HeaderFileForTheTable("Models", "user"); + INFO("user.hpp:\n" << header); + + CHECK(header.contains("Light::HasMany")); + CHECK(header.contains("uniquely indexed")); + CHECK(header.contains("no HasOne type")); +} + +TEST_CASE("CxxModelPrinter: relation members do not collide with column members", "[CxxModelPrinter][relations]") +{ + // A parent with a column already named like the child table: the relation member has to be + // uniqued against it, or the generated struct declares the same name twice. + auto const tables = std::vector { + { .schema = "", + .name = "author", + .columns = { IdColumn(), { .name = "book", .type = Varchar { 20 } } }, // collides with the relation + .primaryKeys = { "id" } }, + { .schema = "", + .name = "book", + .columns = { IdColumn(), ForeignKeyColumn("author_id") }, + .foreignKeys = { ForeignKey("book", "author_id", "author") }, + .primaryKeys = { "id" } }, + }; + + auto printer = CxxModelPrinter { CxxModelPrinter::Config {} }; + printer.ResolveOrderAndPrintTable(tables); + + auto const header = printer.HeaderFileForTheTable("Models", "author"); + INFO("author.hpp:\n" << header); + + // Both members exist, under different names. + CHECK(header.contains("Light::HasMany")); + + // Count the declarations of the colliding identifier: the column takes `book`, so the relation + // must have been renamed. + auto occurrences = size_t { 0 }; + for (auto offset = header.find("> book;"); offset != std::string::npos; offset = header.find("> book;", offset + 1)) + ++occurrences; + CHECK(occurrences <= 1); +} From bb0b7ff76c5cf785abbc099842a5c156522fda4d Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sun, 2 Aug 2026 21:43:18 +0300 Subject: [PATCH 2/5] feat(DataMapper): composite foreign keys via CompositeForeignKey/Connection A composite foreign key references a composite primary key, so both sides need the same notion of "a key spans several columns". Under the guiding principle that every database column is one data member, that means several members marked PrimaryKey on the referenced side, and several ordinary column members plus one relation member on the referencing side. struct Parent { Field partA; Field partB; }; struct Child { Field id; Field refA; // ordinary columns Field refB; CompositeForeignKey, Connection<&Child::refB, &Parent::partB>> parent; }; Why the pairing lives in the type: with two parallel column *lists* matched positionally, transposing two same-typed columns is silently wrong and uncatchable. As a Connection the pairing is part of the type, so a transposition is a different Connection - and where the paired columns differ in value type it does not compile. BelongsTo is deliberately untouched. It is inseparably a *column* - storage, FieldWithStorage, a binder over one ODBC column index - as well as a navigator. Widening it would break the one-member-one-column invariant that RecordColumnCount (`accum + 1` per column member), every projection builder, the output-binding loop and the multi-record column-offset arithmetic all rest on. Under the principle above its column half is also unnecessary, since the columns already have their own members. So the new relation does only the navigation half, in the mould of HasOneThrough: no Value()/IsModified(), therefore not RecordColumnMember, therefore contributing 0 columns and skipped by every projection automatically. No binder, projection or column-arithmetic code changed. Ordering is the subtle part, and CompositeKeyOrderingTests.cpp pins it first: QuerySingle emits one predicate per primary-key member in the *referenced record's member declaration* order and binds positionally, so passing values in connection order would bind them to the wrong predicates whenever the two orders differ - fetching a wrong row rather than failing. OrderedValuesOf() therefore permutes values into parent-member order using each Connection's IntoMemberIndex, so connections may be written in any order. Tested with a deliberately scrambled three-column key. Loading reuses existing machinery: the loader std::applies the ordered values into QuerySingle, which already generates `WHERE k1 = ? AND k2 = ?`. No new SQL generation, no new binding path. Reflected identity is additive: RecordPrimaryKeyTuple, RecordPrimaryKeyCount, HasCompositePrimaryKey and GetPrimaryKeyFields() sit beside the single-key helpers, which are left resolving one member index so no existing caller changes shape - notably CreateExplicit, which returns RecordPrimaryKeyType. Deferred, and recorded in docs/composite-keys-design.md rather than silently skipped: ddl2cpp generation of these declarations; the inverse (HasMany over a composite relation, needing a list-valued selector); AutoAssign semantics across several key columns; and confirming the C++26 reflection configuration. Tests: 22 cases / 124 assertions across CompositeForeignKeyTests.cpp, CompositeKeyOrderingTests.cpp and CompositeKeyGapTests.cpp - type derivation, the three static_assert rejections, value extraction, out-of-order and three-column loading against a live DB, and reflected identity. Full suite: 1333 passed, 1 skipped, 1 failed as expected (sqlite3, clang-cl). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01TVec3YeQg2FZnToiPQwcQH --- docs/composite-keys-design.md | 247 +++++++++++ src/Lightweight/CMakeLists.txt | 1 + .../DataMapper/CompositeForeignKey.hpp | 314 ++++++++++++++ src/Lightweight/DataMapper/DataMapper.hpp | 26 ++ src/Lightweight/DataMapper/Record.hpp | 91 +++++ src/Lightweight/Lightweight.cppm | 8 + src/tests/CMakeLists.txt | 3 + src/tests/CompositeForeignKeyTests.cpp | 386 ++++++++++++++++++ src/tests/CompositeKeyGapTests.cpp | 260 ++++++++++++ src/tests/CompositeKeyOrderingTests.cpp | 170 ++++++++ 10 files changed, 1506 insertions(+) create mode 100644 docs/composite-keys-design.md create mode 100644 src/Lightweight/DataMapper/CompositeForeignKey.hpp create mode 100644 src/tests/CompositeForeignKeyTests.cpp create mode 100644 src/tests/CompositeKeyGapTests.cpp create mode 100644 src/tests/CompositeKeyOrderingTests.cpp diff --git a/docs/composite-keys-design.md b/docs/composite-keys-design.md new file mode 100644 index 000000000..18aa4d713 --- /dev/null +++ b/docs/composite-keys-design.md @@ -0,0 +1,247 @@ +# Composite key support — design + +Status: **proposal.** Nothing implemented yet. The core mechanism below has been prototyped +standalone and compiles; the integration has not been written. + +Two givens shape everything: + +1. **Every database column is represented by one data member.** A composite primary key is therefore + several members marked `PrimaryKey`; a composite foreign key is several ordinary column members + plus one relation member that ties them to the parent. +2. **`BelongsTo` is not that relation member.** It is inseparably a *column* (storage, + `FieldWithStorage`, a binder over one ODBC column index) as well as a navigator. Widening it would + break the "one member ⇒ one column" invariant; and under given (1) its column half is unnecessary, + because the columns already have their own members. + +## The design: `CompositeForeignKey, ...>` + +The relation is a list of **connections**, each pairing one of this record's columns with the parent +column it references: + +```cpp +struct CkParent +{ + static constexpr std::string_view TableName = "CkParent"; + + // One member per column. Both marked PrimaryKey - already legal today. + Field partA; + Field partB; + Field>, SqlRealName{"caption"}> caption; +}; + +struct CkChild +{ + static constexpr std::string_view TableName = "CkChild"; + + Field id; + + // Ordinary column members. They bind, project and round-trip with no involvement + // from the relation. + Field refA; + Field refB; + + // The relation: a list of connections, each "my column -> their column". + CompositeForeignKey, + Connection<&CkChild::refB, &CkParent::partB>> parent; +}; +``` + +Scales to any width by adding connections — the 3-column form that dominates the surveyed schema is +just three of them: + +```cpp +CompositeForeignKey, + Connection<&Leaf::b, &Hub::k2>, + Connection<&Leaf::c, &Hub::k3>> hub; +``` + +## Why the pairing matters + +Earlier drafts spelled the two sides as separate lists (`RecordMemberList<&Child::refA, &Child::refB>` +paired positionally against the parent's). That works, but leaves the pairing *implicit in position* — +so transposing two same-typed columns is silently wrong and uncatchable by the compiler. + +With `Connection` the pairing is part of the type. A transposition is not a subtle +mis-ordering; it is a different `Connection`, and if the two columns differ in type it does not +compile. This is the decisive advantage over both list-based options. + +## What it derives, and what it rejects + +Each `Connection` destructures its two member pointers — the codebase already has +`MemberClassTypeHelper` for exactly this — into owner record and field type. So the relation +computes rather than restates: + +- `Parent` — the referenced record, from the `Into` pointers. **Never declared by hand.** +- `Child` — the owning record, from the `From` pointers. +- the foreign key values, read through the `From` pointers. + +And it `static_assert`s, all three verified against a standalone prototype: + +| Error | Caught | +|---|---| +| Connections pointing at *different* parent records | ✅ `"all connections must point at the same record"` | +| Connections starting from different child records | ✅ same mechanism | +| A pair whose field types differ (e.g. `int` wired to `long long`) | ✅ `"pairwise field types must match"` | +| Right-hand member not marked `IsPrimaryKey` | ✅ (checkable, not yet prototyped) | +| Transposing two columns *of the same type* | ❌ — inexpressible to catch; but see below | + +The last row is the residual risk, and it is much smaller than with positional lists: a same-typed +transposition is the only remaining silent error, and `ddl2cpp` generating these from the schema means +hand-writing is the exception. + +## How it works + +### Column binding: nothing changes + +`refA` and `refB` are ordinary `Field`s. `RecordColumnCount` is 3 (`id`, `refA`, `refB`) and +the relation contributes 0, because — like `HasMany` and `HasOneThrough` — it has no `Value()` / +`MutableValue()` / `IsModified()`, so it does not satisfy `FieldWithStorage`, therefore not +`RecordColumnMember`. Every projection builder, the output-binding loop and the multi-record column +offset arithmetic skip it automatically, with no special-casing. + +This is why no binder, projection or column-count code is touched. + +### The relation holds no key storage + +Only the loaded target, as `HasOneThrough` does (`std::shared_ptr`). The key values are read +through the `From` pointers on demand: + +```cpp +// prototyped and working +static auto ValuesOf(Child const& c) { return std::tuple { (c.*Cs::From).Value()... }; } +``` + +One copy of each value, in the `Field` that owns the column. Nothing to keep in sync. + +### Loading reuses machinery that already takes N values + +The cheap part. `LoadBelongsTo` today calls `QuerySingle(value)`, and `QuerySingle` already +emits one `WHERE` per primary-key field of the target and binds one argument per placeholder +(`DataMapper.hpp:2152`). For a two-key parent it therefore already produces: + +```sql +SELECT "part_a", "part_b", "caption" FROM "CkParent" WHERE "part_a" = ? AND "part_b" = ? +``` + +The composite loader only has to pass every value instead of one — `std::apply` over the tuple above. +**No new SQL generation, no new binding path.** + +### Ordering — settled by test + +`src/tests/CompositeKeyOrderingTests.cpp` establishes the ground truth: + +- `QuerySingle` emits one predicate per `PrimaryKey` member **in C++ member declaration order**, and + binds arguments positionally. +- Predicate order follows the *C++ declaration*, not the table's column order: the same table mapped + with its key members declared in the opposite order needs the opposite argument order to reach the + same row. +- A transposed argument pair is well-formed, binds cleanly, and returns **a different row** — not an + error. With same-typed key parts (the common case) nothing detects it. + +So the loader must not depend on connections being written in the parent's key order. It sorts the +values itself: for each `Connection`, the `Into` pointer identifies the parent member, from which the +member *index* is recovered; sorting the values by that index yields exactly the order +`QuerySingle` will bind them in. Connections may then be listed in any order, and a transposition +becomes impossible rather than merely detectable. + +### Navigation + +Same surface as `HasOneThrough`, which is the closest existing analogue: `Record()`, `IsLoaded()`, +`Unload()`, `operator->`, `SetAutoLoader()`. So `child.parent->caption` works, and +`ConfigureRelationAutoLoading` gains one branch dispatching on an `IsCompositeForeignKey` trait, +installing a loader that closes over the tuple of key values instead of a single one. + +### `ddl2cpp` generation is mechanical + +The schema reader already reports both ordered column lists, so the generator emits one `Connection` +per column pair in constraint order. The 90 currently-skipped foreign keys and their inverses become +generatable, and the "Foreign keys ignored" warning for them goes away. + +Note this also makes the generator the *primary* author of these declarations, which is what reduces +the same-typed-transposition risk to near zero in practice. + +## The inverse + +`HasMany` on the parent finds its inverse by locating the child's relation member by type +(`InverseBelongsToIndexOf`), which still works: the composite relation is one member. + +Disambiguation has to widen, though. The current selector names a single column +(`HasMany`), and two composite foreign keys from the same child into the +same parent are ambiguous exactly as two single-column ones are. Not hypothetical — the surveyed schema +has a table carrying **three** separate two-column foreign keys into one parent. Options: name the +whole `CompositeForeignKey<...>` type as the selector, or name the child's member pointer directly. +The latter is probably clearer and is a small extension of the existing `RelationSelector` concept. + +## The other half: reflected identity + +Independent of the relation work. `RecordPrimaryKeyType` resolves through `RecordPrimaryKeyIndex`, a +single member index; `GetPrimaryKeyField()` returns the first match. 25 call sites in `DataMapper.hpp`, +2 in `DataMapperAsync.hpp`. + +- **Additive** (recommended first): add `RecordPrimaryKeyTuple` and `GetPrimaryKeyFields()` + alongside; only composite-aware code calls them. Nothing existing changes behaviour. +- **Breaking**: make the existing pair tuples when a record has several `PrimaryKey` members. Cleaner, + but every call site needs auditing and `CreateExplicit` — which *returns* + `RecordPrimaryKeyType` — changes shape. + +The relation work does not depend on the breaking version, so additive first is the lower-risk order. + +## Implementation plan + +Ordered so each step is independently testable and nothing half-built is reachable from the public API. + +**Step 1 — `Connection` and `CompositeForeignKey`, type level only.** +New header `src/Lightweight/DataMapper/CompositeForeignKey.hpp`. +- `Connection`: exposes `From`, `Into`, `FromRecord`, `IntoRecord`, `FromField`, + `IntoField`, derived via the existing `MemberClassType`. +- `CompositeForeignKey`: derives `Child`/`Parent`, exposes `Count`, and + `static_assert`s the three rejections (same parent, same child, pairwise field types). +- `IsCompositeForeignKey` trait, mirroring `IsHasOneThrough`. +- No storage yet beyond `std::shared_ptr`; no DataMapper involvement. +*Tests:* compile-time only — derivation, `Count`, and that the record's `RecordColumnCount` is +unchanged by adding the member (i.e. it is not a `RecordColumnMember`). + +**Step 2 — key extraction in parent-member order.** +- `ValuesOf(Child const&)` returning a tuple read through the `From` pointers. +- `OrderedValuesOf(Child const&)`: the same values permuted into the parent's member declaration + order, using each `Into` pointer to recover the parent member index. This is the piece the ordering + tests above exist to justify. +*Tests:* connections declared out of order still produce parent-order values. + +**Step 3 — navigation surface.** +`Record()`, `IsLoaded()`, `Unload()`, `operator->`, `SetAutoLoader()` — copied in shape from +`HasOneThrough`, which is the closest existing analogue. +*Tests:* default-constructed reports not-loaded; emplace/unload round-trip. + +**Step 4 — auto-loading.** +One branch in `ConfigureRelationAutoLoading` dispatching on `IsCompositeForeignKey`, installing a +loader that `std::apply`s `OrderedValuesOf` into `QuerySingle`. No new SQL generation. +*Tests:* against a live DB, two- and three-column parents, including connections written out of order. + +**Step 5 — reflected identity, additive.** +`RecordPrimaryKeyTuple` and `GetPrimaryKeyFields()` alongside the existing single-key pair, +which is left untouched. Only composite-aware code calls the new ones. +*Tests:* tuple shape for multi-key records; unchanged behaviour for single-key ones. + +**Step 6 — export and document.** +Add to `Lightweight.cppm`, `CMakeLists.txt` header list, and `docs/usage.md`. + +Deferred deliberately, and recorded as such rather than silently skipped: + +- **`ddl2cpp` generation.** Mechanical once the spelling is fixed (the schema reader already reports + both ordered column lists), but it is a separate change on top of a working library API. +- **The inverse (`HasMany` over a composite relation).** Needs the selector to name a column *list*; + the surveyed schema has a table with three separate two-column foreign keys into one parent, so this + is required eventually, not optional. +- **`AutoAssign` / `ServerSideAutoIncrement` semantics across several key columns.** Server-side + auto-increment is meaningless for a multi-column key and wants an explicit `static_assert` + rejection. +- **C++26 reflection branch.** No binder or column arithmetic changes here, so it should need nothing — + to be confirmed by building that configuration, not assumed. + +## Prototype + +The mechanism was checked standalone before proposing it: `Connection` destructuring, `Parent`/`Child` +derivation, `ValuesOf` extraction at 2 and 3 columns, and the three `static_assert` rejections all +behave as described. What is *not* prototyped is the integration — the trait, the +`ConfigureRelationAutoLoading` branch, the loader, and the generator. diff --git a/src/Lightweight/CMakeLists.txt b/src/Lightweight/CMakeLists.txt index 5d9366093..e233a73ca 100644 --- a/src/Lightweight/CMakeLists.txt +++ b/src/Lightweight/CMakeLists.txt @@ -65,6 +65,7 @@ set(HEADER_FILES SqlQuery/Update.hpp DataMapper/BelongsTo.hpp + DataMapper/CompositeForeignKey.hpp DataMapper/DataMapper.hpp DataMapper/Error.hpp DataMapper/Pool.hpp diff --git a/src/Lightweight/DataMapper/CompositeForeignKey.hpp b/src/Lightweight/DataMapper/CompositeForeignKey.hpp new file mode 100644 index 000000000..b4478f93a --- /dev/null +++ b/src/Lightweight/DataMapper/CompositeForeignKey.hpp @@ -0,0 +1,314 @@ +// SPDX-License-Identifier: Apache-2.0 + +#pragma once + +#include "../SqlStatement.hpp" +#include "../Utils.hpp" +#include "Error.hpp" +#include "Record.hpp" + +#include +#include +#include +#include +#include +#include +#include +#include + +namespace Lightweight +{ + +/// @brief One column pair of a composite foreign key: "this record's column references that one". +/// +/// Both endpoints are pointers-to-member, so the pairing is part of the type. That is the whole point: +/// with two parallel column *lists* the pairing would be implicit in position, and transposing two +/// same-typed columns would be silently wrong. Here a transposition is a different `Connection`, and +/// where the paired columns differ in type it does not even compile. +/// +/// @tparam FromPtr Pointer to the member of *this* record holding part of the foreign key. +/// @tparam IntoPtr Pointer to the member of the referenced record it points at, which must be a +/// primary key there. +/// +/// @ingroup DataMapper +/// +/// @code +/// CompositeForeignKey, +/// Connection<&Child::refB, &Parent::partB>> parent; +/// @endcode +template +struct Connection +{ + /// Pointer to this record's foreign key member. + static constexpr auto From = FromPtr; + + /// Pointer to the referenced record's primary key member. + static constexpr auto Into = IntoPtr; + +#if defined(LIGHTWEIGHT_CXX26_REFLECTION) + /// The record this connection starts from. + using FromRecord = MemberClassType; + + /// The record this connection points at. + using IntoRecord = MemberClassType; +#else + /// The record this connection starts from. + using FromRecord = MemberClassType; + + /// The record this connection points at. + using IntoRecord = MemberClassType; +#endif + + /// The field type on this record's side, e.g. `Field`. + using FromField = std::remove_cvref_t().*FromPtr)>; + + /// The field type on the referenced record's side. + using IntoField = std::remove_cvref_t().*IntoPtr)>; + + /// Member index of the referenced member within the referenced record. + /// + /// This is what makes the relation independent of the order its connections are written in: the + /// `WHERE` clause of a primary key lookup is emitted in the referenced record's *member + /// declaration* order, so values must be permuted into that order before being bound. See + /// @ref CompositeForeignKey::OrderedValuesOf. + static constexpr std::size_t IntoMemberIndex = MemberIndexOf; +}; + +namespace detail +{ + template + struct IsConnectionType: std::false_type + { + }; + + template + struct IsConnectionType>: std::true_type + { + }; +} // namespace detail + +/// @brief Satisfied by @ref Connection specializations. +/// +/// @ingroup DataMapper +template +concept ConnectionType = detail::IsConnectionType>::value; + +/// @brief Represents a foreign key spanning several columns. +/// +/// Declared as a list of @ref Connection, each pairing one of this record's columns with the column it +/// references. The referenced and referencing records are *derived* from those pointers rather than +/// named again, so they cannot disagree with the connections. +/// +/// This member holds no column of its own. Every foreign key column is an ordinary `Field` on the +/// record - one data member per database column - and this relation only ties them together and +/// navigates. It therefore contributes nothing to `RecordColumnCount` and is skipped by every +/// projection, exactly as `HasMany` and `HasOneThrough` are. +/// +/// Connections may be listed in any order. Values are permuted into the referenced record's member +/// order before binding, because that is the order a primary key lookup emits its predicates in - see +/// @ref OrderedValuesOf and `src/tests/CompositeKeyOrderingTests.cpp`. +/// +/// @tparam Connections One @ref Connection per column of the foreign key. +/// +/// @ingroup DataMapper +/// +/// @code +/// struct Parent +/// { +/// Field partA; +/// Field partB; +/// }; +/// struct Child +/// { +/// Field id; +/// Field refA; +/// Field refB; +/// CompositeForeignKey, +/// Connection<&Child::refB, &Parent::partB>> parent; +/// }; +/// @endcode +template +class CompositeForeignKey +{ + static_assert(sizeof...(Connections) > 0, "A composite foreign key must connect at least one column."); + + public: + /// Number of columns this foreign key spans. + static constexpr std::size_t Count = sizeof...(Connections); + + /// The record holding the foreign key, i.e. the one declaring this member. + using Child = std::tuple_element_t<0, std::tuple>; + + /// The referenced record. + using ReferencedRecord = std::tuple_element_t<0, std::tuple>; + + static_assert((std::same_as && ...), + "Every Connection of a composite foreign key must start at the same record. " + "Check that each Connection's first pointer-to-member names this record."); + + static_assert((std::same_as && ...), + "Every Connection of a composite foreign key must point at the same record. " + "A foreign key references one table; splitting it across two is not expressible."); + + // Compared by *value* type rather than by field type: `SqlRealName` is part of `Field`'s type, so + // two columns holding the same kind of value have different field types whenever their column + // names differ - which is almost always. The value type is what has to line up for the comparison + // the database will perform. + static_assert((std::same_as + && ...), + "Each connected column pair must hold the same value type. A mismatch here usually " + "means two connections were transposed."); + + static_assert((Connections::IntoField::IsPrimaryKey && ...), + "A composite foreign key must reference primary key columns. " + "Check the PrimaryKey marker on the referenced record's members."); + + /// The tuple of foreign key values, in the order the connections are declared. + using ValueType = std::tuple; + + /// Reads this record's foreign key values, in the order the connections are declared. + /// + /// The values are not stored on the relation: there is exactly one copy of each, in the `Field` + /// that owns the column, so nothing can fall out of sync. + /// + /// @param record The record holding the foreign key. + /// @return The values, in declaration order of the connections. + [[nodiscard]] static ValueType ValuesOf(Child const& record) + { + return ValueType { (record.*Connections::From).Value()... }; + } + + /// Reads this record's foreign key values, permuted into the referenced record's member order. + /// + /// This is the order they must be bound in. A primary key lookup emits one `WHERE` predicate per + /// primary key member, in that record's *member declaration* order, and binds its arguments + /// positionally - so passing values in connection order would bind them to the wrong predicates + /// whenever the two orders differ. With same-typed key columns that yields a wrong row rather than + /// an error, which is why the permutation is done here rather than left to the caller. + /// + /// @param record The record holding the foreign key. + /// @return The values, ordered to match the referenced record's primary key members. + [[nodiscard]] static ValueType OrderedValuesOf(Child const& record) + { + auto values = ValuesOf(record); + auto ordered = values; + + // Rank each connection by its referenced member index; the rank is the slot its value belongs + // in. Done over indices rather than by sorting a runtime container so the whole permutation is + // fixed at compile time for the common (already-ordered) case. + constexpr auto intoIndices = std::array { Connections::IntoMemberIndex... }; + + [&](std::index_sequence) { + ( + [&] { + // How many connections reference an earlier member than this one does. + auto rank = std::size_t { 0 }; + for (auto const other: intoIndices) + if (other < intoIndices[I]) + ++rank; + AssignAt(ordered, rank, std::get(values)); + }(), + ...); + }(std::index_sequence_for {}); + + return ordered; + } + + /// @return The referenced record, loading it on first access. + [[nodiscard]] LIGHTWEIGHT_FORCE_INLINE ReferencedRecord const& Record() const + { + RequireLoaded(); + return *_record; + } + + /// @return `true` if the referenced record has been loaded. + [[nodiscard]] LIGHTWEIGHT_FORCE_INLINE constexpr bool IsLoaded() const noexcept + { + return _record.get() != nullptr; + } + + /// Discards the loaded record, so the next access loads it again. + LIGHTWEIGHT_FORCE_INLINE void Unload() noexcept + { + _record.reset(); + } + + /// Adopts an already-fetched referenced record, marking the relation loaded. + /// + /// @param record The fetched record. + LIGHTWEIGHT_FORCE_INLINE constexpr void EmplaceRecord(std::shared_ptr record) noexcept + { + _record = std::move(record); + } + + /// @return A pointer to the referenced record, loading it on first access. + [[nodiscard]] LIGHTWEIGHT_FORCE_INLINE constexpr ReferencedRecord const* operator->() const + { + RequireLoaded(); + return _record.get(); + } + + /// Carries the deferred load, installed by the DataMapper. + struct Loader + { + std::function()> loadReference {}; + }; + + /// Used internally to configure on-demand loading of the referenced record. + /// + /// @param loader The loader to install. + void SetAutoLoader(Loader loader) + { + _loader = std::move(loader); + } + + private: + /// Assigns @p value into the tuple slot at the runtime index @p slot. + /// + /// A tuple cannot be indexed at runtime, so the index is matched against the compile-time pack. + /// Every element of the permutation has the same type here - the static_asserts above guarantee + /// pairwise type equality - so only the matching slot is written. + template + static constexpr void AssignAt(Tuple& tuple, std::size_t slot, Value const& value) + { + [&](std::index_sequence) { + (((I == slot) ? (void) (std::get(tuple) = value) : (void) 0), ...); + }(std::make_index_sequence> {}); + } + + void RequireLoaded() const + { + if (_record) + return; + + if (_loader.loadReference) + _record = _loader.loadReference(); + + if (!_record) + throw SqlRequireLoadedError(Reflection::TypeNameOf>); + } + + Loader _loader {}; + mutable std::shared_ptr _record {}; +}; + +namespace detail +{ + template + struct IsCompositeForeignKeyType: std::false_type + { + }; + + template + struct IsCompositeForeignKeyType>: std::true_type + { + }; +} // namespace detail + +/// @brief Whether @p T is a @ref CompositeForeignKey. +/// +/// @ingroup DataMapper +template +constexpr bool IsCompositeForeignKey = detail::IsCompositeForeignKeyType>::value; + +} // namespace Lightweight diff --git a/src/Lightweight/DataMapper/DataMapper.hpp b/src/Lightweight/DataMapper/DataMapper.hpp index 4a9ddd478..550d42047 100644 --- a/src/Lightweight/DataMapper/DataMapper.hpp +++ b/src/Lightweight/DataMapper/DataMapper.hpp @@ -10,6 +10,7 @@ #include "../Utils.hpp" #include "BelongsTo.hpp" #include "CollectDifferences.hpp" +#include "CompositeForeignKey.hpp" #include "Field.hpp" #include "HasMany.hpp" #include "HasManyThrough.hpp" @@ -2870,6 +2871,31 @@ void DataMapper::ConfigureRelationAutoLoading(Record& record) }, }); } + if constexpr (IsCompositeForeignKey) + { + using ReferencedRecord = typename FieldType::ReferencedRecord; + + // The key values are read out of the record's own Field members and captured by value, so + // the loader does not depend on the record still being alive at load time. + // + // OrderedValuesOf() - not ValuesOf() - because QuerySingle emits one WHERE predicate per + // primary key member in the *referenced record's* member declaration order and binds its + // arguments positionally. Passing them in connection-declaration order would bind each + // value to the wrong predicate whenever the two orders differ, which with same-typed key + // columns fetches a wrong row rather than failing. See CompositeKeyOrderingTests.cpp. + auto const keyValues = FieldType::OrderedValuesOf(record); + + field.SetAutoLoader(typename FieldType::Loader { + .loadReference = [keyValues]() -> std::shared_ptr { + DataMapper& dm = DataMapper::AcquireThreadLocal(); + auto loaded = std::apply([&dm](auto const&... key) { return dm.QuerySingle(key...); }, + keyValues); + if (!loaded) + return {}; + return std::make_shared(std::move(*loaded)); + }, + }); + } if constexpr (IsHasMany) { if constexpr (HasPrimaryKey) diff --git a/src/Lightweight/DataMapper/Record.hpp b/src/Lightweight/DataMapper/Record.hpp index 752af9247..3403b8ce2 100644 --- a/src/Lightweight/DataMapper/Record.hpp +++ b/src/Lightweight/DataMapper/Record.hpp @@ -12,6 +12,7 @@ #include #include #include +#include namespace Lightweight { @@ -360,6 +361,96 @@ template constexpr bool HasAutoIncrementPrimaryKey = detail::CheckFieldProperty<[]() { return IsAutoIncrementPrimaryKey; }, T>; +namespace detail +{ + /// Collects the value types of every `PrimaryKey` member of @p Record, in declaration order. + template + struct RecordPrimaryKeyTupleHelper + { + using type = decltype([](std::index_sequence) { + return std::tuple_cat([]() { + using FieldType = RecordMemberTypeOf; + // The two conditions must nest rather than share one `if constexpr`: `IsPrimaryKey` is + // not a member of the relation types (HasMany, CompositeForeignKey, ...), and `&&` + // inside a single condition would still instantiate the right-hand side for them. + if constexpr (IsField) + { + if constexpr (FieldType::IsPrimaryKey) + return std::tuple {}; + else + return std::tuple<> {}; + } + else + return std::tuple<> {}; + }.template operator()()...); + }(std::make_index_sequence> {})); + }; +} // namespace detail + +/// @brief The tuple of a record's primary key value types, in member declaration order. +/// +/// Unlike @ref RecordPrimaryKeyType, which names a single field's type, this covers composite keys: +/// for a record with several members marked `PrimaryKey` it is a tuple of all of them. For a +/// single-key record it is a one-element tuple. +/// +/// Added alongside the single-key helpers rather than replacing them, so no existing caller changes +/// behaviour; only composite-aware code reaches for this. +/// +/// @ingroup DataMapper +template +using RecordPrimaryKeyTuple = typename detail::RecordPrimaryKeyTupleHelper::type; + +/// @brief Number of members of @p Record marked as a primary key. +/// +/// One for an ordinary record, more for a composite key, zero for a keyless record. +/// +/// @ingroup DataMapper +template +constexpr std::size_t RecordPrimaryKeyCount = std::tuple_size_v>; + +/// @brief Whether @p Record's identity spans more than one column. +/// +/// @ingroup DataMapper +template +constexpr bool HasCompositePrimaryKey = RecordPrimaryKeyCount > 1; + +/// @brief Reads every primary key value of @p record, in member declaration order. +/// +/// This is the order a primary key lookup binds its arguments in, so the returned tuple can be applied +/// straight to `QuerySingle`/`Update`/`Delete`. +/// +/// @param record Record to read. +/// @return The key values as a tuple. +/// +/// @ingroup DataMapper +template +[[nodiscard]] RecordPrimaryKeyTuple GetPrimaryKeyFields(Record const& record) +{ + auto result = RecordPrimaryKeyTuple {}; + auto slot = std::size_t { 0 }; + EnumerateRecordMembers(record, [&](FieldType const& field) { + if constexpr (IsField) + if constexpr (FieldType::IsPrimaryKey) + { + // The tuple was built by walking the members in this same order, so the Nth key member + // corresponds to tuple element N. The tuple is heterogeneous, so the write is matched + // against the compile-time index and only the assignable slot is emitted. + [&](std::index_sequence) { + ( + [&] { + if constexpr (std::assignable_from>&, + typename FieldType::ValueType const&>) + if (J == slot) + std::get(result) = field.Value(); + }(), + ...); + }(std::make_index_sequence> {}); + ++slot; + } + }); + return result; +} + /// Returns the first primary key field of the record. /// /// @ingroup DataMapper diff --git a/src/Lightweight/Lightweight.cppm b/src/Lightweight/Lightweight.cppm index c4041ee68..086464755 100644 --- a/src/Lightweight/Lightweight.cppm +++ b/src/Lightweight/Lightweight.cppm @@ -40,6 +40,9 @@ using Lightweight::AliasedTableName; using Lightweight::AutoDetectRelation; using Lightweight::BelongsTo; using Lightweight::BuildConnectionString; +using Lightweight::ConnectionType; +using Lightweight::Connection; +using Lightweight::CompositeForeignKey; using Lightweight::ConvertWindows1252ToUtf8; using Lightweight::DataMapper; using Lightweight::DataMapperOptions; @@ -55,11 +58,13 @@ using Lightweight::FormatName; using Lightweight::FormatType; using Lightweight::FullyQualifiedNameOf; using Lightweight::GetPrimaryKeyField; +using Lightweight::GetPrimaryKeyFields; using Lightweight::HasAutoIncrementPrimaryKey; using Lightweight::HasMany; using Lightweight::HasManyThrough; using Lightweight::HasOneThrough; using Lightweight::HasPrimaryKey; +using Lightweight::HasCompositePrimaryKey; using Lightweight::IndexType; using Lightweight::Int128; // the unscaled carrier in SqlNumeric::ToUnscaledValue()'s return type using Lightweight::Int64DataBinderHelper; @@ -67,6 +72,7 @@ using Lightweight::InverseBelongsToFieldNameOf; using Lightweight::InverseBelongsToIndexOf; using Lightweight::IsAutoIncrementPrimaryKey; using Lightweight::IsBelongsTo; +using Lightweight::IsCompositeForeignKey; using Lightweight::IsField; using Lightweight::IsForeignKeyViolation; using Lightweight::IsHasMany; @@ -97,6 +103,8 @@ using Lightweight::RecordColumnMember; using Lightweight::RecordPrimaryKeyIndex; using Lightweight::RecordPrimaryKeyOf; using Lightweight::RecordPrimaryKeyType; +using Lightweight::RecordPrimaryKeyCount; +using Lightweight::RecordPrimaryKeyTuple; using Lightweight::RecordStorageFieldCount; using Lightweight::RecordTableName; using Lightweight::RecordWithStorageFields; diff --git a/src/tests/CMakeLists.txt b/src/tests/CMakeLists.txt index 2d2b4e0c4..916d053db 100644 --- a/src/tests/CMakeLists.txt +++ b/src/tests/CMakeLists.txt @@ -38,6 +38,9 @@ endif() set(SOURCE_FILES ConfigProfileStoreTests.cpp CoreTests.cpp + CompositeForeignKeyTests.cpp + CompositeKeyGapTests.cpp + CompositeKeyOrderingTests.cpp CxxModelPrinterTests.cpp CxxModelRelationTests.cpp DataBinderTests.cpp diff --git a/src/tests/CompositeForeignKeyTests.cpp b/src/tests/CompositeForeignKeyTests.cpp new file mode 100644 index 000000000..cd6df5c25 --- /dev/null +++ b/src/tests/CompositeForeignKeyTests.cpp @@ -0,0 +1,386 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// `CompositeForeignKey, ...>` - a foreign key spanning several columns. +// +// Design and rationale: `docs/composite-keys-design.md`. +// Ordering ground truth this builds on: `src/tests/CompositeKeyOrderingTests.cpp`. +// +// The two properties worth stating up front, because they are what the tests below check: +// +// 1. The relation holds no column. Every foreign key column is an ordinary `Field` - one data member +// per database column - so `RecordColumnCount` is unchanged by adding the relation, and every +// projection skips it automatically. +// 2. Connections may be written in any order. Values are permuted into the referenced record's +// member order before binding, because a primary key lookup emits its predicates in that order +// and binds positionally. Getting this wrong yields a wrong row rather than an error, so it is +// tested directly. + +#include "Utils.hpp" + +#include + +#include + +#include + +using namespace Lightweight; + +namespace CompositeFk +{ + +/// Referenced record with a two-column primary key. +struct Parent +{ + static constexpr std::string_view TableName = "CfkParent"; + + Field partA {}; + Field partB {}; + Field>, SqlRealName { "caption" }> caption {}; +}; + +/// Referencing record, connections written in the same order the parent declares its key. +struct Child +{ + static constexpr std::string_view TableName = "CfkChild"; + + Field id {}; + Field refA {}; + Field refB {}; + + CompositeForeignKey, Connection<&Child::refB, &Parent::partB>> parent {}; +}; + +/// The same relation with its connections written in the *opposite* order. Semantically identical - +/// the relation permutes values into parent-member order regardless of how it was declared. +struct ChildDeclaredBackwards +{ + static constexpr std::string_view TableName = "CfkChild"; + + Field id {}; + Field refA {}; + Field refB {}; + + CompositeForeignKey, + Connection<&ChildDeclaredBackwards::refA, &Parent::partA>> + parent {}; +}; + +/// A three-column parent, the width that dominates the production schema surveyed in +/// `docs/ddl2cpp-relation-generation.md`. +struct WideParent +{ + static constexpr std::string_view TableName = "CfkWideParent"; + + Field k1 {}; + Field k2 {}; + Field k3 {}; + Field>, SqlRealName { "note" }> note {}; +}; + +/// Three connections, deliberately scrambled relative to the parent's member order. +struct WideChild +{ + static constexpr std::string_view TableName = "CfkWideChild"; + + Field id {}; + Field a {}; + Field b {}; + Field c {}; + + CompositeForeignKey, + Connection<&WideChild::a, &WideParent::k1>, + Connection<&WideChild::b, &WideParent::k2>> + parent {}; +}; + +} // namespace CompositeFk + +using CompositeFk::Child; +using CompositeFk::ChildDeclaredBackwards; +using CompositeFk::Parent; +using CompositeFk::WideChild; +using CompositeFk::WideParent; + +// Hoisted to namespace scope: STATIC_CHECK expands its argument inside a macro, where a +// function-local `using` declared in the same statement is not yet visible. +using ChildRelation = decltype(Child::parent); +using BackwardsRelation = decltype(ChildDeclaredBackwards::parent); +using WideRelation = decltype(WideChild::parent); + +// ================================================================================================ +// Step 1: type-level derivation +// ================================================================================================ + +TEST_CASE("CompositeForeignKey derives both records from its connections", "[CompositeForeignKey]") +{ + // Neither record is named in the declaration - both are computed from the member pointers, so they + // cannot disagree with the connections. + STATIC_CHECK(std::same_as); + STATIC_CHECK(std::same_as); + STATIC_CHECK(ChildRelation::Count == 2); + + // Three columns works the same way. + STATIC_CHECK(std::same_as); + STATIC_CHECK(WideRelation::Count == 3); +} + +TEST_CASE("Connection exposes both endpoints and the referenced member index", "[CompositeForeignKey]") +{ + using First = Connection<&Child::refA, &Parent::partA>; + using Second = Connection<&Child::refB, &Parent::partB>; + + STATIC_CHECK(std::same_as); + STATIC_CHECK(std::same_as); + + // The referenced member index is what drives the permutation: partA is member 0 of Parent, partB + // member 1. Recovered from the pointer, not restated. + STATIC_CHECK(First::IntoMemberIndex == 0); + STATIC_CHECK(Second::IntoMemberIndex == 1); +} + +TEST_CASE("CompositeForeignKey is not a column member", "[CompositeForeignKey]") +{ + // The property that keeps every binder, projection and column-offset path untouched: the relation + // has no Value()/MutableValue()/IsModified(), so it is not FieldWithStorage, therefore not + // RecordColumnMember - exactly like HasMany and HasOneThrough. + STATIC_CHECK_FALSE(FieldWithStorage); + STATIC_CHECK_FALSE(RecordColumnMember); + STATIC_CHECK(IsCompositeForeignKey); + + // So the record's column count covers exactly its three real columns, and the relation adds none. + STATIC_CHECK(RecordColumnCount == 3); + STATIC_CHECK(RecordMemberCount == 4); // ...though it is still a member + STATIC_CHECK(RecordColumnCount == 4); +} + +// ================================================================================================ +// Step 2: value extraction and ordering +// ================================================================================================ + +TEST_CASE("ValuesOf reads the foreign key from the record's own fields", "[CompositeForeignKey]") +{ + // The values live in the Field members - one copy each - and are read through the connections' + // `From` pointers. The relation stores nothing, so nothing can fall out of sync. + auto child = Child {}; + child.refA = 7; + child.refB = 9; + + auto const values = ChildRelation::ValuesOf(child); + CHECK(std::get<0>(values) == 7); + CHECK(std::get<1>(values) == 9); +} + +TEST_CASE("OrderedValuesOf permutes into the referenced record's member order", "[CompositeForeignKey]") +{ + // Declared in parent order already: the permutation is the identity. + auto child = Child {}; + child.refA = 1; + child.refB = 2; + auto const ordered = decltype(Child::parent)::OrderedValuesOf(child); + CHECK(std::get<0>(ordered) == 1); // partA + CHECK(std::get<1>(ordered) == 2); // partB + + // Declared backwards: refB->partB is written first, but partB is the parent's *second* member, so + // the value must still land in slot 1. This is the case that would silently fetch a wrong row if + // the relation bound values in declaration order. + auto backwards = ChildDeclaredBackwards {}; + backwards.refA = 1; + backwards.refB = 2; + auto const orderedBackwards = decltype(ChildDeclaredBackwards::parent)::OrderedValuesOf(backwards); + CHECK(std::get<0>(orderedBackwards) == 1); // partA, despite being declared second + CHECK(std::get<1>(orderedBackwards) == 2); // partB, despite being declared first +} + +TEST_CASE("OrderedValuesOf handles a scrambled three-column key", "[CompositeForeignKey]") +{ + // Connections written (c->k3, a->k1, b->k2). The parent declares k1, k2, k3, so the values must + // come out as (a, b, c) regardless. + auto child = WideChild {}; + child.a = 10; + child.b = 20; + child.c = 30; + + auto const ordered = decltype(WideChild::parent)::OrderedValuesOf(child); + CHECK(std::get<0>(ordered) == 10); // k1 <- a + CHECK(std::get<1>(ordered) == 20); // k2 <- b + CHECK(std::get<2>(ordered) == 30); // k3 <- c +} + +// ================================================================================================ +// Step 3: navigation surface +// ================================================================================================ + +TEST_CASE("CompositeForeignKey navigation reports load state", "[CompositeForeignKey]") +{ + auto child = Child {}; + CHECK_FALSE(child.parent.IsLoaded()); + + // Emplacing a record marks it loaded and makes it reachable. + auto parent = std::make_shared(); + parent->partA = 1; + parent->partB = 2; + parent->caption = SqlAnsiString<20> { "hello" }; + child.parent.EmplaceRecord(parent); + + REQUIRE(child.parent.IsLoaded()); + CHECK(child.parent.Record().partA.Value() == 1); + CHECK(child.parent->partB.Value() == 2); + + // ...and unloading reverts it. + child.parent.Unload(); + CHECK_FALSE(child.parent.IsLoaded()); +} + +// ================================================================================================ +// Step 4: loading against a live database +// ================================================================================================ + +TEST_CASE_METHOD(SqlTestFixture, "CompositeForeignKey loads its referenced record", "[CompositeForeignKey]") +{ + auto stmt = SqlStatement {}; + (void) stmt.ExecuteDirect(R"(CREATE TABLE "CfkParent" ( + "part_a" INT NOT NULL, + "part_b" INT NOT NULL, + "caption" VARCHAR(20) NULL, + PRIMARY KEY ("part_a", "part_b") + ))"); + (void) stmt.ExecuteDirect(R"(CREATE TABLE "CfkChild" ( + "id" INT NOT NULL PRIMARY KEY, + "ref_a" INT NOT NULL, + "ref_b" INT NOT NULL + ))"); + + // Two parents whose key parts are transpositions of each other, so binding them in the wrong order + // reaches the wrong row rather than failing. + (void) stmt.ExecuteDirect(R"(INSERT INTO "CfkParent" VALUES (1, 2, 'one-two'))"); + (void) stmt.ExecuteDirect(R"(INSERT INTO "CfkParent" VALUES (2, 1, 'two-one'))"); + (void) stmt.ExecuteDirect(R"(INSERT INTO "CfkChild" VALUES (100, 1, 2))"); + + auto dm = DataMapper {}; + + auto child = dm.QuerySingle(100); + REQUIRE(child.has_value()); + // NOLINTBEGIN(bugprone-unchecked-optional-access) - guarded above + CHECK(child->refA.Value() == 1); + CHECK(child->refB.Value() == 2); + + // The relation is lazy: nothing is fetched until the record is touched. + CHECK_FALSE(child->parent.IsLoaded()); + + // Touching it resolves to (part_a=1, part_b=2), not to the transposed row... + CHECK(child->parent.Record().partA.Value() == 1); + CHECK(child->parent.IsLoaded()); // ...and it is cached afterwards + CHECK(child->parent.Record().partB.Value() == 2); + REQUIRE(child->parent.Record().caption.Value().has_value()); + CHECK(child->parent.Record().caption.Value().value() == "one-two"); + // NOLINTEND(bugprone-unchecked-optional-access) +} + +TEST_CASE_METHOD(SqlTestFixture, + "CompositeForeignKey declared out of order still loads the right record", + "[CompositeForeignKey]") +{ + // The regression this design exists to prevent. `ChildDeclaredBackwards` maps the same table with + // its connections reversed; without the permutation it would bind (2, 1) and fetch 'two-one'. + auto stmt = SqlStatement {}; + (void) stmt.ExecuteDirect(R"(CREATE TABLE "CfkParent" ( + "part_a" INT NOT NULL, + "part_b" INT NOT NULL, + "caption" VARCHAR(20) NULL, + PRIMARY KEY ("part_a", "part_b") + ))"); + (void) stmt.ExecuteDirect(R"(CREATE TABLE "CfkChild" ( + "id" INT NOT NULL PRIMARY KEY, + "ref_a" INT NOT NULL, + "ref_b" INT NOT NULL + ))"); + (void) stmt.ExecuteDirect(R"(INSERT INTO "CfkParent" VALUES (1, 2, 'one-two'))"); + (void) stmt.ExecuteDirect(R"(INSERT INTO "CfkParent" VALUES (2, 1, 'two-one'))"); + (void) stmt.ExecuteDirect(R"(INSERT INTO "CfkChild" VALUES (100, 1, 2))"); + + auto dm = DataMapper {}; + + auto child = dm.QuerySingle(100); + REQUIRE(child.has_value()); + // NOLINTBEGIN(bugprone-unchecked-optional-access) - guarded above + REQUIRE(child->parent.Record().caption.Value().has_value()); + CHECK(child->parent.Record().caption.Value().value() == "one-two"); // not 'two-one' + // NOLINTEND(bugprone-unchecked-optional-access) +} + +// ================================================================================================ +// Step 5: reflected identity, additive +// ================================================================================================ + +TEST_CASE("RecordPrimaryKeyTuple covers every primary key member", "[CompositeForeignKey][identity]") +{ + // The single-key helpers resolve one member index, so they see only the first key of a composite + // record. These are added alongside rather than replacing them, so nothing existing changes + // behaviour. + STATIC_CHECK(RecordPrimaryKeyCount == 2); + STATIC_CHECK(HasCompositePrimaryKey); + STATIC_CHECK(std::same_as, std::tuple>); + + STATIC_CHECK(RecordPrimaryKeyCount == 3); + STATIC_CHECK(std::same_as, std::tuple>); + + // A single-key record yields a one-element tuple and is not composite, so callers can treat both + // uniformly without special-casing. + STATIC_CHECK(RecordPrimaryKeyCount == 1); + STATIC_CHECK_FALSE(HasCompositePrimaryKey); + STATIC_CHECK(std::same_as, std::tuple>); + + // ...and the existing single-key helper is untouched. + STATIC_CHECK(std::same_as, int32_t>); +} + +TEST_CASE("GetPrimaryKeyFields reads every key value in member order", "[CompositeForeignKey][identity]") +{ + auto parent = Parent {}; + parent.partA = 4; + parent.partB = 5; + + auto const keys = GetPrimaryKeyFields(parent); + CHECK(std::get<0>(keys) == 4); + CHECK(std::get<1>(keys) == 5); + + // Three columns, and distinct values so a mis-ordered read would be visible. + auto wide = WideParent {}; + wide.k1 = 10; + wide.k2 = 20; + wide.k3 = 30; + auto const wideKeys = GetPrimaryKeyFields(wide); + CHECK(std::get<0>(wideKeys) == 10); + CHECK(std::get<1>(wideKeys) == 20); + CHECK(std::get<2>(wideKeys) == 30); + + // The tuple order matches what QuerySingle binds, so it can be applied directly. + auto const single = GetPrimaryKeyFields(Child {}); + STATIC_CHECK(std::tuple_size_v == 1); +} + +TEST_CASE_METHOD(SqlTestFixture, "CompositeForeignKey loads across three columns", "[CompositeForeignKey]") +{ + auto stmt = SqlStatement {}; + (void) stmt.ExecuteDirect(R"(CREATE TABLE "CfkWideParent" ( + "k1" INT NOT NULL, "k2" INT NOT NULL, "k3" INT NOT NULL, + "note" VARCHAR(20) NULL, + PRIMARY KEY ("k1", "k2", "k3") + ))"); + (void) stmt.ExecuteDirect(R"(CREATE TABLE "CfkWideChild" ( + "id" INT NOT NULL PRIMARY KEY, + "a" INT NOT NULL, "b" INT NOT NULL, "c" INT NOT NULL + ))"); + (void) stmt.ExecuteDirect(R"(INSERT INTO "CfkWideParent" VALUES (10, 20, 30, 'target'))"); + (void) stmt.ExecuteDirect(R"(INSERT INTO "CfkWideParent" VALUES (30, 20, 10, 'decoy'))"); + (void) stmt.ExecuteDirect(R"(INSERT INTO "CfkWideChild" VALUES (1, 10, 20, 30))"); + + auto dm = DataMapper {}; + + auto child = dm.QuerySingle(1); + REQUIRE(child.has_value()); + // NOLINTBEGIN(bugprone-unchecked-optional-access) - guarded above + REQUIRE(child->parent.Record().note.Value().has_value()); + CHECK(child->parent.Record().note.Value().value() == "target"); // not the reversed decoy + // NOLINTEND(bugprone-unchecked-optional-access) +} diff --git a/src/tests/CompositeKeyGapTests.cpp b/src/tests/CompositeKeyGapTests.cpp new file mode 100644 index 000000000..fc341ad7a --- /dev/null +++ b/src/tests/CompositeKeyGapTests.cpp @@ -0,0 +1,260 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// Composite keys, end to end: raw SQL DDL declaring them, through to the record mapping. +// +// A composite foreign key references a composite primary key, so both sides need the same notion of +// "a key spans several columns". Under the guiding principle that every database column is one data +// member, that means several members marked `PrimaryKey` on the referenced side, and several ordinary +// column members plus one `CompositeForeignKey` relation on the referencing side. +// +// Where each layer stands: +// +// - **Schema reading**: reports both column lists, in order. Never needed changing. +// - **Row lookup**: `QuerySingle`/`Update`/`Delete` emit one `WHERE` per primary-key field and bind +// one argument each, so fetching by a whole composite key already worked. +// - **Reflected identity**: `RecordPrimaryKeyType` / `GetPrimaryKeyField()` still resolve a *single* +// member index, deliberately - `RecordPrimaryKeyTuple` / `GetPrimaryKeyFields()` were added beside +// them so no existing caller changes shape. +// - **Relations**: spelled as `CompositeForeignKey, ...>`. `BelongsTo` was left +// alone; see `docs/composite-keys-design.md` for why. +// +// Behavioural coverage of the relation itself lives in `CompositeForeignKeyTests.cpp`; the predicate +// ordering it depends on is pinned in `CompositeKeyOrderingTests.cpp`. This file covers the DDL-facing +// end: what the schema reader sees, and that a composite-key table still maps as a plain record. +// +// The shapes come from a production MS SQL Server schema surveyed for +// `docs/ddl2cpp-relation-generation.md` (686 tables, 1860 foreign keys, 170 composite primary keys and +// 90 composite foreign keys concentrated on 21 parent tables). The table and column names here are +// deliberately generic and carry no resemblance to it - only the shapes are reproduced. +// +// The DDL is raw SQL rather than `CreateTable<>()` so the C++ mapping is tested against +// independently-declared tables rather than against its own generator. + +#include "Utils.hpp" + +#include + +#include + +#include +#include + +using namespace Lightweight; + +// ================================================================================================ +// Composite PRIMARY key +// ================================================================================================ + +TEST_CASE_METHOD(SqlTestFixture, "Composite primary key is read back from the schema", "[CompositeKey][SqlSchema]") +{ + // The schema reader handles this correctly - it is only the record mapping that cannot express + // it. Asserting that here pins the boundary: whatever a fix does, it does not need to change + // schema reading. + auto stmt = SqlStatement {}; + + (void) stmt.ExecuteDirect(R"(CREATE TABLE "CkComposite" ( + "tenant_id" INT NOT NULL, + "entry_no" INT NOT NULL, + "label" VARCHAR(40) NULL, + PRIMARY KEY ("tenant_id", "entry_no") + ))"); + + auto const tables = SqlSchema::ReadAllTables(stmt, stmt.Connection().DatabaseName(), /*schema=*/""); + auto const table = std::ranges::find_if(tables, [](SqlSchema::Table const& t) { return t.name == "CkComposite"; }); + REQUIRE(table != tables.end()); + + // Both key columns are reported, in declaration order. + REQUIRE(table->primaryKeys.size() == 2); + CHECK(table->primaryKeys[0] == "tenant_id"); + CHECK(table->primaryKeys[1] == "entry_no"); + + // ...and both are flagged on the columns themselves. + auto const primaryKeyColumns = std::ranges::count_if(table->columns, [](auto const& c) { return c.isPrimaryKey; }); + CHECK(primaryKeyColumns == 2); +} + +namespace CompositeKeyGap +{ + +/// A record whose identity is a pair of columns, i.e. the C++ counterpart of +/// `PRIMARY KEY ("tenant_id", "entry_no")`. +/// +/// Both members are declared as primary keys, which is the only spelling available. It compiles, but +/// see the test below for what it actually means. +struct CkCompositeRecord +{ + static constexpr std::string_view TableName = "CkComposite"; + + Field tenantId {}; + Field entryNo {}; + Field>, SqlRealName { "label" }> label {}; +}; + +/// The referenced side of the composite foreign key declared in the DDL above. +struct CkParentRecord +{ + static constexpr std::string_view TableName = "CkParent"; + + Field partA {}; + Field partB {}; + Field>, SqlRealName { "caption" }> caption {}; +}; + +/// The referencing side: two ordinary column members, one per column. +struct CkChildRecord +{ + static constexpr std::string_view TableName = "CkChild"; + + Field id {}; + Field refA {}; + Field refB {}; +}; + +} // namespace CompositeKeyGap + +using CompositeKeyGap::CkCompositeRecord; + +TEST_CASE("Reflected identity covers every primary key member", "[CompositeKey][identity]") +{ + // Previously a gap: `RecordPrimaryKeyType` resolves through `RecordPrimaryKeyIndex`, a single member + // index, so anything built on it sees only the first key field. That helper is unchanged - it still + // names one field, and existing single-key callers are unaffected - but the composite-aware pair + // added beside it reports the whole key. + STATIC_CHECK(RecordPrimaryKeyCount == 2); + STATIC_CHECK(HasCompositePrimaryKey); + STATIC_CHECK(std::same_as, std::tuple>); + + // The single-key helper still collapses to the first field, deliberately: making it a tuple would + // change the shape of every existing caller, including CreateExplicit's return type. + STATIC_CHECK(std::same_as, int32_t>); +} + +TEST_CASE_METHOD(SqlTestFixture, "A composite primary key does identify a row", "[CompositeKey]") +{ + // Surprisingly, this half already works. QuerySingle() emits one `WHERE` per primary-key field + // (it enumerates members and adds a predicate for each `FieldType::IsPrimaryKey`) and binds one + // argument per placeholder, so passing the whole key selects exactly one row. + // + // So the composite-key gap is *not* in row lookup. It is in the two places that assume a single + // key field: `RecordPrimaryKeyType` / `GetPrimaryKeyField()` (which collapse to the first one), + // and `BelongsTo`, which can only name one referenced field. Pinned here so a composite-key + // implementation does not regress what already works. + auto stmt = SqlStatement {}; + (void) stmt.ExecuteDirect(R"(CREATE TABLE "CkComposite" ( + "tenant_id" INT NOT NULL, + "entry_no" INT NOT NULL, + "label" VARCHAR(40) NULL, + PRIMARY KEY ("tenant_id", "entry_no") + ))"); + (void) stmt.ExecuteDirect(R"(INSERT INTO "CkComposite" VALUES (1, 1, 'first'))"); + (void) stmt.ExecuteDirect(R"(INSERT INTO "CkComposite" VALUES (1, 2, 'second'))"); + (void) stmt.ExecuteDirect(R"(INSERT INTO "CkComposite" VALUES (2, 1, 'third'))"); + + auto dm = DataMapper {}; + + // Both key parts are bound, and the right row of the three comes back. + auto const second = dm.QuerySingle(1, 2); + REQUIRE(second.has_value()); + // NOLINTBEGIN(bugprone-unchecked-optional-access) - guarded above + CHECK(second->tenantId.Value() == 1); + CHECK(second->entryNo.Value() == 2); + REQUIRE(second->label.Value().has_value()); + CHECK(second->label.Value().value() == "second"); + + // ...and the row sharing entry_no but not tenant_id is distinguished too. + auto const third = dm.QuerySingle(2, 1); + REQUIRE(third.has_value()); + REQUIRE(third->label.Value().has_value()); + CHECK(third->label.Value().value() == "third"); + // NOLINTEND(bugprone-unchecked-optional-access) +} + +// ================================================================================================ +// Composite FOREIGN key +// ================================================================================================ + +TEST_CASE_METHOD(SqlTestFixture, "Composite foreign key is read back from the schema", "[CompositeKey][SqlSchema]") +{ + // As with the primary key: the reader is fine, the record mapping is not. + auto stmt = SqlStatement {}; + + (void) stmt.ExecuteDirect(R"(CREATE TABLE "CkParent" ( + "part_a" INT NOT NULL, + "part_b" INT NOT NULL, + "caption" VARCHAR(40) NULL, + PRIMARY KEY ("part_a", "part_b") + ))"); + (void) stmt.ExecuteDirect(R"(CREATE TABLE "CkChild" ( + "id" INT NOT NULL PRIMARY KEY, + "ref_a" INT NOT NULL, + "ref_b" INT NOT NULL, + CONSTRAINT "FK_CkChild_CkParent" + FOREIGN KEY ("ref_a", "ref_b") REFERENCES "CkParent" ("part_a", "part_b") + ))"); + + auto const tables = SqlSchema::ReadAllTables(stmt, stmt.Connection().DatabaseName(), /*schema=*/""); + auto const child = std::ranges::find_if(tables, [](SqlSchema::Table const& t) { return t.name == "CkChild"; }); + REQUIRE(child != tables.end()); + REQUIRE(child->foreignKeys.size() == 1); + + // Both column pairs are reported, and the ordering pairs ref_a->part_a, ref_b->part_b. + auto const& constraint = child->foreignKeys.front(); + REQUIRE(constraint.foreignKey.columns.size() == 2); + REQUIRE(constraint.primaryKey.columns.size() == 2); + CHECK(constraint.foreignKey.columns[0] == "ref_a"); + CHECK(constraint.foreignKey.columns[1] == "ref_b"); + CHECK(constraint.primaryKey.columns[0] == "part_a"); + CHECK(constraint.primaryKey.columns[1] == "part_b"); + CHECK(constraint.primaryKey.table.table == "CkParent"); +} + +TEST_CASE("A composite foreign key is expressed as a CompositeForeignKey", "[CompositeKey]") +{ + // Previously a gap: `BelongsTo<&Parent::field>` names one referenced field, so a two-column + // reference could not be written at all, and ddl2cpp reported such keys under "Foreign keys + // ignored". + // + // It is now spelled as a list of connections, each pairing one of this record's columns with the + // column it references - see `CompositeForeignKeyTests.cpp` for the behavioural coverage and + // `docs/composite-keys-design.md` for why the pairing lives in the type rather than in two + // positionally-matched lists. + // + // `BelongsTo` was deliberately left alone: it is inseparably a *column* (storage, one bound ODBC + // index) as well as a navigator, and widening it would have broken the one-member-one-column + // invariant that every projection and column-offset path depends on. + using Relation = + CompositeForeignKey, + Connection<&CompositeKeyGap::CkChildRecord::refB, &CompositeKeyGap::CkParentRecord::partB>>; + + STATIC_CHECK(Relation::Count == 2); + STATIC_CHECK(std::same_as); + + // It carries no column of its own: the two foreign key columns are ordinary Fields on the record. + STATIC_CHECK_FALSE(RecordColumnMember); +} + +// ================================================================================================ +// What works today, and is worth keeping working +// ================================================================================================ + +TEST_CASE_METHOD(SqlTestFixture, "A composite-key table still maps as a plain record", "[CompositeKey][SqlSchema]") +{ + // The fallback that makes the gap survivable: the columns are ordinary, so the data round-trips. + // Only identity and relations are affected. A fix must not regress this. + auto stmt = SqlStatement {}; + (void) stmt.ExecuteDirect(R"(CREATE TABLE "CkComposite" ( + "tenant_id" INT NOT NULL, + "entry_no" INT NOT NULL, + "label" VARCHAR(40) NULL, + PRIMARY KEY ("tenant_id", "entry_no") + ))"); + (void) stmt.ExecuteDirect(R"(INSERT INTO "CkComposite" VALUES (7, 3, 'kept'))"); + + auto dm = DataMapper {}; + auto const all = dm.Query().All(); + REQUIRE(all.size() == 1); + CHECK(all[0].tenantId.Value() == 7); + CHECK(all[0].entryNo.Value() == 3); + REQUIRE(all[0].label.Value().has_value()); + CHECK(all[0].label.Value().value() == "kept"); +} diff --git a/src/tests/CompositeKeyOrderingTests.cpp b/src/tests/CompositeKeyOrderingTests.cpp new file mode 100644 index 000000000..ce1523e83 --- /dev/null +++ b/src/tests/CompositeKeyOrderingTests.cpp @@ -0,0 +1,170 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// Predicate ordering for composite keys. +// +// `QuerySingle`/`Update`/`Delete` build their `WHERE` clause by enumerating the record's members in +// *declaration* order and appending one predicate per `PrimaryKey` field (`DataMapper.hpp`, the +// `EnumerateRecordMembers` loop). Arguments are then bound positionally, in the order given at the call +// site. So for a composite key the caller's argument order has to match the record's member order, and +// nothing in the type system enforces it. +// +// That matters for `CompositeForeignKey`, whose loader has to supply those arguments from a list of +// `Connection`s. If the connections are written in a different order than the parent declares its key +// fields, a positional bind sends each value to the wrong predicate. When the key columns share a type +// - which is the common case, e.g. two `INT` parts - the result is not an error but a wrong row. +// +// These tests establish the ground truth the loader is designed against, using only what exists today. +// They are the specification for the ordering decision recorded in `docs/composite-keys-design.md`. + +#include "Utils.hpp" + +#include + +#include + +#include + +using namespace Lightweight; + +namespace CompositeKeyOrdering +{ + +/// A record whose two key columns are deliberately *both* `int32_t`, so a transposed argument pair +/// still compiles and still binds - the failure is a wrong row, not a type error. +struct OrderedKey +{ + static constexpr std::string_view TableName = "CkOrdered"; + + Field keyFirst {}; + Field keySecond {}; + Field>, SqlRealName { "payload" }> payload {}; +}; + +/// The same table, mapped with the two key members declared in the *opposite* order. Nothing about the +/// database changes; only the C++ declaration order does. Used to prove that declaration order - not +/// column order in the table - is what drives predicate order. +struct ReversedKey +{ + static constexpr std::string_view TableName = "CkOrdered"; + + Field keySecond {}; + Field keyFirst {}; + Field>, SqlRealName { "payload" }> payload {}; +}; + +/// Creates the shared fixture table and three rows that make every ordering mistake observable. +/// +/// The rows are chosen so that (1, 2) and (2, 1) both exist and carry different payloads: a +/// transposed bind therefore returns a row rather than nothing, which is exactly the silent failure +/// being guarded against. +void CreateOrderingFixture(SqlStatement& stmt) +{ + (void) stmt.ExecuteDirect(R"(CREATE TABLE "CkOrdered" ( + "key_first" INT NOT NULL, + "key_second" INT NOT NULL, + "payload" VARCHAR(20) NULL, + PRIMARY KEY ("key_first", "key_second") + ))"); + (void) stmt.ExecuteDirect(R"(INSERT INTO "CkOrdered" VALUES (1, 2, 'one-two'))"); + (void) stmt.ExecuteDirect(R"(INSERT INTO "CkOrdered" VALUES (2, 1, 'two-one'))"); + (void) stmt.ExecuteDirect(R"(INSERT INTO "CkOrdered" VALUES (3, 3, 'three-three'))"); +} + +} // namespace CompositeKeyOrdering + +using CompositeKeyOrdering::CreateOrderingFixture; +using CompositeKeyOrdering::OrderedKey; +using CompositeKeyOrdering::ReversedKey; + +TEST_CASE_METHOD(SqlTestFixture, "Composite key arguments bind in member declaration order", "[CompositeKey][ordering]") +{ + // The contract the loader must honour: argument N goes to the Nth `PrimaryKey` member, counting in + // declaration order. + auto stmt = SqlStatement {}; + CreateOrderingFixture(stmt); + + auto dm = DataMapper {}; + + // keyFirst is declared first, so the first argument is matched against it. + auto const oneTwo = dm.QuerySingle(1, 2); + REQUIRE(oneTwo.has_value()); + // NOLINTBEGIN(bugprone-unchecked-optional-access) - guarded above + CHECK(oneTwo->keyFirst.Value() == 1); + CHECK(oneTwo->keySecond.Value() == 2); + REQUIRE(oneTwo->payload.Value().has_value()); + CHECK(oneTwo->payload.Value().value() == "one-two"); + // NOLINTEND(bugprone-unchecked-optional-access) +} + +TEST_CASE_METHOD(SqlTestFixture, + "Transposed composite key arguments silently select a different row", + "[CompositeKey][ordering]") +{ + // The failure mode that makes the ordering question a correctness issue rather than a style one. + // Swapping the two arguments is well-formed, binds cleanly, and returns the *other* row. + // + // This is why `CompositeForeignKey`'s loader must not rely on the caller (or the generator) + // happening to list its connections in the parent's key order. + auto stmt = SqlStatement {}; + CreateOrderingFixture(stmt); + + auto dm = DataMapper {}; + + auto const transposed = dm.QuerySingle(2, 1); + REQUIRE(transposed.has_value()); + // NOLINTBEGIN(bugprone-unchecked-optional-access) - guarded above + // Not "no such row" - a real, wrong row. + REQUIRE(transposed->payload.Value().has_value()); + CHECK(transposed->payload.Value().value() == "two-one"); + CHECK(transposed->keyFirst.Value() == 2); + // NOLINTEND(bugprone-unchecked-optional-access) +} + +TEST_CASE_METHOD(SqlTestFixture, + "Predicate order follows C++ member order, not table column order", + "[CompositeKey][ordering]") +{ + // `ReversedKey` maps the same table with its key members declared the other way round. If + // predicate order were driven by the table, both records would need the same argument order; it is + // driven by the C++ declaration, so they need opposite orders to reach the same row. + // + // Consequence for the design: the loader cannot derive argument order from the schema. It has to + // derive it from the parent record's member order - which is what the `Into` member pointers in + // each `Connection` give access to. + auto stmt = SqlStatement {}; + CreateOrderingFixture(stmt); + + auto dm = DataMapper {}; + + // Reaching the row (key_first=1, key_second=2) through ReversedKey means passing key_second first. + auto const viaReversed = dm.QuerySingle(2, 1); + REQUIRE(viaReversed.has_value()); + // NOLINTBEGIN(bugprone-unchecked-optional-access) - guarded above + CHECK(viaReversed->keyFirst.Value() == 1); + CHECK(viaReversed->keySecond.Value() == 2); + REQUIRE(viaReversed->payload.Value().has_value()); + CHECK(viaReversed->payload.Value().value() == "one-two"); + // NOLINTEND(bugprone-unchecked-optional-access) + + // ...and the same values in the other order reach the other row, confirming the asymmetry. + auto const viaReversedSwapped = dm.QuerySingle(1, 2); + REQUIRE(viaReversedSwapped.has_value()); + REQUIRE(viaReversedSwapped->payload.Value().has_value()); // NOLINT(bugprone-unchecked-optional-access) + CHECK(viaReversedSwapped->payload.Value().value() == "two-one"); // NOLINT(bugprone-unchecked-optional-access) +} + +TEST_CASE_METHOD(SqlTestFixture, "A composite key with equal parts is order-insensitive", "[CompositeKey][ordering]") +{ + // The degenerate case worth pinning: when both key values are equal, ordering cannot be observed. + // Any test that only used such a row would pass regardless of a transposition bug - which is why + // the fixture above deliberately contains (1, 2) and (2, 1). + auto stmt = SqlStatement {}; + CreateOrderingFixture(stmt); + + auto dm = DataMapper {}; + + auto const threeThree = dm.QuerySingle(3, 3); + REQUIRE(threeThree.has_value()); + REQUIRE(threeThree->payload.Value().has_value()); // NOLINT(bugprone-unchecked-optional-access) + CHECK(threeThree->payload.Value().value() == "three-three"); // NOLINT(bugprone-unchecked-optional-access) +} From 86086f4fcfed341ec826232affb933d5895ea359 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sun, 2 Aug 2026 22:33:04 +0300 Subject: [PATCH 3/5] fix(DataMapper): address the composite key review findings Ten confirmed findings from the high-effort review of acbc8c03. Two of them contradicted claims made in that commit's own message and comments; those are corrected rather than restated. Correctness: - AssignAt() permuted values by assigning through a runtime-matched tuple index, which forces *every* slot's assignment to be well-formed - so any composite key whose columns did not all share one value type failed to compile. Reproduced in isolation with a mixed int/string key. OrderedValuesOf now *builds* the ordered tuple by slot instead: the connection occupying each key position is resolved at compile time from the Into member indices and its value read directly, so heterogeneous keys work and AssignAt is gone. The comment claiming the static_asserts guaranteed equal types was wrong - they only compare types within each Connection, never across them. - The loader snapshotted the key values at ConfigureRelationAutoLoading time, so repointing a foreign key afterwards kept resolving to the previous parent. It now reads them through the connections at load time, which is what the header already claimed ("one copy of each value, in the Field that owns the column"). - Neither Inspect() nor LoadRelations() knew about the new relation. Inspect() reached std::format on it and would not compile for any record holding one; LoadRelations() had no branch, so an eagerly-loaded record's relation stayed unloaded and then threw. Both fixed, LoadRelations in both the reflection and non-reflection halves, via a new LoadCompositeForeignKey(). - The C++26 reflection build was broken: only MemberClassType was guarded, while the member pointers were still dereferenced with `.*`, invalid on the std::meta::info that branch supplies. Member access now goes through Connection::FieldOf(), which is the single place the two modes differ, and field types come from the pointer type rather than a declval of the owning record. Validation, all previously silent: - Two connections naming the same referenced member collapsed the permutation, leaving a key slot default-constructed against a real predicate. - A partial key (fewer connections than the referenced key has columns) compiled and threw an argument-count mismatch at first navigation. - A connection pairing a member with itself passed every check. The last two are deferred to first use rather than asserted in the class body: the declaring record is incomplete while the relation is instantiated as one of its members, so reflecting over it there is ill-formed - which MSVC diagnosed and clang-cl did not. Multi-column AutoAssign: GenerateAutoAssignPrimaryKey now rejects more than one auto-assigned key member, because it yields a single value that SetId() writes into every key member. Scoped to the value types auto-assignment actually generates, so the pre-existing MultiPkRecord test - two AutoAssign *string* keys, where nothing is generated - keeps working. Every fixture and both doc examples moved off multi-column AutoAssign accordingly. Docs: the design doc no longer says "Status: proposal. Nothing implemented yet." in the commit that ships the feature, and its rejection table, ordering section and AutoAssign guidance now match the implementation. Also found while fixing: clang-cl 22.1.3 miscompiles a record transitively holding a std::function returning a record that references it back - an impossible stack frame and a prologue fault, eight frames deep with no runtime recursion. Same defect already documented for a self-referential BelongsTo in RelationShapeTests. The identical code compiles and every case passes under MSVC, so the tests are guarded on the toolchain with a [!shouldfail] placeholder standing in. Verified: MSVC cl-release : 1337 passed, 1 skipped, 0 failed (all composite coverage ACTIVE) clang-cl release : 1321 passed, 1 skipped, 2 failed as expected (both guarded) Composite tests under MSVC: 124 assertions in 22 cases, all green. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01TVec3YeQg2FZnToiPQwcQH --- docs/composite-keys-design.md | 50 +++-- .../DataMapper/CompositeForeignKey.hpp | 200 ++++++++++++++---- src/Lightweight/DataMapper/DataMapper.hpp | 75 ++++++- src/Lightweight/DataMapper/Record.hpp | 29 +++ src/tests/CompositeForeignKeyTests.cpp | 55 ++++- src/tests/CompositeKeyGapTests.cpp | 11 +- src/tests/CompositeKeyOrderingTests.cpp | 8 +- 7 files changed, 347 insertions(+), 81 deletions(-) diff --git a/docs/composite-keys-design.md b/docs/composite-keys-design.md index 18aa4d713..949bfe014 100644 --- a/docs/composite-keys-design.md +++ b/docs/composite-keys-design.md @@ -1,7 +1,14 @@ # Composite key support — design -Status: **proposal.** Nothing implemented yet. The core mechanism below has been prototyped -standalone and compiles; the integration has not been written. +Status: **implemented.** `CompositeForeignKey` / `Connection` ship in +`src/Lightweight/DataMapper/CompositeForeignKey.hpp`, with the additive identity helpers in +`Record.hpp` and loading wired into both `ConfigureRelationAutoLoading` (lazy) and `LoadRelations` +(eager). Coverage: `src/tests/CompositeForeignKeyTests.cpp`, +`src/tests/CompositeKeyOrderingTests.cpp`, `src/tests/CompositeKeyGapTests.cpp`. + +Still open, and tracked in "Deferred" at the end: `ddl2cpp` generation, the inverse (`HasMany` over a +composite relation), and multi-column `AutoAssign` semantics — the last of which is a **live +limitation**, see the warning under "Declaring the referenced side". Two givens shape everything: @@ -23,9 +30,9 @@ struct CkParent { static constexpr std::string_view TableName = "CkParent"; - // One member per column. Both marked PrimaryKey - already legal today. - Field partA; - Field partB; + // One member per column, both primary keys. NOT AutoAssign - see the warning below. + Field partA; + Field partB; Field>, SqlRealName{"caption"}> caption; }; @@ -55,6 +62,13 @@ CompositeForeignKey, Connection<&Leaf::c, &Hub::k3>> hub; ``` +> **Do not mark several key members `PrimaryKey::AutoAssign`.** Auto-assignment produces a single +> value which `SetId()` then writes into *every* primary key member, so a composite key would receive +> the same value in all of its columns. This is rejected at compile time by a `static_assert` in +> `GenerateAutoAssignPrimaryKey`, for the value types auto-assignment actually generates (GUIDs and +> incrementable ones). Declare composite key members without `AutoAssign` and set their values +> yourself before calling `Create()`. + ## Why the pairing matters Earlier drafts spelled the two sides as separate lists (`RecordMemberList<&Child::refA, &Child::refB>` @@ -75,14 +89,19 @@ computes rather than restates: - `Child` — the owning record, from the `From` pointers. - the foreign key values, read through the `From` pointers. -And it `static_assert`s, all three verified against a standalone prototype: +And it `static_assert`s the following. The first four are checked in the class body; the last two are +deferred to first use, because the declaring record is still incomplete while the relation is +instantiated as one of its members and reflecting over it is not yet possible there: | Error | Caught | |---|---| | Connections pointing at *different* parent records | ✅ `"all connections must point at the same record"` | | Connections starting from different child records | ✅ same mechanism | | A pair whose field types differ (e.g. `int` wired to `long long`) | ✅ `"pairwise field types must match"` | -| Right-hand member not marked `IsPrimaryKey` | ✅ (checkable, not yet prototyped) | +| Right-hand member not marked `IsPrimaryKey` | ✅ | +| Two connections naming the *same* referenced member | ✅ `"reference the same member of the referenced record"` | +| Connections not covering the referenced record's whole key | ✅ (deferred to first use) | +| A connection pairing a member with itself | ✅ (deferred to first use) | | Transposing two columns *of the same type* | ❌ — inexpressible to catch; but see below | The last row is the residual risk, and it is much smaller than with positional lists: a same-typed @@ -107,10 +126,12 @@ Only the loaded target, as `HasOneThrough` does (`std::shared_ptr`). The through the `From` pointers on demand: ```cpp -// prototyped and working -static auto ValuesOf(Child const& c) { return std::tuple { (c.*Cs::From).Value()... }; } +static auto ValuesOf(Child const& c) { return std::tuple { Cs::FieldOf(c).Value()... }; } ``` +`FieldOf` wraps the member access so the C++26 reflection and non-reflection modes differ in exactly +one place - the former splices the reflection, the latter dereferences a pointer-to-member. + One copy of each value, in the `Field` that owns the column. Nothing to keep in sync. ### Loading reuses machinery that already takes N values @@ -138,11 +159,12 @@ The composite loader only has to pass every value instead of one — `std::apply - A transposed argument pair is well-formed, binds cleanly, and returns **a different row** — not an error. With same-typed key parts (the common case) nothing detects it. -So the loader must not depend on connections being written in the parent's key order. It sorts the -values itself: for each `Connection`, the `Into` pointer identifies the parent member, from which the -member *index* is recovered; sorting the values by that index yields exactly the order -`QuerySingle` will bind them in. Connections may then be listed in any order, and a transposition -becomes impossible rather than merely detectable. +So the loader must not depend on connections being written in the parent's key order. `OrderedValuesOf` +*builds* the ordered tuple by slot: for each key position, the connection occupying it is found at +compile time from the `Into` member indices, and its value is read directly. Building rather than +assigning matters - permuting by assignment through a runtime-matched index would require every slot's +assignment to be well-formed, which fails as soon as two key columns differ in type. Connections may +therefore be listed in any order, and heterogeneous keys work. ### Navigation diff --git a/src/Lightweight/DataMapper/CompositeForeignKey.hpp b/src/Lightweight/DataMapper/CompositeForeignKey.hpp index b4478f93a..d6016dfe0 100644 --- a/src/Lightweight/DataMapper/CompositeForeignKey.hpp +++ b/src/Lightweight/DataMapper/CompositeForeignKey.hpp @@ -19,6 +19,21 @@ namespace Lightweight { +namespace detail +{ + /// Extracts the member type from a pointer-to-member, without requiring the owning class to be + /// complete. `decltype(std::declval().*Ptr)` would require completeness, which a + /// relation declared *inside* its own record cannot offer. + template + struct MemberPointeeType; + + template + struct MemberPointeeType + { + using type = Member; + }; +} // namespace detail + /// @brief One column pair of a composite foreign key: "this record's column references that one". /// /// Both endpoints are pointers-to-member, so the pairing is part of the type. That is the whole point: @@ -59,11 +74,48 @@ struct Connection using IntoRecord = MemberClassType; #endif +#if defined(LIGHTWEIGHT_CXX26_REFLECTION) + /// The field type on this record's side, e.g. `Field`. + using FromField = std::remove_cvref_t; + + /// The field type on the referenced record's side. + using IntoField = std::remove_cvref_t; +#else /// The field type on this record's side, e.g. `Field`. - using FromField = std::remove_cvref_t().*FromPtr)>; + /// + /// Taken from the pointer-to-member's own type rather than from a `declval` of the owning record: + /// the declaring record is incomplete while this relation is instantiated as one of its members. + using FromField = std::remove_cvref_t::type>; /// The field type on the referenced record's side. - using IntoField = std::remove_cvref_t().*IntoPtr)>; + using IntoField = std::remove_cvref_t::type>; +#endif + + /// Reads this connection's field out of @p record. + /// + /// Wraps the member access so the two reflection modes differ in exactly one place: the + /// non-reflection branch uses a pointer-to-member, while the C++26 branch splices the reflection. + /// + /// @param record The record holding the foreign key. + /// @return A reference to the field. + template + [[nodiscard]] static auto const& FieldOf(RecordT const& record) noexcept + { +#if defined(LIGHTWEIGHT_CXX26_REFLECTION) + return record.[:FromPtr:]; +#else + return record.*FromPtr; +#endif + } + + /// Member index of this record's foreign key member within its own record. + /// + /// A function rather than a variable: the declaring record is still incomplete while this relation + /// is instantiated as one of its members, so reflecting over it has to wait until first use. + [[nodiscard]] static consteval std::size_t FromMemberIndex() noexcept + { + return MemberIndexOf; + } /// Member index of the referenced member within the referenced record. /// @@ -115,8 +167,10 @@ concept ConnectionType = detail::IsConnectionType>::value /// @code /// struct Parent /// { -/// Field partA; -/// Field partB; +/// // Composite key members must not be PrimaryKey::AutoAssign: auto-assignment yields one value +/// // that would be written into every key member. See GenerateAutoAssignPrimaryKey. +/// Field partA; +/// Field partB; /// }; /// struct Child /// { @@ -163,9 +217,78 @@ class CompositeForeignKey "A composite foreign key must reference primary key columns. " "Check the PrimaryKey marker on the referenced record's members."); + // A foreign key pointing at its own record through the same member is degenerate: it would read a + // value out of a record and then look that same record up by it. Each endpoint on its own passes + // every check above, so the pairing has to be rejected explicitly. + + // The permutation below ranks each connection by how many name an earlier referenced member, which + // is only a total ordering while those indices are distinct. Two connections naming the same + // referenced member would share a rank, leaving one key slot unwritten and binding a + // default-constructed value against a real predicate - a wrong-row lookup with no diagnostic. + static_assert( + []() consteval { + auto const indices = std::array { Connections::IntoMemberIndex... }; + for (auto outer = std::size_t { 0 }; outer != indices.size(); ++outer) + for (auto inner = outer + 1; inner != indices.size(); ++inner) + if (indices[outer] == indices[inner]) + return false; + return true; + }(), + "Two Connections of a composite foreign key reference the same member of the referenced " + "record. Each column of the key must be connected exactly once."); + + // NB: the "connections cover the whole referenced key" check cannot live here. This class is + // instantiated while the *declaring* record is still incomplete - the relation is one of its + // members - and RecordPrimaryKeyCount reflects over the referenced record, which in a mutually + // referencing pair is equally incomplete at that point. It is therefore checked in + // AssertCoversReferencedKey() below, which runs from the value accessors, i.e. at first use, when + // both records are complete. + /// The tuple of foreign key values, in the order the connections are declared. using ValueType = std::tuple; + private: + /// Referenced member index of each connection, in declaration order. + static constexpr auto IntoIndices = std::array { Connections::IntoMemberIndex... }; + + /// Index of the connection whose referenced member comes @p Slot-th in the referenced record. + /// + /// Computed by counting how many connections name an earlier member, which is a total ranking + /// because the referenced indices are asserted pairwise distinct above. + template + static constexpr std::size_t ConnectionForSlot = []() consteval { + for (auto candidate = std::size_t { 0 }; candidate != Count; ++candidate) + { + auto rank = std::size_t { 0 }; + for (auto const other: IntoIndices) + if (other < IntoIndices[candidate]) + ++rank; + if (rank == Slot) + return candidate; + } + return Count; // unreachable: the ranking is a bijection onto [0, Count) + }(); + + /// The connection occupying @p Slot of the referenced record's key order. + template + using ConnectionAtSlot = std::tuple_element_t, std::tuple>; + + /// Reads the value belonging in @p Slot straight out of the record's own field. + template + [[nodiscard]] static decltype(auto) ValueAtSlot(Child const& record) + { + return ConnectionAtSlot::FieldOf(record).Value(); + } + + public: + /// The tuple of foreign key values, ordered to match the referenced record's key members. + /// + /// Differs from @ref ValueType whenever the connections are not written in the referenced record's + /// member order, and differs in *type* too when the key columns are heterogeneous. + using OrderedValueType = decltype([](std::index_sequence) { + return std::tuple::FromField::ValueType...> {}; + }(std::index_sequence_for {})); + /// Reads this record's foreign key values, in the order the connections are declared. /// /// The values are not stored on the relation: there is exactly one copy of each, in the `Field` @@ -175,7 +298,30 @@ class CompositeForeignKey /// @return The values, in declaration order of the connections. [[nodiscard]] static ValueType ValuesOf(Child const& record) { - return ValueType { (record.*Connections::From).Value()... }; + AssertCoversReferencedKey(); + return ValueType { Connections::FieldOf(record).Value()... }; + } + + /// Checks that the connections cover the referenced record's whole primary key. + /// + /// Deferred to first use rather than asserted in the class body: at class-instantiation time the + /// referenced record can still be incomplete, so reflecting over its members is not yet possible. + /// A partial key would otherwise surface only as an argument-count mismatch thrown from the first + /// navigation, far from the declaration that caused it. + static constexpr void AssertCoversReferencedKey() noexcept + { + static_assert(Count == RecordPrimaryKeyCount, + "A composite foreign key must connect every primary key column of the referenced " + "record. Connecting only some of them cannot identify a row."); + + // Also deferred, and for the same reason: recovering a member index reflects over the owning + // record. Only the same-record case can be degenerate - across records the two endpoints are + // different entities by construction. + static_assert(((!std::same_as + || Connections::FromMemberIndex() != Connections::IntoMemberIndex) + && ...), + "A Connection must join two different members. Pairing a member with itself reads " + "a value out of a record only to look the same record up by it."); } /// Reads this record's foreign key values, permuted into the referenced record's member order. @@ -188,30 +334,17 @@ class CompositeForeignKey /// /// @param record The record holding the foreign key. /// @return The values, ordered to match the referenced record's primary key members. - [[nodiscard]] static ValueType OrderedValuesOf(Child const& record) + [[nodiscard]] static OrderedValueType OrderedValuesOf(Child const& record) { - auto values = ValuesOf(record); - auto ordered = values; - - // Rank each connection by its referenced member index; the rank is the slot its value belongs - // in. Done over indices rather than by sorting a runtime container so the whole permutation is - // fixed at compile time for the common (already-ordered) case. - constexpr auto intoIndices = std::array { Connections::IntoMemberIndex... }; - - [&](std::index_sequence) { - ( - [&] { - // How many connections reference an earlier member than this one does. - auto rank = std::size_t { 0 }; - for (auto const other: intoIndices) - if (other < intoIndices[I]) - ++rank; - AssignAt(ordered, rank, std::get(values)); - }(), - ...); + // The permutation is entirely a compile-time property of the connection list, so the ordered + // tuple is *built* by index rather than default-constructed and then assigned into. That keeps + // heterogeneous keys working - assigning through a runtime-matched index would require every + // slot's assignment to be well-formed, which fails as soon as two key columns differ in type - + // and it needs no default-constructible value type. + AssertCoversReferencedKey(); + return [&](std::index_sequence) { + return OrderedValueType { ValueAtSlot(record)... }; }(std::index_sequence_for {}); - - return ordered; } /// @return The referenced record, loading it on first access. @@ -263,19 +396,6 @@ class CompositeForeignKey } private: - /// Assigns @p value into the tuple slot at the runtime index @p slot. - /// - /// A tuple cannot be indexed at runtime, so the index is matched against the compile-time pack. - /// Every element of the permutation has the same type here - the static_asserts above guarantee - /// pairwise type equality - so only the matching slot is written. - template - static constexpr void AssignAt(Tuple& tuple, std::size_t slot, Value const& value) - { - [&](std::index_sequence) { - (((I == slot) ? (void) (std::get(tuple) = value) : (void) 0), ...); - }(std::make_index_sequence> {}); - } - void RequireLoaded() const { if (_record) diff --git a/src/Lightweight/DataMapper/DataMapper.hpp b/src/Lightweight/DataMapper/DataMapper.hpp index 550d42047..488346642 100644 --- a/src/Lightweight/DataMapper/DataMapper.hpp +++ b/src/Lightweight/DataMapper/DataMapper.hpp @@ -622,6 +622,13 @@ class DataMapper template std::optional LoadBelongsTo(FieldType::ValueType value); + /// Eagerly loads the record referenced by a composite foreign key. + /// + /// @param record The record holding the foreign key. + /// @param field The relation to fill. + template + void LoadCompositeForeignKey(Record const& record, FieldType& field); + template void LoadHasMany(Record& record, HasMany& field); @@ -1619,7 +1626,8 @@ std::string DataMapper::Inspect(Record const& record) str += std::format("{} {} := {}", Reflection::TypeNameOf, name, value.InspectValue()); } } - else if constexpr (!IsHasMany && !IsHasManyThrough && !IsHasOneThrough && !IsBelongsTo) + else if constexpr (!IsHasMany && !IsHasManyThrough && !IsHasOneThrough && !IsBelongsTo + && !IsCompositeForeignKey) str += std::format("{} {} := {}", Reflection::TypeNameOf, name, value); }); return "{\n" + std::move(str) + "\n}"; @@ -1671,6 +1679,17 @@ void DataMapper::CreateTables() template std::optional> DataMapper::GenerateAutoAssignPrimaryKey(Record const& record) { + // Auto-assignment produces exactly one value, and SetId() writes it into *every* primary key + // member - so a record with several auto-assigned key members would silently receive the same value + // in all of them. A composite key must therefore be supplied explicitly rather than generated. + // Rejected here rather than in SetId(), which legitimately serves multi-key records whose values + // the caller provides. + static_assert(detail::AutoAssignPrimaryKeyFieldCount <= 1, + "A record may declare at most one auto-assigned primary key member. Auto-assignment yields a " + "single value that would be written into every key member, so a composite key cannot be " + "generated - declare the key members without PrimaryKey::AutoAssign and set their values " + "yourself before calling Create()."); + std::optional> result; EnumerateRecordMembers( record, [this, &result](PrimaryKeyType const& primaryKeyField) { @@ -2465,6 +2484,34 @@ inline LIGHTWEIGHT_FORCE_INLINE void CallOnBelongsTo(Callable const& callable) }); } +template +void DataMapper::LoadCompositeForeignKey(Record const& record, FieldType& field) +{ + using ReferencedRecord = typename FieldType::ReferencedRecord; + + ZoneScopedN("DataMapper::LoadCompositeForeignKey"); + ZoneTextObject(RecordTableName); + + // OrderedValuesOf() rather than ValuesOf(): QuerySingle emits one WHERE predicate per primary key + // member of the referenced record, in that record's member declaration order, and binds its + // arguments positionally - so the values have to be permuted into that order first. See + // CompositeKeyOrderingTests.cpp. + auto loaded = std::apply([this](auto const&... key) { return QuerySingle(key...); }, + FieldType::OrderedValuesOf(record)); + + // A missing target row leaves the relation unloaded rather than throwing here: eagerly loading a + // dangling foreign key is a data-integrity problem to surface at the accessor, which is where the + // lazy path reports it too. + if (!loaded) + { + SqlLogger::GetLogger().OnWarning( + std::format("Loading composite foreign key failed for {}", RecordTableName)); + return; + } + + field.EmplaceRecord(std::make_shared(std::move(*loaded))); +} + template std::optional DataMapper::LoadBelongsTo(FieldType::ValueType value) { @@ -2739,6 +2786,10 @@ void DataMapper::LoadRelations(Record& record) auto& field = record.[:el:]; field.AdoptFetchedRecord(LoadBelongsTo(field.Value())); } + else if constexpr (IsCompositeForeignKey) + { + LoadCompositeForeignKey(record, record.[:el:]); + } else if constexpr (IsHasMany) { LoadHasMany(record, record.[:el:]); @@ -2758,6 +2809,10 @@ void DataMapper::LoadRelations(Record& record) { field.AdoptFetchedRecord(LoadBelongsTo(field.Value())); } + else if constexpr (IsCompositeForeignKey) + { + LoadCompositeForeignKey(record, field); + } else if constexpr (IsHasMany) { LoadHasMany(record, field); @@ -2875,21 +2930,27 @@ void DataMapper::ConfigureRelationAutoLoading(Record& record) { using ReferencedRecord = typename FieldType::ReferencedRecord; - // The key values are read out of the record's own Field members and captured by value, so - // the loader does not depend on the record still being alive at load time. + // The key is read out of the record's own Field members *at load time*, through the + // connections' member pointers, rather than snapshotted here. Snapshotting would make the + // relation resolve to whichever parent the key named when the record was configured, so + // repointing the foreign key afterwards would silently keep returning the old parent. The + // relation deliberately owns no copy of the key - the Field that owns each column is the + // single source of truth. + // + // Capturing the record by pointer is safe because it is the mapper's own stored record: the + // same lifetime the HasMany/HasOneThrough loaders already rely on. Unlike those, no value is + // copied out, so a moved-from record would be observed rather than a stale duplicate. // // OrderedValuesOf() - not ValuesOf() - because QuerySingle emits one WHERE predicate per // primary key member in the *referenced record's* member declaration order and binds its // arguments positionally. Passing them in connection-declaration order would bind each // value to the wrong predicate whenever the two orders differ, which with same-typed key // columns fetches a wrong row rather than failing. See CompositeKeyOrderingTests.cpp. - auto const keyValues = FieldType::OrderedValuesOf(record); - field.SetAutoLoader(typename FieldType::Loader { - .loadReference = [keyValues]() -> std::shared_ptr { + .loadReference = [owner = &record]() -> std::shared_ptr { DataMapper& dm = DataMapper::AcquireThreadLocal(); auto loaded = std::apply([&dm](auto const&... key) { return dm.QuerySingle(key...); }, - keyValues); + FieldType::OrderedValuesOf(*owner)); if (!loaded) return {}; return std::make_shared(std::move(*loaded)); diff --git a/src/Lightweight/DataMapper/Record.hpp b/src/Lightweight/DataMapper/Record.hpp index 3403b8ce2..acfd3ade7 100644 --- a/src/Lightweight/DataMapper/Record.hpp +++ b/src/Lightweight/DataMapper/Record.hpp @@ -387,6 +387,35 @@ namespace detail }; } // namespace detail +namespace detail +{ + /// Number of members of @p Record declared as `PrimaryKey::AutoAssign`. + /// + /// Auto-assignment yields a single value that is then written into every primary key member, so more + /// than one such member cannot be honoured - see the static_assert in + /// `DataMapper::GenerateAutoAssignPrimaryKey`. + /// Whether `GenerateAutoAssignPrimaryKey` actually produces a value for @p FieldType. + /// + /// `PrimaryKey::AutoAssign` only generates for a GUID or an incrementable value; on any other type + /// (a string key, say) it silently generates nothing and the caller supplies the value. Only the + /// generating case can collide across several key members, so only it is counted. + template + concept IncrementableKeyValue = requires(ValueType value) { value + 1; }; + + template + concept GeneratesAutoAssignedKey = IsField && IsAutoAssignPrimaryKeyField::value + && IncrementableKeyValue; + + template + constexpr std::size_t AutoAssignPrimaryKeyFieldCount = + FoldRecordMembers(std::size_t { 0 }, [](std::size_t const accum) { + if constexpr (GeneratesAutoAssignedKey) + return accum + 1; + else + return accum; + }); +} // namespace detail + /// @brief The tuple of a record's primary key value types, in member declaration order. /// /// Unlike @ref RecordPrimaryKeyType, which names a single field's type, this covers composite keys: diff --git a/src/tests/CompositeForeignKeyTests.cpp b/src/tests/CompositeForeignKeyTests.cpp index cd6df5c25..627f018b1 100644 --- a/src/tests/CompositeForeignKeyTests.cpp +++ b/src/tests/CompositeForeignKeyTests.cpp @@ -25,6 +25,18 @@ using namespace Lightweight; +// clang-cl 22.1.3 miscompiles a record that transitively contains a std::function returning a record +// which references it back - it computes a ~5e14-byte stack frame for any function constructing one and +// faults in the prologue. That is the same defect documented at length in +// src/tests/DataMapper/RelationShapeTests.cpp for a self-referential BelongsTo: eight frames deep, one +// consuming most of the stack, no runtime recursion. The identical code compiles and all 22 cases pass +// under MSVC cl, so the shape is sound and the guard is keyed to the toolchain. +#if defined(__clang__) && defined(_MSC_VER) + #define LIGHTWEIGHT_COMPOSITE_FK_MISCOMPILED 1 +#endif + +#if !defined(LIGHTWEIGHT_COMPOSITE_FK_MISCOMPILED) + namespace CompositeFk { @@ -33,8 +45,8 @@ struct Parent { static constexpr std::string_view TableName = "CfkParent"; - Field partA {}; - Field partB {}; + Field partA {}; + Field partB {}; Field>, SqlRealName { "caption" }> caption {}; }; @@ -71,9 +83,9 @@ struct WideParent { static constexpr std::string_view TableName = "CfkWideParent"; - Field k1 {}; - Field k2 {}; - Field k3 {}; + Field k1 {}; + Field k2 {}; + Field k3 {}; Field>, SqlRealName { "note" }> note {}; }; @@ -264,12 +276,14 @@ TEST_CASE_METHOD(SqlTestFixture, "CompositeForeignKey loads its referenced recor CHECK(child->refA.Value() == 1); CHECK(child->refB.Value() == 2); - // The relation is lazy: nothing is fetched until the record is touched. + // QuerySingle(primaryKeys...) does not install auto-loaders - only the query-builder overloads do - + // so the relation is filled explicitly here. That is the eager path LoadRelations() serves. CHECK_FALSE(child->parent.IsLoaded()); + dm.LoadRelations(*child); + REQUIRE(child->parent.IsLoaded()); - // Touching it resolves to (part_a=1, part_b=2), not to the transposed row... + // It resolves to (part_a=1, part_b=2), not to the transposed row. CHECK(child->parent.Record().partA.Value() == 1); - CHECK(child->parent.IsLoaded()); // ...and it is cached afterwards CHECK(child->parent.Record().partB.Value() == 2); REQUIRE(child->parent.Record().caption.Value().has_value()); CHECK(child->parent.Record().caption.Value().value() == "one-two"); @@ -303,6 +317,7 @@ TEST_CASE_METHOD(SqlTestFixture, auto child = dm.QuerySingle(100); REQUIRE(child.has_value()); // NOLINTBEGIN(bugprone-unchecked-optional-access) - guarded above + dm.LoadRelations(*child); REQUIRE(child->parent.Record().caption.Value().has_value()); CHECK(child->parent.Record().caption.Value().value() == "one-two"); // not 'two-one' // NOLINTEND(bugprone-unchecked-optional-access) @@ -319,16 +334,19 @@ TEST_CASE("RecordPrimaryKeyTuple covers every primary key member", "[CompositeFo // behaviour. STATIC_CHECK(RecordPrimaryKeyCount == 2); STATIC_CHECK(HasCompositePrimaryKey); - STATIC_CHECK(std::same_as, std::tuple>); + using ParentKeyTuple = std::tuple; + STATIC_CHECK(std::same_as, ParentKeyTuple>); STATIC_CHECK(RecordPrimaryKeyCount == 3); - STATIC_CHECK(std::same_as, std::tuple>); + using WideKeyTuple = std::tuple; + STATIC_CHECK(std::same_as, WideKeyTuple>); // A single-key record yields a one-element tuple and is not composite, so callers can treat both // uniformly without special-casing. STATIC_CHECK(RecordPrimaryKeyCount == 1); STATIC_CHECK_FALSE(HasCompositePrimaryKey); - STATIC_CHECK(std::same_as, std::tuple>); + using ChildKeyTuple = std::tuple; + STATIC_CHECK(std::same_as, ChildKeyTuple>); // ...and the existing single-key helper is untouched. STATIC_CHECK(std::same_as, int32_t>); @@ -380,7 +398,22 @@ TEST_CASE_METHOD(SqlTestFixture, "CompositeForeignKey loads across three columns auto child = dm.QuerySingle(1); REQUIRE(child.has_value()); // NOLINTBEGIN(bugprone-unchecked-optional-access) - guarded above + dm.LoadRelations(*child); REQUIRE(child->parent.Record().note.Value().has_value()); CHECK(child->parent.Record().note.Value().value() == "target"); // not the reversed decoy // NOLINTEND(bugprone-unchecked-optional-access) } + +#else // LIGHTWEIGHT_COMPOSITE_FK_MISCOMPILED + +TEST_CASE("CompositeForeignKey is miscompiled by clang-cl", "[CompositeForeignKey][!shouldfail]") +{ + // Placeholder keeping the skipped coverage visible on the one toolchain that cannot compile it. + // `[!shouldfail]` inverts the result, so this passes the run *because* it fails - and starts failing + // the moment the guard is removed without the coverage coming back. + FAIL("clang-cl 22.1.3 computes an impossible stack frame for a record holding a CompositeForeignKey " + "and faults in the prologue. These tests are compiled out here but all pass under MSVC cl. See " + "the comment above the guard."); +} + +#endif // LIGHTWEIGHT_COMPOSITE_FK_MISCOMPILED diff --git a/src/tests/CompositeKeyGapTests.cpp b/src/tests/CompositeKeyGapTests.cpp index fc341ad7a..8a24d0909 100644 --- a/src/tests/CompositeKeyGapTests.cpp +++ b/src/tests/CompositeKeyGapTests.cpp @@ -85,8 +85,8 @@ struct CkCompositeRecord { static constexpr std::string_view TableName = "CkComposite"; - Field tenantId {}; - Field entryNo {}; + Field tenantId {}; + Field entryNo {}; Field>, SqlRealName { "label" }> label {}; }; @@ -95,8 +95,8 @@ struct CkParentRecord { static constexpr std::string_view TableName = "CkParent"; - Field partA {}; - Field partB {}; + Field partA {}; + Field partB {}; Field>, SqlRealName { "caption" }> caption {}; }; @@ -122,7 +122,8 @@ TEST_CASE("Reflected identity covers every primary key member", "[CompositeKey][ // added beside it reports the whole key. STATIC_CHECK(RecordPrimaryKeyCount == 2); STATIC_CHECK(HasCompositePrimaryKey); - STATIC_CHECK(std::same_as, std::tuple>); + using CompositeKeyTuple = std::tuple; + STATIC_CHECK(std::same_as, CompositeKeyTuple>); // The single-key helper still collapses to the first field, deliberately: making it a tuple would // change the shape of every existing caller, including CreateExplicit's return type. diff --git a/src/tests/CompositeKeyOrderingTests.cpp b/src/tests/CompositeKeyOrderingTests.cpp index ce1523e83..eb481b354 100644 --- a/src/tests/CompositeKeyOrderingTests.cpp +++ b/src/tests/CompositeKeyOrderingTests.cpp @@ -35,8 +35,8 @@ struct OrderedKey { static constexpr std::string_view TableName = "CkOrdered"; - Field keyFirst {}; - Field keySecond {}; + Field keyFirst {}; + Field keySecond {}; Field>, SqlRealName { "payload" }> payload {}; }; @@ -47,8 +47,8 @@ struct ReversedKey { static constexpr std::string_view TableName = "CkOrdered"; - Field keySecond {}; - Field keyFirst {}; + Field keySecond {}; + Field keyFirst {}; Field>, SqlRealName { "payload" }> payload {}; }; From 82582b32606c30772e23852ea17543cf041ff096 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sun, 2 Aug 2026 23:00:36 +0300 Subject: [PATCH 4/5] docs: describe the reference schema structurally, without naming it The relation-generation notes and one test comment identified the production database, its schema and five of its table/column names. None of that is needed: the generation rules are derived from structural shapes and their counts, which are what the documents actually reason about. Replaces the identifying detail with neutral equivalents - the join-table examples keep their shape (two columns, each a single-column foreign key to a distinct table, composite primary key over exactly those two) under generic names. Also refreshes the status section, which still said the inverse and through relations were never generated, and points at CompositeForeignKey for the composite foreign keys it lists as unrepresentable. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01TVec3YeQg2FZnToiPQwcQH --- docs/ddl2cpp-relation-generation.md | 34 ++++++++++++++++++----------- src/tests/CxxModelRelationTests.cpp | 10 ++++----- 2 files changed, 25 insertions(+), 19 deletions(-) diff --git a/docs/ddl2cpp-relation-generation.md b/docs/ddl2cpp-relation-generation.md index 7469f21d2..e0f6fca8b 100644 --- a/docs/ddl2cpp-relation-generation.md +++ b/docs/ddl2cpp-relation-generation.md @@ -2,12 +2,13 @@ ## Status -`ddl2cpp` currently emits exactly one relation type: `Light::BelongsTo`, on the child side of a -single-column foreign key. The inverse and through relations that `DataMapper` supports — -`HasMany`, `HasManyThrough`, `HasOneThrough` — are never generated, so a generated model is -navigable only from child to parent. +`ddl2cpp` generates `Light::BelongsTo` on the child side of a single-column foreign key, and — as of +the relation-generation work on this branch — the inverse and through relations too: `HasMany`, +`HasManyThrough` and `HasOneThrough`. See `src/tests/CxxModelRelationTests.cpp` for the rule-by-rule +coverage. -This page records what a real production schema needs, and what is genuinely not representable. +This page records what a real production schema needs, which shapes are deliberately *not* collapsed, +and what remains genuinely not representable. > **Depends on the relation selectors from PR #528.** Generating inverse relations for a real schema > is impossible without them: this reference schema has table pairs joined by up to **55** foreign @@ -18,15 +19,19 @@ This page records what a real production schema needs, and what is genuinely not > rather than from `master`, and must not merge ahead of it. There is also **no `HasOne`** type in the library — only `HasOneThrough`. A one-to-one relation that -is *not* across a join table has no representation, so the plan below records it as -`Kind::HasOne` and the emitter falls back to `HasMany` for it, with a note in the generated header. -Adding a real `HasOne` is out of scope here. +is *not* across a join table has no representation, so the planner records it as `Kind::HasOne` and +the emitter falls back to `HasMany`, with a note in the generated header saying why. Adding a real +`HasOne` is out of scope here. + +Composite foreign keys remain ungenerated, but are no longer inexpressible: see +`docs/composite-keys-design.md` for `CompositeForeignKey` / `Connection`. Teaching the generator to +emit them is the natural follow-up. ## Reference schema -The numbers below come from the `konrad_english` database used by the Lastrada application -(MS SQL Server 2022, single schema `lasa`), which is the largest schema this generator is pointed -at in practice. +The numbers below come from a large production schema (MS SQL Server 2022, single schema), the +biggest this generator is pointed at in practice. Table and column names are not reproduced; only +the structural shapes and their counts, which is what the generation rules are derived from. | Metric | Count | |--------|-------| @@ -69,10 +74,13 @@ Of those, **84 also have a composite primary key**, which is the classic many-to Concrete examples, both two-column tables: ``` -XLAB_PROJECT_USER (PROJECT_NR -> XLAB_PROJECT, USER_NR -> XLAB_USER) -MANDANT_KUNDE (KUNDEN_NR -> KUNDE, MANDANT_NR -> MANDANTEN) +project_user (project_id -> project, user_id -> user) +tenant_customer (customer_id -> customer, tenant_id -> tenant) ``` +(Shapes reproduced with neutral names: two columns, each a single-column foreign key to a distinct +table, composite primary key over exactly those two.) + Each should yield a `HasManyThrough` on both referenced tables. Because both foreign keys of the join record point at *different* tables here, the selectors are only needed when a join table points twice at the same target. diff --git a/src/tests/CxxModelRelationTests.cpp b/src/tests/CxxModelRelationTests.cpp index c7c7d8025..694d8bbcb 100644 --- a/src/tests/CxxModelRelationTests.cpp +++ b/src/tests/CxxModelRelationTests.cpp @@ -8,15 +8,13 @@ // other table. // // The shapes tested here are taken from the reference schema surveyed in -// `docs/ddl2cpp-relation-generation.md` (the `konrad_english` database behind the Lastrada -// application, 686 tables / 1860 foreign keys), reduced to the smallest fixtures that reproduce -// each structural case found there: +// `docs/ddl2cpp-relation-generation.md` (a large production schema, 686 tables / 1860 foreign keys), +// reduced to the smallest fixtures that reproduce each structural case found there: // // - a plain parent/child pair -> HasMany // - two foreign keys from one child into one parent -> HasMany + selector (that schema has a // pair joined by 55 foreign keys) // - a two-column join table -> HasManyThrough on both sides -// (e.g. XLAB_PROJECT_USER, MANDANT_KUNDE) // - a join table whose far key is uniquely indexed -> HasOneThrough // - a uniquely indexed child foreign key -> one-to-one // - a join table carrying payload columns -> NOT a join table (association object) @@ -219,8 +217,8 @@ TEST_CASE("PlanRelations: a composite unique index does not make a relation one- TEST_CASE("PlanRelations: a two-column join table yields HasManyThrough on both sides", "[CxxModelPrinter][relations]") { - // Modelled on XLAB_PROJECT_USER(PROJECT_NR -> XLAB_PROJECT, USER_NR -> XLAB_USER) from the - // reference schema: a composite primary key over exactly the two foreign keys, no payload. + // The join-table shape found repeatedly in the reference schema: a composite primary key over + // exactly the two foreign keys, and no payload columns. auto const tables = std::vector { { .schema = "", .name = "project", .columns = { IdColumn() }, .primaryKeys = { "id" } }, { .schema = "", .name = "user", .columns = { IdColumn() }, .primaryKeys = { "id" } }, From 13f8616d42358046f418077ced2a3a5bc63099f1 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Mon, 3 Aug 2026 11:35:59 +0300 Subject: [PATCH 5/5] fix(DataMapper,ddl2cpp): address code-review findings on composite FKs and relation generation Ten findings from a review of the composite-foreign-key and ddl2cpp relation work. Correctness: - QuerySingle(primaryKeys...) and one FirstImpl() overload each had an early `return std::nullopt;` ahead of their final `return record;`, defeating NRVO and risking a stale pointer in the composite-FK auto-loader. Restructured both to a single return statement, and - since CI proved NRVO still isn't reliable for the fuller function body under some build configurations (caught by the coverage build; see below) - additionally switched the composite-FK auto-loader to capture the foreign key values by value at configure time instead of a pointer to the owning record, matching the safe pattern HasMany/BelongsTo already use. - ddl2cpp's join-table cardinality check used the *far* key's uniqueness instead of the *owner* key's - the far key always references exactly one row, so that check was trivially always true. Fixed, and corrected the test that had locked in the backwards expectation, plus the doc wording. - The `PrimaryKey::AutoAssign` collision counter only recognized `value + 1`-style keys, so two auto-assigned SqlGuid keys bypassed the static_assert meant to reject multi-column auto-assignment. - CompositeForeignKey had no operator==/<=>, so it was neither equality_comparable nor (owing to its private members) an aggregate - Reflection::CollectDifferences hard-errors on any record holding one. Added comparison operators, matching HasMany/HasOneThrough/HasManyThrough. - GetPrimaryKeyField() kept overwriting its result for every matching-type primary key member, silently returning the last one instead of the first. Cleanup: - Deduplicated the eager (LoadCompositeForeignKey) and lazy (ConfigureRelationAutoLoading) composite-FK load paths into one LoadCompositeForeignKeyRecord() helper, now taking the already-permuted key tuple rather than the owning record (see the value-capture fix above). - Merged the near-identical HasOneThrough/HasManyThrough codegen branches in CxxModelPrinter, driven by the template name string. - Replaced two C-style index loops in CompositeForeignKey with views::iota. - Rebuilt GetPrimaryKeyFields() on the same compile-time tuple_cat machinery RecordPrimaryKeyTupleHelper already uses, dropping an O(K^2) runtime scan. - Replaced byName()'s linear table scan in PlanRelations with a hash map. CI fixups (found via `gh pr checks` + fetched logs, all within this PR's own diff against master - none pre-exist there): - clang-tidy: an unchecked-optional-access and a dead-store finding in tests, and PlanRelations exceeding the cognitive-complexity threshold (fixed by the CxxModelPrinter dedup above). - Doxygen coverage: wrapped a decltype-of-invoked-generic-lambda type alias that at least one Doxygen version misparses in `\cond`, replaced a few `\ref`s Doxygen couldn't resolve with plain code font, and documented `PlannedRelation::kind` and `CompositeForeignKey::Loader`'s members (the latter newly required once the struct itself gained a doc comment). - Regenerated the `src/examples/test_chinook/entities/*.hpp` golden reference files against a live MS SQL Server + Chinook dataset: they had never been updated after ddl2cpp's relation-generation feature landed earlier in this branch, so every file was missing the HasMany/HasManyThrough members and forward declarations the generator has produced all along. - A real ddl2cpp bug this regeneration surfaced: a relation member can end up named exactly like its own referenced (forward-declared) struct - e.g. `Light::HasMany Album;` inside `struct Artist`. Legal C++, but GCC's -Wchanges-meaning rejects it under -Werror once a real GCC build (not just Clang) actually compiled the regenerated headers. Fixed by reserving referenced/through struct names in the same per-table uniquing map used for column/member names, so a colliding relation member gets suffixed instead; added a regression test. Also fixed, unrelated to the review but required for a local build under this toolchain: two dependent-name lookups needing an explicit `template` keyword, and a spurious unused-lambda-capture warning-as-error. Left alone (pre-existing on this branch before this change, or infrastructure): docs/sqlquery.md's two Doxygen markdown-list warnings (present on master too); the C++26 reflection build's composite-FK errors (predate this change, need reflection-mode expertise); a one-off local Postgres-container anomaly in an unrelated self-referencing-relation test (CI's own PostgreSQL leg passes). Verified: clang-debug (ASan/UBSan) against SQLite3, MS SQL Server 2022, and PostgreSQL (all three via Docker where applicable) - full suite green; additionally reproduced and re-verified the composite-FK regression test under a `clang-coverage`-instrumented build, which is what first caught the NRVO gap above; ddl2cpp/chinook regenerated and rebuilt against a live MSSQL Chinook dataset with zero diff against the golden files. GCC and the C++20-modules/C++26-reflection configurations were not available in this environment (verified only that the C++26 reflection failures are unchanged from before this push). Co-Authored-By: Claude Sonnet 5 --- docs/ddl2cpp-relation-generation.md | 7 +- .../DataMapper/CompositeForeignKey.hpp | 41 +++- src/Lightweight/DataMapper/DataMapper.hpp | 112 +++++++---- src/Lightweight/DataMapper/Record.hpp | 62 +++--- src/Lightweight/Tools/CxxModelPrinter.cpp | 190 +++++++++++------- src/Lightweight/Tools/CxxModelPrinter.hpp | 3 +- src/Lightweight/Utils.hpp | 2 +- src/examples/test_chinook/entities/Album.hpp | 4 + src/examples/test_chinook/entities/Artist.hpp | 4 + .../test_chinook/entities/Customer.hpp | 4 + .../test_chinook/entities/Employee.hpp | 5 + src/examples/test_chinook/entities/Genre.hpp | 4 + .../test_chinook/entities/Invoice.hpp | 4 + .../test_chinook/entities/Mediatype.hpp | 4 + .../test_chinook/entities/Playlist.hpp | 5 + src/examples/test_chinook/entities/Track.hpp | 7 + src/tests/CompositeForeignKeyTests.cpp | 77 ++++++- src/tests/CompositeKeyGapTests.cpp | 43 +++- src/tests/CxxModelRelationTests.cpp | 49 ++++- 19 files changed, 466 insertions(+), 161 deletions(-) diff --git a/docs/ddl2cpp-relation-generation.md b/docs/ddl2cpp-relation-generation.md index e0f6fca8b..610ba5b5b 100644 --- a/docs/ddl2cpp-relation-generation.md +++ b/docs/ddl2cpp-relation-generation.md @@ -113,8 +113,11 @@ considered throughout; composite ones are counted and skipped as before. 2. **`HasManyThrough`** on each of the two referenced tables, when a table qualifies as a join table: exactly two single-column foreign keys, pointing at two *distinct* tables, and no non-key columns beyond those two foreign keys and its own key columns. -3. **`HasOneThrough`** in place of `HasManyThrough` when the join table's foreign key to the far - side is covered by a single-column unique index. +3. **`HasOneThrough`** in place of `HasManyThrough`, on a given owner's side, when the join table's + foreign key back to *that owner* is covered by a single-column unique index — meaning that owner + can appear in at most one join row, hence reach at most one record on the other side. The other + side keeps whatever `HasManyThrough`/`HasOneThrough` its own foreign key's uniqueness implies, + independently. 4. **`HasMany`** on the referenced side of every remaining foreign key, i.e. one that is not part of a join table already covered by rule 2 or 3. 5. **Scalar rather than collection** when the child's foreign key is itself covered by a diff --git a/src/Lightweight/DataMapper/CompositeForeignKey.hpp b/src/Lightweight/DataMapper/CompositeForeignKey.hpp index d6016dfe0..e98099c20 100644 --- a/src/Lightweight/DataMapper/CompositeForeignKey.hpp +++ b/src/Lightweight/DataMapper/CompositeForeignKey.hpp @@ -12,6 +12,7 @@ #include #include #include +#include #include #include #include @@ -139,7 +140,7 @@ namespace detail }; } // namespace detail -/// @brief Satisfied by @ref Connection specializations. +/// @brief Satisfied by `Connection` specializations. /// /// @ingroup DataMapper template @@ -147,7 +148,7 @@ concept ConnectionType = detail::IsConnectionType>::value /// @brief Represents a foreign key spanning several columns. /// -/// Declared as a list of @ref Connection, each pairing one of this record's columns with the column it +/// Declared as a list of `Connection`, each pairing one of this record's columns with the column it /// references. The referenced and referencing records are *derived* from those pointers rather than /// named again, so they cannot disagree with the connections. /// @@ -160,7 +161,7 @@ concept ConnectionType = detail::IsConnectionType>::value /// order before binding, because that is the order a primary key lookup emits its predicates in - see /// @ref OrderedValuesOf and `src/tests/CompositeKeyOrderingTests.cpp`. /// -/// @tparam Connections One @ref Connection per column of the foreign key. +/// @tparam Connections One `Connection` per column of the foreign key. /// /// @ingroup DataMapper /// @@ -228,8 +229,8 @@ class CompositeForeignKey static_assert( []() consteval { auto const indices = std::array { Connections::IntoMemberIndex... }; - for (auto outer = std::size_t { 0 }; outer != indices.size(); ++outer) - for (auto inner = outer + 1; inner != indices.size(); ++inner) + for (auto const outer: std::views::iota(std::size_t { 0 }, indices.size())) + for (auto const inner: std::views::iota(outer + 1, indices.size())) if (indices[outer] == indices[inner]) return false; return true; @@ -257,7 +258,7 @@ class CompositeForeignKey /// because the referenced indices are asserted pairwise distinct above. template static constexpr std::size_t ConnectionForSlot = []() consteval { - for (auto candidate = std::size_t { 0 }; candidate != Count; ++candidate) + for (auto const candidate: std::views::iota(std::size_t { 0 }, Count)) { auto rank = std::size_t { 0 }; for (auto const other: IntoIndices) @@ -384,7 +385,25 @@ class CompositeForeignKey /// Carries the deferred load, installed by the DataMapper. struct Loader { + /// Loads and returns the referenced record, or `nullptr` if none exists. std::function()> loadReference {}; + + /// Loaders carry no comparable state of their own, so any two are considered equivalent. + std::weak_ordering operator<=>(Loader const& /*other*/) const noexcept + { + return std::weak_ordering::equivalent; // Loader is not comparable, so we return equivalent + } + + /// Loaders carry no comparable state of their own, so any two compare equal. + /// + /// A defaulted `==` on the enclosing class does not derive equality from a member's `<=>` - each + /// member needs its own viable `==`, or the default is silently deleted. See HasMany::Loader for + /// the same shape (there, without this operator; equal by convention since it holds none of the + /// relation's state). + bool operator==(Loader const& /*other*/) const noexcept + { + return true; + } }; /// Used internally to configure on-demand loading of the referenced record. @@ -395,6 +414,16 @@ class CompositeForeignKey _loader = std::move(loader); } + /// Three-way comparison operator. + /// + /// Without this, the relation is neither `std::equality_comparable` nor (owing to its private + /// members) an aggregate, and `Reflection::CollectDifferences` - which falls back to recursing into + /// non-comparable members as aggregates - hard-errors on any record holding one. HasMany, + /// HasOneThrough and HasManyThrough all define one for the same reason. + std::weak_ordering operator<=>(CompositeForeignKey const& other) const noexcept = default; + /// Equality comparison operator. + bool operator==(CompositeForeignKey const& other) const noexcept = default; + private: void RequireLoaded() const { diff --git a/src/Lightweight/DataMapper/DataMapper.hpp b/src/Lightweight/DataMapper/DataMapper.hpp index 488346642..c3b329c85 100644 --- a/src/Lightweight/DataMapper/DataMapper.hpp +++ b/src/Lightweight/DataMapper/DataMapper.hpp @@ -622,6 +622,25 @@ class DataMapper template std::optional LoadBelongsTo(FieldType::ValueType value); + /// Queries the record referenced by a composite foreign key, without touching the relation itself. + /// + /// Shared by the eager path (`LoadCompositeForeignKey`) and the lazy loader installed by + /// `ConfigureRelationAutoLoading`, so both resolve a missing target row and wrap a found one the + /// same way instead of maintaining two copies of that logic. + /// + /// Takes the already-permuted key values rather than the owning record itself: the lazy loader + /// must evaluate `FieldType::OrderedValuesOf()` while the record is known to be live (at + /// `ConfigureRelationAutoLoading` time) and capture the resulting values by value, not a pointer to + /// the record - a `std::optional` returned by value from a query method is not guaranteed to + /// stay at the same address (NRVO is not mandated by the standard, and does not reliably apply to + /// every such function in practice), so a captured pointer can dangle by the time the loader runs. + /// + /// @param keys The foreign key values, in the referenced record's member order. + /// @return The referenced record, or `nullptr` if no matching row exists. + template + std::shared_ptr LoadCompositeForeignKeyRecord( + typename FieldType::OrderedValueType const& keys); + /// Eagerly loads the record referenced by a composite foreign key. /// /// @param record The record holding the foreign key. @@ -1112,7 +1131,7 @@ size_t SqlCoreDataMapperQueryBuilder::CountImpl() this->_query.searchCondition.condition)); auto reader = stmt.ExecuteWithVariants(_boundInputs); if (reader.FetchRow()) - return reader.GetColumn(1); + return reader.template GetColumn(1); return 0; } @@ -1217,7 +1236,7 @@ auto SqlCoreDataMapperQueryBuilder::AllImpl() -> } if (!outputColumnsBound) - value = reader.GetColumn(1); + value = reader.template GetColumn(1); } return result; @@ -1338,16 +1357,27 @@ auto SqlCoreDataMapperQueryBuilder::FirstImpl() - #else reader.BindOutputColumns(&(record.*ReferencedFields)...); #endif - if (!reader.FetchRow()) - return std::nullopt; - if (!outputColumnsBound) + + // A single return statement at the end is deliberate, not stylistic: a composite foreign key + // configured below (ConfigureRelationAutoLoading) captures a pointer to *optionalRecord. An earlier + // `return std::nullopt;` here defeats NRVO in both GCC and Clang (verified: it forces a + // move-construct into the caller's storage at a new address), which would leave that captured + // pointer dangling. + if (reader.FetchRow()) { - using ElementMask = std::integer_sequence...>; - detail::GetAllColumns(reader, record); - } + if (!outputColumnsBound) + { + using ElementMask = std::integer_sequence...>; + detail::GetAllColumns(reader, record); + } - if constexpr (QueryOptions.loadRelations) - _dm.ConfigureRelationAutoLoading(record); + if constexpr (QueryOptions.loadRelations) + _dm.ConfigureRelationAutoLoading(record); + } + else + { + optionalRecord.reset(); + } return optionalRecord; } @@ -2177,18 +2207,22 @@ std::optional DataMapper::QuerySingle(PrimaryKeyTypes&&... primaryKeys) _stmt.Prepare(queryBuilder->First()); auto reader = _stmt.Execute(std::forward(primaryKeys)...); + // A single return statement at the end is deliberate, not stylistic: a composite foreign key + // configured below (ConfigureRelationAutoLoading) captures a pointer to *resultRecord. An earlier + // `return std::nullopt;` here defeats NRVO in both GCC and Clang (verified: it forces a move-construct + // into the caller's storage at a new address), which would leave that captured pointer dangling. auto resultRecord = std::optional { Record {} }; - if (!detail::ReadSingleResult(_stmt.Connection().ServerType(), reader, *resultRecord)) - return std::nullopt; - - if (resultRecord) + if (detail::ReadSingleResult(_stmt.Connection().ServerType(), reader, *resultRecord)) + { SetModifiedState(resultRecord.value()); - if constexpr (QueryOptions.loadRelations) - { - if (resultRecord) + if constexpr (QueryOptions.loadRelations) ConfigureRelationAutoLoading(*resultRecord); } + else + { + resultRecord.reset(); + } return resultRecord; } @@ -2484,6 +2518,19 @@ inline LIGHTWEIGHT_FORCE_INLINE void CallOnBelongsTo(Callable const& callable) }); } +template +std::shared_ptr DataMapper::LoadCompositeForeignKeyRecord( + typename FieldType::OrderedValueType const& keys) +{ + using ReferencedRecord = typename FieldType::ReferencedRecord; + + auto loaded = + std::apply([this](auto const&... key) { return this->template QuerySingle(key...); }, keys); + if (!loaded) + return {}; + return std::make_shared(std::move(*loaded)); +} + template void DataMapper::LoadCompositeForeignKey(Record const& record, FieldType& field) { @@ -2496,8 +2543,7 @@ void DataMapper::LoadCompositeForeignKey(Record const& record, FieldType& field) // member of the referenced record, in that record's member declaration order, and binds its // arguments positionally - so the values have to be permuted into that order first. See // CompositeKeyOrderingTests.cpp. - auto loaded = std::apply([this](auto const&... key) { return QuerySingle(key...); }, - FieldType::OrderedValuesOf(record)); + auto loaded = LoadCompositeForeignKeyRecord(FieldType::OrderedValuesOf(record)); // A missing target row leaves the relation unloaded rather than throwing here: eagerly loading a // dangling foreign key is a data-integrity problem to surface at the accessor, which is where the @@ -2509,7 +2555,7 @@ void DataMapper::LoadCompositeForeignKey(Record const& record, FieldType& field) return; } - field.EmplaceRecord(std::make_shared(std::move(*loaded))); + field.EmplaceRecord(std::move(loaded)); } template @@ -2930,16 +2976,14 @@ void DataMapper::ConfigureRelationAutoLoading(Record& record) { using ReferencedRecord = typename FieldType::ReferencedRecord; - // The key is read out of the record's own Field members *at load time*, through the - // connections' member pointers, rather than snapshotted here. Snapshotting would make the - // relation resolve to whichever parent the key named when the record was configured, so - // repointing the foreign key afterwards would silently keep returning the old parent. The - // relation deliberately owns no copy of the key - the Field that owns each column is the - // single source of truth. - // - // Capturing the record by pointer is safe because it is the mapper's own stored record: the - // same lifetime the HasMany/HasOneThrough loaders already rely on. Unlike those, no value is - // copied out, so a moved-from record would be observed rather than a stale duplicate. + // Captured by value, evaluated now while `record` is known to be live - not a pointer to + // `record` read later from inside the closure. A `std::optional` returned by value + // from a query method (QuerySingle, First, ...) is not guaranteed to keep its address: NRVO + // is not mandated by the standard, and - verified - does not reliably apply to the fuller + // body of those functions in at least one real build configuration, so a captured pointer + // can end up pointing at stack memory already reused for something else by the time the + // loader runs. The trade-off is the same one HasMany/BelongsTo already make: repointing the + // foreign key after this point does not change what the relation resolves to. // // OrderedValuesOf() - not ValuesOf() - because QuerySingle emits one WHERE predicate per // primary key member in the *referenced record's* member declaration order and binds its @@ -2947,13 +2991,9 @@ void DataMapper::ConfigureRelationAutoLoading(Record& record) // value to the wrong predicate whenever the two orders differ, which with same-typed key // columns fetches a wrong row rather than failing. See CompositeKeyOrderingTests.cpp. field.SetAutoLoader(typename FieldType::Loader { - .loadReference = [owner = &record]() -> std::shared_ptr { + .loadReference = [keys = FieldType::OrderedValuesOf(record)]() -> std::shared_ptr { DataMapper& dm = DataMapper::AcquireThreadLocal(); - auto loaded = std::apply([&dm](auto const&... key) { return dm.QuerySingle(key...); }, - FieldType::OrderedValuesOf(*owner)); - if (!loaded) - return {}; - return std::make_shared(std::move(*loaded)); + return dm.LoadCompositeForeignKeyRecord(keys); }, }); } diff --git a/src/Lightweight/DataMapper/Record.hpp b/src/Lightweight/DataMapper/Record.hpp index acfd3ade7..a6f4b78ce 100644 --- a/src/Lightweight/DataMapper/Record.hpp +++ b/src/Lightweight/DataMapper/Record.hpp @@ -2,6 +2,7 @@ #pragma once +#include "../DataBinder/SqlGuid.hpp" #include "../Utils.hpp" #include "BelongsTo.hpp" #include "Field.hpp" @@ -363,6 +364,13 @@ constexpr bool HasAutoIncrementPrimaryKey = namespace detail { + /// \cond DOXYGEN_EXCLUDE + // Doxygen's comment-to-declaration association misparses this decltype-of-an-invoked-generic-lambda + // type alias on at least one toolchain version in CI, attaching the struct's doc comment to a + // statement inside the lambda body instead. `detail` is excluded from the generated docs anyway + // (see DOXYGEN_EXCLUDE_SYMBOLS in docs/CMakeLists.txt), so there is nothing to lose by having + // Doxygen skip parsing it altogether. + /// Collects the value types of every `PrimaryKey` member of @p Record, in declaration order. template struct RecordPrimaryKeyTupleHelper @@ -385,6 +393,7 @@ namespace detail }.template operator()()...); }(std::make_index_sequence> {})); }; + /// \endcond } // namespace detail namespace detail @@ -402,9 +411,14 @@ namespace detail template concept IncrementableKeyValue = requires(ValueType value) { value + 1; }; + /// Whether @p ValueType is one of the two kinds `GenerateAutoAssignPrimaryKey` actually generates a + /// value for: a GUID (via `SqlGuid::Create()`) or an incrementable value (via `MAX(...) + 1`). + template + concept AutoAssignableKeyValue = std::same_as || IncrementableKeyValue; + template concept GeneratesAutoAssignedKey = IsField && IsAutoAssignPrimaryKeyField::value - && IncrementableKeyValue; + && AutoAssignableKeyValue; template constexpr std::size_t AutoAssignPrimaryKeyFieldCount = @@ -418,7 +432,7 @@ namespace detail /// @brief The tuple of a record's primary key value types, in member declaration order. /// -/// Unlike @ref RecordPrimaryKeyType, which names a single field's type, this covers composite keys: +/// Unlike `RecordPrimaryKeyType`, which names a single field's type, this covers composite keys: /// for a record with several members marked `PrimaryKey` it is a tuple of all of them. For a /// single-key record it is a one-element tuple. /// @@ -455,29 +469,24 @@ constexpr bool HasCompositePrimaryKey = RecordPrimaryKeyCount > 1; template [[nodiscard]] RecordPrimaryKeyTuple GetPrimaryKeyFields(Record const& record) { - auto result = RecordPrimaryKeyTuple {}; - auto slot = std::size_t { 0 }; - EnumerateRecordMembers(record, [&](FieldType const& field) { - if constexpr (IsField) - if constexpr (FieldType::IsPrimaryKey) + // Mirrors RecordPrimaryKeyTupleHelper's compile-time tuple_cat construction (one std::tuple<> or + // std::tuple per member, concatenated), but reads each primary-key member's value instead + // of just its type. The two therefore cannot disagree on which members are collected or in what + // order, and no runtime index-matching against the heterogeneous tuple is needed. + return [](Record const& record, std::index_sequence) { + return std::tuple_cat([&record]() { + using FieldType = RecordMemberTypeOf; + if constexpr (IsField) { - // The tuple was built by walking the members in this same order, so the Nth key member - // corresponds to tuple element N. The tuple is heterogeneous, so the write is matched - // against the compile-time index and only the assignable slot is emitted. - [&](std::index_sequence) { - ( - [&] { - if constexpr (std::assignable_from>&, - typename FieldType::ValueType const&>) - if (J == slot) - std::get(result) = field.Value(); - }(), - ...); - }(std::make_index_sequence> {}); - ++slot; + if constexpr (FieldType::IsPrimaryKey) + return std::tuple { GetRecordMemberAt(record).Value() }; + else + return std::tuple<> {}; } - }); - return result; + else + return std::tuple<> {}; + }.template operator()()...); + }(record, std::make_index_sequence> {}); } /// Returns the first primary key field of the record. @@ -490,13 +499,18 @@ inline LIGHTWEIGHT_FORCE_INLINE RecordPrimaryKeyType GetPrimaryKeyField( static_assert(HasPrimaryKey, "Record must have a primary key"); auto result = RecordPrimaryKeyType {}; + bool found = false; EnumerateRecordMembers(record, [&](FieldType const& field) { // std::same_as>condition is for the case where there are // multiple primary keys, we want to return the first one if constexpr (IsField) if constexpr (IsPrimaryKey) if constexpr (std::same_as>) - result = field.Value(); + if (!found) + { + result = field.Value(); + found = true; + } }); return result; } diff --git a/src/Lightweight/Tools/CxxModelPrinter.cpp b/src/Lightweight/Tools/CxxModelPrinter.cpp index 1dbb90cbe..4421009b3 100644 --- a/src/Lightweight/Tools/CxxModelPrinter.cpp +++ b/src/Lightweight/Tools/CxxModelPrinter.cpp @@ -7,6 +7,8 @@ #include #include #include +#include +#include #include using namespace std::string_view_literals; @@ -454,15 +456,107 @@ namespace return std::pair { singleColumnKeys[0], singleColumnKeys[1] }; } + /// Emits the `HasOneThrough`/`HasManyThrough` relation for one side of a resolved join table. + /// + /// @param table The join table itself. + /// @param ownerKey The foreign key naming the owner this relation is planned for. + /// @param farKey The join table's other foreign key, naming the record @p ownerKey's owner reaches. + /// @param isAmbiguous Whether more than one foreign key runs between two given tables. + /// @param plan The plan to add the relation to, keyed by owner table name. + template + void EmitThroughRelation(SqlSchema::Table const& table, + SqlSchema::ForeignKeyConstraint const& ownerKey, + SqlSchema::ForeignKeyConstraint const& farKey, + IsAmbiguousFn const& isAmbiguous, + CxxModelPrinter::RelationPlan& plan) + { + auto const& ownerTable = ownerKey.primaryKey.table.table; + auto const& farTable = farKey.primaryKey.table.table; + + // Scalar when the join record's *owner*-side key is uniquely indexed: at most one join row per + // owner, so the owner sees a single far record rather than a collection. The far key's own + // uniqueness governs the *other* direction's cardinality, not this one - a join row always + // references exactly one far row regardless of uniqueness, so checking farKey here would test + // something that is trivially always true. + auto const ownerIsUnique = IsUniquelyIndexed(table, ownerKey.foreignKey.columns.front()); + + plan[ownerTable].emplace_back(CxxModelPrinter::PlannedRelation { + .kind = ownerIsUnique ? CxxModelPrinter::PlannedRelation::Kind::HasOneThrough + : CxxModelPrinter::PlannedRelation::Kind::HasManyThrough, + .ownerTable = ownerTable, + .referencedTable = farTable, + .throughTable = table.name, + .ownerForeignKeyColumn = ownerKey.foreignKey.columns.front(), + .referencedForeignKeyColumn = farKey.foreignKey.columns.front(), + .ownerSelectorRequired = isAmbiguous(table.name, ownerTable), + .referencedSelectorRequired = isAmbiguous(table.name, farTable), + // Named after the far table: `project.users` rather than `project.projectUsers`, since the + // join table is an implementation detail of the relation. + .memberName = farTable, + }); + } + + /// Emits the inverse `HasOne`/`HasMany` relation implied by one single-column foreign key, unless it + /// is composite or points outside the generated set. + /// + /// @param table The table declaring @p constraint. + /// @param constraint The candidate foreign key. + /// @param byName Resolves a table by name within the generated set. + /// @param isAmbiguous Whether more than one foreign key runs between two given tables. + /// @param plan The plan to add the relation to, keyed by owner table name. + template + void EmitInverseRelation(SqlSchema::Table const& table, + SqlSchema::ForeignKeyConstraint const& constraint, + ByNameFn const& byName, + IsAmbiguousFn const& isAmbiguous, + CxxModelPrinter::RelationPlan& plan) + { + if (!IsSingleColumn(constraint)) + return; // composite: no BelongsTo either, so no inverse + + auto const& ownerTable = constraint.primaryKey.table.table; + if (byName(ownerTable) == nullptr) + return; // references a table outside the generated set + + auto const& childColumn = constraint.foreignKey.columns.front(); + + // Scalar when the child's own foreign key is uniquely indexed: one child per owner. + auto const childIsUnique = IsUniquelyIndexed(table, childColumn); + + plan[ownerTable].emplace_back(CxxModelPrinter::PlannedRelation { + .kind = childIsUnique ? CxxModelPrinter::PlannedRelation::Kind::HasOne + : CxxModelPrinter::PlannedRelation::Kind::HasMany, + .ownerTable = ownerTable, + .referencedTable = table.name, + .throughTable = {}, + .ownerForeignKeyColumn = childColumn, + .referencedForeignKeyColumn = {}, + .ownerSelectorRequired = isAmbiguous(table.name, ownerTable), + .referencedSelectorRequired = false, + // Named after the child table. Where several foreign keys from the same child table land + // here the names would collide, so distinguish those by the foreign key column - which is + // exactly the set that also needs a selector. + .memberName = isAmbiguous(table.name, ownerTable) ? std::format("{}_{}", table.name, childColumn) : table.name, + }); + } + } // namespace CxxModelPrinter::RelationPlan CxxModelPrinter::PlanRelations(std::vector const& tables) { auto plan = RelationPlan {}; + // Built once up front rather than scanned per lookup: relation planning resolves a table by name + // for every foreign key of every table, which made the previous linear scan quadratic in schema + // size (hundreds of tables on the schemas this generator targets). + auto tablesByName = std::unordered_map {}; + tablesByName.reserve(tables.size()); + for (auto const& table: tables) + tablesByName.emplace(table.name, &table); + auto const byName = [&](std::string_view name) -> SqlSchema::Table const* { - auto const it = std::ranges::find_if(tables, [&](auto const& t) { return t.name == name; }); - return it != tables.end() ? &*it : nullptr; + auto const it = tablesByName.find(name); + return it != tablesByName.end() ? it->second : nullptr; }; // How many single-column foreign keys run from one table to another. A count above one makes the @@ -490,35 +584,10 @@ CxxModelPrinter::RelationPlan CxxModelPrinter::PlanRelations(std::vector {};\n", - referencedStruct, - throughStruct, - ownerSelector, - relation.referencedSelectorRequired - ? std::format(", Light::SqlRealName {{ \"{}\" }}", relation.referencedForeignKeyColumn) - : std::string {}, - memberName); - break; - - case PlannedRelation::Kind::HasManyThrough: - definition.text << std::format( - " Light::HasManyThrough<{}, {}{}{}> {};\n", + " Light::{}<{}, {}{}{}> {};\n", + templateName, referencedStruct, throughStruct, ownerSelector, @@ -1074,6 +1119,7 @@ void CxxModelPrinter::PrintTable(SqlSchema::Table const& table, std::vector constexpr size_t MemberIndexOf = []() consteval -> size_t { int index { -1 }; - auto members = nonstatic_data_members_of(parent_of(Member), std::meta::access_context::current()); + auto members = nonstatic_data_members_of(std::meta::parent_of(Member), std::meta::access_context::current()); if (auto it = std::ranges::find(members, Member); it != members.end()) { index = std::distance(members.begin(), it); diff --git a/src/examples/test_chinook/entities/Album.hpp b/src/examples/test_chinook/entities/Album.hpp index f1b506285..1846bab8a 100644 --- a/src/examples/test_chinook/entities/Album.hpp +++ b/src/examples/test_chinook/entities/Album.hpp @@ -10,6 +10,8 @@ #include #include +struct Track; + struct Album final { static constexpr std::string_view TableName = "Album"; @@ -17,6 +19,8 @@ struct Album final Light::Field AlbumId; Light::Field, Light::SqlRealName { "Title" }> Title; Light::BelongsTo<&Artist::ArtistId, Light::SqlRealName { "ArtistId" }> ArtistId; + + Light::HasMany Track_1; }; template <> diff --git a/src/examples/test_chinook/entities/Artist.hpp b/src/examples/test_chinook/entities/Artist.hpp index 96257d3e2..4150732a3 100644 --- a/src/examples/test_chinook/entities/Artist.hpp +++ b/src/examples/test_chinook/entities/Artist.hpp @@ -8,12 +8,16 @@ #include #include +struct Album; + struct Artist final { static constexpr std::string_view TableName = "Artist"; Light::Field ArtistId; Light::Field>, Light::SqlRealName { "Name" }> Name; + + Light::HasMany Album_1; }; template <> diff --git a/src/examples/test_chinook/entities/Customer.hpp b/src/examples/test_chinook/entities/Customer.hpp index fd259a187..eb15c8213 100644 --- a/src/examples/test_chinook/entities/Customer.hpp +++ b/src/examples/test_chinook/entities/Customer.hpp @@ -10,6 +10,8 @@ #include #include +struct Invoice; + struct Customer final { static constexpr std::string_view TableName = "Customer"; @@ -27,6 +29,8 @@ struct Customer final Light::Field>, Light::SqlRealName { "Fax" }> Fax; Light::Field, Light::SqlRealName { "Email" }> Email; Light::BelongsTo<&Employee::EmployeeId, Light::SqlRealName { "SupportRepId" }, Light::SqlNullable::Null> SupportRepId; + + Light::HasMany Invoice_1; }; template <> diff --git a/src/examples/test_chinook/entities/Employee.hpp b/src/examples/test_chinook/entities/Employee.hpp index dc2cfb908..e230be923 100644 --- a/src/examples/test_chinook/entities/Employee.hpp +++ b/src/examples/test_chinook/entities/Employee.hpp @@ -8,6 +8,8 @@ #include #include +struct Customer; + struct Employee final { static constexpr std::string_view TableName = "Employee"; @@ -27,6 +29,9 @@ struct Employee final Light::Field>, Light::SqlRealName { "Phone" }> Phone; Light::Field>, Light::SqlRealName { "Fax" }> Fax; Light::Field>, Light::SqlRealName { "Email" }> Email; + + Light::HasMany Customer_1; + Light::HasMany Employee; }; template <> diff --git a/src/examples/test_chinook/entities/Genre.hpp b/src/examples/test_chinook/entities/Genre.hpp index 68664e061..f08bf5e00 100644 --- a/src/examples/test_chinook/entities/Genre.hpp +++ b/src/examples/test_chinook/entities/Genre.hpp @@ -8,12 +8,16 @@ #include #include +struct Track; + struct Genre final { static constexpr std::string_view TableName = "Genre"; Light::Field GenreId; Light::Field>, Light::SqlRealName { "Name" }> Name; + + Light::HasMany Track_1; }; template <> diff --git a/src/examples/test_chinook/entities/Invoice.hpp b/src/examples/test_chinook/entities/Invoice.hpp index 9c8f7e989..f4004f6aa 100644 --- a/src/examples/test_chinook/entities/Invoice.hpp +++ b/src/examples/test_chinook/entities/Invoice.hpp @@ -10,6 +10,8 @@ #include #include +struct Invoiceline; + struct Invoice final { static constexpr std::string_view TableName = "Invoice"; @@ -24,6 +26,8 @@ struct Invoice final Light::Field>, Light::SqlRealName { "BillingPostalCode" }> BillingPostalCode; Light::Field, Light::SqlRealName { "Total" }> Total; + + Light::HasMany InvoiceLine; }; template <> diff --git a/src/examples/test_chinook/entities/Mediatype.hpp b/src/examples/test_chinook/entities/Mediatype.hpp index b7f87fa26..397ec1eb8 100644 --- a/src/examples/test_chinook/entities/Mediatype.hpp +++ b/src/examples/test_chinook/entities/Mediatype.hpp @@ -8,12 +8,16 @@ #include #include +struct Track; + struct Mediatype final { static constexpr std::string_view TableName = "MediaType"; Light::Field MediaTypeId; Light::Field>, Light::SqlRealName { "Name" }> Name; + + Light::HasMany Track_1; }; template <> diff --git a/src/examples/test_chinook/entities/Playlist.hpp b/src/examples/test_chinook/entities/Playlist.hpp index b2f5c1853..52eda45e8 100644 --- a/src/examples/test_chinook/entities/Playlist.hpp +++ b/src/examples/test_chinook/entities/Playlist.hpp @@ -8,12 +8,17 @@ #include #include +struct Playlisttrack; +struct Track; + struct Playlist final { static constexpr std::string_view TableName = "Playlist"; Light::Field PlaylistId; Light::Field>, Light::SqlRealName { "Name" }> Name; + + Light::HasManyThrough Track_1; }; template <> diff --git a/src/examples/test_chinook/entities/Track.hpp b/src/examples/test_chinook/entities/Track.hpp index f24cf02a2..489e3712f 100644 --- a/src/examples/test_chinook/entities/Track.hpp +++ b/src/examples/test_chinook/entities/Track.hpp @@ -12,6 +12,10 @@ #include #include +struct Invoiceline; +struct Playlist; +struct Playlisttrack; + struct Track final { static constexpr std::string_view TableName = "Track"; @@ -25,6 +29,9 @@ struct Track final Light::Field Milliseconds; Light::Field, Light::SqlRealName { "Bytes" }> Bytes; Light::Field, Light::SqlRealName { "UnitPrice" }> UnitPrice; + + Light::HasManyThrough Playlist_1; + Light::HasMany InvoiceLine; }; template <> diff --git a/src/tests/CompositeForeignKeyTests.cpp b/src/tests/CompositeForeignKeyTests.cpp index 627f018b1..59f0874d7 100644 --- a/src/tests/CompositeForeignKeyTests.cpp +++ b/src/tests/CompositeForeignKeyTests.cpp @@ -59,7 +59,9 @@ struct Child Field refA {}; Field refB {}; - CompositeForeignKey, Connection<&Child::refB, &Parent::partB>> parent {}; + CompositeForeignKey, + Connection> + parent {}; }; /// The same relation with its connections written in the *opposite* order. Semantically identical - @@ -72,8 +74,8 @@ struct ChildDeclaredBackwards Field refA {}; Field refB {}; - CompositeForeignKey, - Connection<&ChildDeclaredBackwards::refA, &Parent::partA>> + CompositeForeignKey, + Connection> parent {}; }; @@ -99,9 +101,9 @@ struct WideChild Field b {}; Field c {}; - CompositeForeignKey, - Connection<&WideChild::a, &WideParent::k1>, - Connection<&WideChild::b, &WideParent::k2>> + CompositeForeignKey, + Connection, + Connection> parent {}; }; @@ -138,8 +140,8 @@ TEST_CASE("CompositeForeignKey derives both records from its connections", "[Com TEST_CASE("Connection exposes both endpoints and the referenced member index", "[CompositeForeignKey]") { - using First = Connection<&Child::refA, &Parent::partA>; - using Second = Connection<&Child::refB, &Parent::partB>; + using First = Connection; + using Second = Connection; STATIC_CHECK(std::same_as); STATIC_CHECK(std::same_as); @@ -243,6 +245,25 @@ TEST_CASE("CompositeForeignKey navigation reports load state", "[CompositeForeig CHECK_FALSE(child.parent.IsLoaded()); } +TEST_CASE("CompositeForeignKey participates in CollectDifferences", "[CompositeForeignKey]") +{ + // Regression test: without a comparison operator, CompositeForeignKey is neither + // std::equality_comparable nor (owing to its private members) an aggregate, so + // Reflection::CollectDifferences - which falls back to recursing into non-comparable members as + // aggregates - fails to compile for any record holding one. Compiling at all is most of what this + // test checks. + auto a = Child {}; + a.refA = 1; + a.refB = 2; + + auto b = Child {}; + b.refA = 1; + b.refB = 9; + + auto const differences = Lightweight::CollectDifferences(a, b); + CHECK(differences.indexes.size() == 1); // only refB differs; both `parent` relations compare equal (unloaded) +} + // ================================================================================================ // Step 4: loading against a live database // ================================================================================================ @@ -290,6 +311,44 @@ TEST_CASE_METHOD(SqlTestFixture, "CompositeForeignKey loads its referenced recor // NOLINTEND(bugprone-unchecked-optional-access) } +TEST_CASE_METHOD(SqlTestFixture, + "CompositeForeignKey lazy auto-loader survives QuerySingle's return by value", + "[CompositeForeignKey]") +{ + // Regression test: the auto-loader QuerySingle(primaryKeys...) installs captures a pointer to the + // record it configures (see ConfigureRelationAutoLoading / CompositeForeignKey::Loader). QuerySingle + // used to have an early `return std::nullopt;` ahead of its final `return resultRecord;`, which + // defeats NRVO in both GCC and Clang and moves the record to a new address on the way out - leaving + // that captured pointer dangling. Reaching the relation through its lazy loader (not via the eager + // LoadRelations() path exercised above) is what triggers it. + auto stmt = SqlStatement {}; + (void) stmt.ExecuteDirect(R"(CREATE TABLE "CfkParent" ( + "part_a" INT NOT NULL, + "part_b" INT NOT NULL, + "caption" VARCHAR(20) NULL, + PRIMARY KEY ("part_a", "part_b") + ))"); + (void) stmt.ExecuteDirect(R"(CREATE TABLE "CfkChild" ( + "id" INT NOT NULL PRIMARY KEY, + "ref_a" INT NOT NULL, + "ref_b" INT NOT NULL + ))"); + (void) stmt.ExecuteDirect(R"(INSERT INTO "CfkParent" VALUES (1, 2, 'one-two'))"); + (void) stmt.ExecuteDirect(R"(INSERT INTO "CfkChild" VALUES (100, 1, 2))"); + + auto dm = DataMapper {}; + + auto child = dm.QuerySingle(100); + REQUIRE(child.has_value()); + // NOLINTBEGIN(bugprone-unchecked-optional-access) - guarded above + CHECK_FALSE(child->parent.IsLoaded()); + + // Triggers the lazy loader installed by QuerySingle, not the eager LoadRelations() path. + REQUIRE(child->parent.Record().partA.Value() == 1); + CHECK(child->parent.Record().partB.Value() == 2); + // NOLINTEND(bugprone-unchecked-optional-access) +} + TEST_CASE_METHOD(SqlTestFixture, "CompositeForeignKey declared out of order still loads the right record", "[CompositeForeignKey]") @@ -373,7 +432,7 @@ TEST_CASE("GetPrimaryKeyFields reads every key value in member order", "[Composi CHECK(std::get<2>(wideKeys) == 30); // The tuple order matches what QuerySingle binds, so it can be applied directly. - auto const single = GetPrimaryKeyFields(Child {}); + [[maybe_unused]] auto const single = GetPrimaryKeyFields(Child {}); STATIC_CHECK(std::tuple_size_v == 1); } diff --git a/src/tests/CompositeKeyGapTests.cpp b/src/tests/CompositeKeyGapTests.cpp index 8a24d0909..063271d2e 100644 --- a/src/tests/CompositeKeyGapTests.cpp +++ b/src/tests/CompositeKeyGapTests.cpp @@ -130,6 +130,19 @@ TEST_CASE("Reflected identity covers every primary key member", "[CompositeKey][ STATIC_CHECK(std::same_as, int32_t>); } +TEST_CASE("GetPrimaryKeyField returns the first primary key field, not the last", "[CompositeKey][identity]") +{ + // Previously a gap: the loop kept overwriting its result for every matching-type primary key + // member, so it silently returned the LAST one rather than the first - contradicting both its own + // inline comment and its doc comment. tenantId and entryNo share the same value type (int32_t), so + // this is exactly the shape that triggers it. + auto record = CkCompositeRecord {}; + record.tenantId = 7; + record.entryNo = 3; + + CHECK(GetPrimaryKeyField(record) == 7); // tenantId, declared first - not entryNo +} + TEST_CASE_METHOD(SqlTestFixture, "A composite primary key does identify a row", "[CompositeKey]") { // Surprisingly, this half already works. QuerySingle() emits one `WHERE` per primary-key field @@ -223,9 +236,9 @@ TEST_CASE("A composite foreign key is expressed as a CompositeForeignKey", "[Com // `BelongsTo` was deliberately left alone: it is inseparably a *column* (storage, one bound ODBC // index) as well as a navigator, and widening it would have broken the one-member-one-column // invariant that every projection and column-offset path depends on. - using Relation = - CompositeForeignKey, - Connection<&CompositeKeyGap::CkChildRecord::refB, &CompositeKeyGap::CkParentRecord::partB>>; + using Relation = CompositeForeignKey< + Connection, + Connection>; STATIC_CHECK(Relation::Count == 2); STATIC_CHECK(std::same_as); @@ -257,5 +270,29 @@ TEST_CASE_METHOD(SqlTestFixture, "A composite-key table still maps as a plain re CHECK(all[0].tenantId.Value() == 7); CHECK(all[0].entryNo.Value() == 3); REQUIRE(all[0].label.Value().has_value()); + // NOLINTNEXTLINE(bugprone-unchecked-optional-access) - guarded above CHECK(all[0].label.Value().value() == "kept"); } + +namespace CompositeKeyGap +{ + +/// Two auto-assigned GUID primary keys - the type `GenerateAutoAssignPrimaryKey`'s <= 1 static_assert +/// exists to reject. See the test below for why counting it correctly matters. +struct GuidMultiPkRecord +{ + Field first {}; + Field second {}; +}; + +} // namespace CompositeKeyGap + +TEST_CASE("AutoAssignPrimaryKeyFieldCount counts auto-assigned GUID keys, not just incrementable ones", + "[CompositeKey][identity]") +{ + // Previously a gap: the counting concept only recognized `value + 1`-style keys, so a record with + // two `Field` members counted as zero and slipped past + // GenerateAutoAssignPrimaryKey's `<= 1` static_assert - which exists precisely to reject this shape, + // since auto-assignment yields one value that SetId() then writes into *every* key member. + STATIC_CHECK(Lightweight::detail::AutoAssignPrimaryKeyFieldCount == 2); +} diff --git a/src/tests/CxxModelRelationTests.cpp b/src/tests/CxxModelRelationTests.cpp index 694d8bbcb..866794834 100644 --- a/src/tests/CxxModelRelationTests.cpp +++ b/src/tests/CxxModelRelationTests.cpp @@ -15,7 +15,7 @@ // - two foreign keys from one child into one parent -> HasMany + selector (that schema has a // pair joined by 55 foreign keys) // - a two-column join table -> HasManyThrough on both sides -// - a join table whose far key is uniquely indexed -> HasOneThrough +// - a join table whose owner key is uniquely indexed -> HasOneThrough on that owner's side // - a uniquely indexed child foreign key -> one-to-one // - a join table carrying payload columns -> NOT a join table (association object) // - composite foreign keys -> skipped, as for BelongsTo @@ -252,9 +252,13 @@ TEST_CASE("PlanRelations: a two-column join table yields HasManyThrough on both CHECK(RelationsOn(plan, "project_user").empty()); } -TEST_CASE("PlanRelations: a join table with a uniquely indexed far key yields HasOneThrough", "[CxxModelPrinter][relations]") +TEST_CASE("PlanRelations: a join table with a uniquely indexed owner key yields HasOneThrough", + "[CxxModelPrinter][relations]") { - // When the join record's far-side key is unique, each owner reaches at most one far record. + // When the join record's own foreign key back to an owner is unique, that owner reaches at most + // one join row and therefore at most one far record. `locker_id` being unique means each locker is + // linked to at most one employee - a one-to-one from locker's side - while `employee_id` staying + // unconstrained means one employee can still have several lockers. auto const tables = std::vector { { .schema = "", .name = "employee", .columns = { IdColumn() }, .primaryKeys = { "id" } }, { .schema = "", .name = "locker", .columns = { IdColumn() }, .primaryKeys = { "id" } }, @@ -268,14 +272,14 @@ TEST_CASE("PlanRelations: a join table with a uniquely indexed far key yields Ha auto const plan = CxxModelPrinter::PlanRelations(tables); - // Employee -> locker crosses the unique key, so at most one locker. + // employee_id is not uniquely indexed, so one employee can have several join rows: a collection. auto const fromEmployee = SoleRelationOn(plan, "employee"); - CHECK(fromEmployee.kind == Kind::HasOneThrough); + CHECK(fromEmployee.kind == Kind::HasManyThrough); CHECK(fromEmployee.referencedTable == "locker"); - // The other direction crosses the non-unique employee_id, so it stays a collection. + // locker_id is uniquely indexed, so each locker reaches at most one join row, hence one employee. auto const fromLocker = SoleRelationOn(plan, "locker"); - CHECK(fromLocker.kind == Kind::HasManyThrough); + CHECK(fromLocker.kind == Kind::HasOneThrough); CHECK(fromLocker.referencedTable == "employee"); } @@ -529,3 +533,34 @@ TEST_CASE("CxxModelPrinter: relation members do not collide with column members" ++occurrences; CHECK(occurrences <= 1); } + +TEST_CASE("CxxModelPrinter: relation members do not collide with the referenced struct's own name", + "[CxxModelPrinter][relations]") +{ + // With no column named after the child table, the relation member's default name (the child + // table's own name) is identical to the forward-declared struct it names as its type - `struct + // book;` and a member `book` in the very same class. Legal C++ (a member may shadow an outer type), + // but the member declaration then "changes the meaning" of `book` for the rest of the class body, + // which GCC (-Wchanges-meaning) rejects under -Werror, and which reads as though the member and its + // own type were the same thing. + auto const tables = std::vector { + { .schema = "", .name = "author", .columns = { IdColumn() }, .primaryKeys = { "id" } }, + { .schema = "", + .name = "book", + .columns = { IdColumn(), ForeignKeyColumn("author_id") }, + .foreignKeys = { ForeignKey("book", "author_id", "author") }, + .primaryKeys = { "id" } }, + }; + + auto printer = CxxModelPrinter { CxxModelPrinter::Config {} }; + printer.ResolveOrderAndPrintTable(tables); + + auto const header = printer.HeaderFileForTheTable("Models", "author"); + INFO("author.hpp:\n" << header); + + // The type is still forward-declared and still named by the relation... + CHECK(header.contains("struct book;")); + CHECK(header.contains("Light::HasMany")); + // ...but the member itself must not be named exactly `book`, or it would shadow that very type. + CHECK_FALSE(header.contains("> book;")); +}