From 4a1b766fc39a0160283e054b271a9932cc55ac94 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Wed, 19 Aug 2026 10:41:29 +0300 Subject: [PATCH 1/2] ddl2cpp: register relation members in the generated Description<> CxxModelPrinter built Description from the columns alone: relation members were emitted into the struct body but never added to FieldCount / Members / FieldNames. Because RecordMemberCount prefers the descriptor whenever a specialization exists, EnumerateRecordMembers never visited the relation, ConfigureRelationAutoLoading installed no loader, and the first access threw SqlRequireLoadedError. The descriptor stands in for reflection, so it must list every non-static data member, not only the ones that map onto a column - RecordColumnCount already documents that it differs from RecordMemberCount exactly by the relation members. Column-only consumers select by the RecordColumnMember concept rather than by index, so the extra entries cost them nothing. A relation has no SQL column, so its FieldNames slot carries the C++ member name, which is what reflection reports there. Fixing that exposed a second defect the truncated descriptor had been hiding: the generator plans inverse and through relations for foreign keys it never emits a BelongsTo for. A column that is both a primary key and a foreign key is emitted as a plain Field (the isForeignKey && !isPrimaryKey guard at the column emission site), yet HasMany, HasOne, HasManyThrough and HasOneThrough all resolve their other end through exactly that BelongsTo. Chinook's PlaylistTrack is that shape, and once ConfigureRelationAutoLoading could finally see the relation members the example stopped compiling on a static_assert. PlanRelations now skips those relations for the same reason it already skips composite foreign keys - no BelongsTo, no relation - which also disqualifies the classic composite-key join table from the through-relation path. A join table carrying a key of its own is unaffected. Supporting the composite-key shape needs BelongsTo to be usable as a primary key, which it is not today; docs/ddl2cpp-relation-generation.md records that as rule 6 along with the reference-schema impact. Regenerates the checked-in Chinook entity headers to match, verified against real ddl2cpp output, and traverses Album::Track_1 in the example so the ddl2cpp CI leg covers relation loading end to end. The through-relation fixtures in CxxModelRelationTests move to a join table with its own key, which keeps the cardinality rules covered, and a new case pins the skipped shape. Fixes #556 Signed-off-by: Yaraslau Tamashevich --- docs/ddl2cpp-relation-generation.md | 17 +++ src/Lightweight/Description.hpp | 7 + src/Lightweight/Tools/CxxModelPrinter.cpp | 28 ++++ src/examples/test_chinook/entities/Album.hpp | 6 +- src/examples/test_chinook/entities/Artist.hpp | 6 +- .../test_chinook/entities/Customer.hpp | 13 +- .../test_chinook/entities/Employee.hpp | 14 +- src/examples/test_chinook/entities/Genre.hpp | 6 +- .../test_chinook/entities/Invoice.hpp | 12 +- .../test_chinook/entities/Mediatype.hpp | 6 +- .../test_chinook/entities/Playlist.hpp | 5 - src/examples/test_chinook/entities/Track.hpp | 15 +- src/examples/test_chinook/main.cpp | 7 + src/tests/CMakeLists.txt | 1 + src/tests/CxxModelPrinterTests.cpp | 56 ++++++++ src/tests/CxxModelRelationTests.cpp | 52 +++++-- .../DataMapper/DescriptorRelationTests.cpp | 133 ++++++++++++++++++ 17 files changed, 332 insertions(+), 52 deletions(-) create mode 100644 src/tests/DataMapper/DescriptorRelationTests.cpp diff --git a/docs/ddl2cpp-relation-generation.md b/docs/ddl2cpp-relation-generation.md index 610ba5b5b..64b7bf848 100644 --- a/docs/ddl2cpp-relation-generation.md +++ b/docs/ddl2cpp-relation-generation.md @@ -85,6 +85,13 @@ Each should yield a `HasManyThrough` on both referenced tables. Because both for join record point at *different* tables here, the selectors are only needed when a join table points twice at the same target. +> **Not yet generated for the composite-key shape.** Of the 159 join-table candidates, the 84 whose +> primary key *is* their two foreign keys are skipped by rule 6 below: those columns are emitted as +> plain `Field`s rather than `BelongsTo`, and a through-relation has no other way to resolve its two +> ends, so generating one yields a record that does not compile. The remaining 75 — join tables with +> a key of their own — are generated as described here. Lifting this needs `BelongsTo` to support +> being a primary key. + ### 3. `HasOneThrough` / one-to-one 24 single-column foreign keys sit under a single-column unique index, which makes the relation @@ -122,6 +129,16 @@ considered throughout; composite ones are counted and skipped as before. 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. +6. **No relation at all** when the child's foreign key column is also part of that child's own + primary key. Such a column is emitted as a plain `Field`, never a `BelongsTo` (see rule 1), and + every inverse relation — `HasMany`, `HasOne` and both through-relations — resolves its other end + through exactly that `BelongsTo`. Generating one anyway produces a record that does not compile. + This rules out the classic composite-key join table, whose primary key *is* its two foreign keys; + a join table carrying a key of its own is unaffected and still yields rule 2 or 3. + + Supporting the composite-key shape needs `BelongsTo` to be usable as a primary key, which it is + not today (`BelongsTo::IsPrimaryKey` is hard-coded `false`). Until that changes, the generator + emits nothing there rather than something that cannot be built. 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 diff --git a/src/Lightweight/Description.hpp b/src/Lightweight/Description.hpp index c8cba85c8..08c895023 100644 --- a/src/Lightweight/Description.hpp +++ b/src/Lightweight/Description.hpp @@ -118,6 +118,13 @@ struct RecordMemberList /// - `static constexpr std::array FieldNames;` — the resolved SQL /// column name for each field (i.e. the value `FieldNameAt` would otherwise compute). /// +/// `Members` must list *every* non-static data member in declaration order, relation members +/// (`HasMany`, `HasManyThrough`, `HasOneThrough`) included — it stands in for reflection, so omitting +/// one hides it from `EnumerateRecordMembers` and relation auto-loading silently never runs. +/// Consumers that want only the columns filter by the `RecordColumnMember` concept (see +/// `RecordColumnCount`), never by index. A relation has no SQL column, so its `FieldNames` slot +/// carries its C++ member name, which is what reflection reports there. +/// /// @ingroup DataMapper template struct Description; diff --git a/src/Lightweight/Tools/CxxModelPrinter.cpp b/src/Lightweight/Tools/CxxModelPrinter.cpp index 1dfb5ad10..b9f10c255 100644 --- a/src/Lightweight/Tools/CxxModelPrinter.cpp +++ b/src/Lightweight/Tools/CxxModelPrinter.cpp @@ -453,6 +453,18 @@ namespace if (!std::ranges::all_of(table.columns, [&](auto const& c) { return isKeyColumn(c.name); })) return std::nullopt; + // A column that is both a primary key and a foreign key is emitted as a plain Field, never a + // BelongsTo (see the `isForeignKey && !isPrimaryKey` guard at the column emission site). A + // through-relation resolves its join record's owner and far sides through exactly those BelongsTo + // members, so a join table keyed on its own foreign keys cannot satisfy the relation it would + // otherwise imply - the generated code would not compile. Skip it for the same reason + // EmitInverseRelation skips composite foreign keys: no BelongsTo, no relation (#556). + auto const keyedOnItsOwnForeignKey = [&](SqlSchema::ForeignKeyConstraint const& constraint) { + return std::ranges::contains(table.primaryKeys, constraint.foreignKey.columns.front()); + }; + if (keyedOnItsOwnForeignKey(singleColumnKeys[0]) || keyedOnItsOwnForeignKey(singleColumnKeys[1])) + return std::nullopt; + return std::pair { singleColumnKeys[0], singleColumnKeys[1] }; } @@ -520,6 +532,13 @@ namespace auto const& childColumn = constraint.foreignKey.columns.front(); + // Same reason as the composite case: a column that is both a primary key and a foreign key is + // emitted as a plain Field, never a BelongsTo (see the `isForeignKey && !isPrimaryKey` guard at + // the column emission site). HasMany resolves its inverse through that BelongsTo, so without one + // the relation cannot be satisfied and the generated record fails to compile (#556). + if (std::ranges::contains(table.primaryKeys, childColumn)) + return; + // Scalar when the child's own foreign key is uniquely indexed: one child per owner. auto const childIsUnique = IsUniquelyIndexed(table, childColumn); @@ -1083,6 +1102,15 @@ void CxxModelPrinter::PrintTable(SqlSchema::Table const& table, std::vector::FieldCount whenever a specialization exists, so a + // relation left out here is invisible to EnumerateRecordMembers - ConfigureRelationAutoLoading + // then installs no loader and the first access throws SqlRequireLoadedError (#556). Column-only + // consumers select members by the RecordColumnMember concept rather than by index, so the extra + // entry costs them nothing. Reflection reports a relation's C++ member name where a column would + // report its SQL name, so mirror that to keep both enumeration paths interchangeable. + definition.members.emplace_back(memberName, memberName); + auto const ownerSelector = relation.ownerSelectorRequired ? std::format(", Light::SqlRealName {{ \"{}\" }}", relation.ownerForeignKeyColumn) : std::string {}; diff --git a/src/examples/test_chinook/entities/Album.hpp b/src/examples/test_chinook/entities/Album.hpp index 1846bab8a..f5bb7cc2a 100644 --- a/src/examples/test_chinook/entities/Album.hpp +++ b/src/examples/test_chinook/entities/Album.hpp @@ -26,7 +26,7 @@ struct Album final template <> struct Lightweight::Description { - static constexpr std::size_t FieldCount = 3; - using Members = Lightweight::RecordMemberList<&Album::AlbumId, &Album::Title, &Album::ArtistId>; - static constexpr std::array FieldNames = { "AlbumId", "Title", "ArtistId" }; + static constexpr std::size_t FieldCount = 4; + using Members = Lightweight::RecordMemberList<&Album::AlbumId, &Album::Title, &Album::ArtistId, &Album::Track_1>; + static constexpr std::array FieldNames = { "AlbumId", "Title", "ArtistId", "Track_1" }; }; diff --git a/src/examples/test_chinook/entities/Artist.hpp b/src/examples/test_chinook/entities/Artist.hpp index 4150732a3..df520efad 100644 --- a/src/examples/test_chinook/entities/Artist.hpp +++ b/src/examples/test_chinook/entities/Artist.hpp @@ -23,7 +23,7 @@ struct Artist final template <> struct Lightweight::Description { - static constexpr std::size_t FieldCount = 2; - using Members = Lightweight::RecordMemberList<&Artist::ArtistId, &Artist::Name>; - static constexpr std::array FieldNames = { "ArtistId", "Name" }; + static constexpr std::size_t FieldCount = 3; + using Members = Lightweight::RecordMemberList<&Artist::ArtistId, &Artist::Name, &Artist::Album_1>; + static constexpr std::array FieldNames = { "ArtistId", "Name", "Album_1" }; }; diff --git a/src/examples/test_chinook/entities/Customer.hpp b/src/examples/test_chinook/entities/Customer.hpp index eb15c8213..fb0961f42 100644 --- a/src/examples/test_chinook/entities/Customer.hpp +++ b/src/examples/test_chinook/entities/Customer.hpp @@ -36,7 +36,7 @@ struct Customer final template <> struct Lightweight::Description { - static constexpr std::size_t FieldCount = 13; + static constexpr std::size_t FieldCount = 14; using Members = Lightweight::RecordMemberList<&Customer::CustomerId, &Customer::FirstName, &Customer::LastName, @@ -49,9 +49,10 @@ struct Lightweight::Description &Customer::Phone, &Customer::Fax, &Customer::Email, - &Customer::SupportRepId>; - static constexpr std::array FieldNames = { "CustomerId", "FirstName", "LastName", "Company", - "Address", "City", "State", "Country", - "PostalCode", "Phone", "Fax", "Email", - "SupportRepId" }; + &Customer::SupportRepId, + &Customer::Invoice_1>; + static constexpr std::array FieldNames = { "CustomerId", "FirstName", "LastName", "Company", + "Address", "City", "State", "Country", + "PostalCode", "Phone", "Fax", "Email", + "SupportRepId", "Invoice_1" }; }; diff --git a/src/examples/test_chinook/entities/Employee.hpp b/src/examples/test_chinook/entities/Employee.hpp index e230be923..b72d5b632 100644 --- a/src/examples/test_chinook/entities/Employee.hpp +++ b/src/examples/test_chinook/entities/Employee.hpp @@ -37,7 +37,7 @@ struct Employee final template <> struct Lightweight::Description { - static constexpr std::size_t FieldCount = 15; + static constexpr std::size_t FieldCount = 17; using Members = Lightweight::RecordMemberList<&Employee::EmployeeId, &Employee::LastName, &Employee::FirstName, @@ -52,9 +52,11 @@ struct Lightweight::Description &Employee::PostalCode, &Employee::Phone, &Employee::Fax, - &Employee::Email>; - static constexpr std::array FieldNames = { "EmployeeId", "LastName", "FirstName", "Title", - "ReportsTo", "BirthDate", "HireDate", "Address", - "City", "State", "Country", "PostalCode", - "Phone", "Fax", "Email" }; + &Employee::Email, + &Employee::Customer_1, + &Employee::Employee>; + static constexpr std::array FieldNames = { + "EmployeeId", "LastName", "FirstName", "Title", "ReportsTo", "BirthDate", "HireDate", "Address", "City", + "State", "Country", "PostalCode", "Phone", "Fax", "Email", "Customer_1", "Employee" + }; }; diff --git a/src/examples/test_chinook/entities/Genre.hpp b/src/examples/test_chinook/entities/Genre.hpp index f08bf5e00..e62dea386 100644 --- a/src/examples/test_chinook/entities/Genre.hpp +++ b/src/examples/test_chinook/entities/Genre.hpp @@ -23,7 +23,7 @@ struct Genre final template <> struct Lightweight::Description { - static constexpr std::size_t FieldCount = 2; - using Members = Lightweight::RecordMemberList<&Genre::GenreId, &Genre::Name>; - static constexpr std::array FieldNames = { "GenreId", "Name" }; + static constexpr std::size_t FieldCount = 3; + using Members = Lightweight::RecordMemberList<&Genre::GenreId, &Genre::Name, &Genre::Track_1>; + static constexpr std::array FieldNames = { "GenreId", "Name", "Track_1" }; }; diff --git a/src/examples/test_chinook/entities/Invoice.hpp b/src/examples/test_chinook/entities/Invoice.hpp index f4004f6aa..e800f6e36 100644 --- a/src/examples/test_chinook/entities/Invoice.hpp +++ b/src/examples/test_chinook/entities/Invoice.hpp @@ -33,7 +33,7 @@ struct Invoice final template <> struct Lightweight::Description { - static constexpr std::size_t FieldCount = 9; + static constexpr std::size_t FieldCount = 10; using Members = Lightweight::RecordMemberList<&Invoice::InvoiceId, &Invoice::CustomerId, &Invoice::InvoiceDate, @@ -42,8 +42,10 @@ struct Lightweight::Description &Invoice::BillingState, &Invoice::BillingCountry, &Invoice::BillingPostalCode, - &Invoice::Total>; - static constexpr std::array FieldNames = { "InvoiceId", "CustomerId", "InvoiceDate", - "BillingAddress", "BillingCity", "BillingState", - "BillingCountry", "BillingPostalCode", "Total" }; + &Invoice::Total, + &Invoice::InvoiceLine>; + static constexpr std::array FieldNames = { "InvoiceId", "CustomerId", "InvoiceDate", + "BillingAddress", "BillingCity", "BillingState", + "BillingCountry", "BillingPostalCode", "Total", + "InvoiceLine" }; }; diff --git a/src/examples/test_chinook/entities/Mediatype.hpp b/src/examples/test_chinook/entities/Mediatype.hpp index 397ec1eb8..24591fc36 100644 --- a/src/examples/test_chinook/entities/Mediatype.hpp +++ b/src/examples/test_chinook/entities/Mediatype.hpp @@ -23,7 +23,7 @@ struct Mediatype final template <> struct Lightweight::Description { - static constexpr std::size_t FieldCount = 2; - using Members = Lightweight::RecordMemberList<&Mediatype::MediaTypeId, &Mediatype::Name>; - static constexpr std::array FieldNames = { "MediaTypeId", "Name" }; + static constexpr std::size_t FieldCount = 3; + using Members = Lightweight::RecordMemberList<&Mediatype::MediaTypeId, &Mediatype::Name, &Mediatype::Track_1>; + static constexpr std::array FieldNames = { "MediaTypeId", "Name", "Track_1" }; }; diff --git a/src/examples/test_chinook/entities/Playlist.hpp b/src/examples/test_chinook/entities/Playlist.hpp index f76ebcad0..b2f5c1853 100644 --- a/src/examples/test_chinook/entities/Playlist.hpp +++ b/src/examples/test_chinook/entities/Playlist.hpp @@ -8,17 +8,12 @@ #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 70f2555bf..c7c9c245a 100644 --- a/src/examples/test_chinook/entities/Track.hpp +++ b/src/examples/test_chinook/entities/Track.hpp @@ -13,8 +13,6 @@ #include struct Invoiceline; -struct Playlist; -struct Playlisttrack; struct Track final { @@ -30,14 +28,13 @@ struct Track final Light::Field, Light::SqlRealName { "Bytes" }> Bytes; Light::Field, Light::SqlRealName { "UnitPrice" }> UnitPrice; - Light::HasManyThrough> Playlist_1; Light::HasMany InvoiceLine; }; template <> struct Lightweight::Description { - static constexpr std::size_t FieldCount = 9; + static constexpr std::size_t FieldCount = 10; using Members = Lightweight::RecordMemberList<&Track::TrackId, &Track::Name, &Track::AlbumId, @@ -46,8 +43,10 @@ struct Lightweight::Description &Track::Composer, &Track::Milliseconds, &Track::Bytes, - &Track::UnitPrice>; - static constexpr std::array FieldNames = { "TrackId", "Name", "AlbumId", - "MediaTypeId", "GenreId", "Composer", - "Milliseconds", "Bytes", "UnitPrice" }; + &Track::UnitPrice, + &Track::InvoiceLine>; + static constexpr std::array FieldNames = { "TrackId", "Name", "AlbumId", + "MediaTypeId", "GenreId", "Composer", + "Milliseconds", "Bytes", "UnitPrice", + "InvoiceLine" }; }; diff --git a/src/examples/test_chinook/main.cpp b/src/examples/test_chinook/main.cpp index 60810c3a1..33a1cef94 100644 --- a/src/examples/test_chinook/main.cpp +++ b/src/examples/test_chinook/main.cpp @@ -139,6 +139,13 @@ int main() Log("Artist name: {}", toString(album.ArtistId->Name.Value().value().c_str())); // NOLINT(bugprone-unchecked-optional-access) + // The other direction: HasMany is a relation member, not a column, so it only loads if the + // generated Description lists it alongside the columns (#556). Traversing it here keeps the + // ddl2cpp CI leg covering that end to end. + Log("Album has {} tracks", album.Track_1.Count()); + for (auto const& track: album.Track_1.All()) + Log(" Track: {}", toString(track->Name.Value().ToStringView())); + { // get an artist with the name "Sir Georg Solti, Sumi Jo & Wiener Philharmoniker" auto artist = dm.Query() // NOLINT(bugprone-unchecked-optional-access) diff --git a/src/tests/CMakeLists.txt b/src/tests/CMakeLists.txt index f654ec59f..c47f8b89f 100644 --- a/src/tests/CMakeLists.txt +++ b/src/tests/CMakeLists.txt @@ -55,6 +55,7 @@ set(SOURCE_FILES DataMapper/RelationShapeTests.cpp DataMapper/RelationTests.cpp DataMapper/BelongsToStateTests.cpp + DataMapper/DescriptorRelationTests.cpp DataMapper/InstantiationCoverageTests.cpp DataMapper/StateTests.cpp DataMapper/ThroughMarkerTests.cpp diff --git a/src/tests/CxxModelPrinterTests.cpp b/src/tests/CxxModelPrinterTests.cpp index 5131b5f5e..67a5000b5 100644 --- a/src/tests/CxxModelPrinterTests.cpp +++ b/src/tests/CxxModelPrinterTests.cpp @@ -984,3 +984,59 @@ TEST_CASE("CxxModelPrinter::PrintReport summarizes tables and multi-key FK warni // stdout; the assertion is that the full report path executes. CHECK_NOTHROW(printer.PrintReport()); } + +// Regression coverage for issue #556: the descriptor is the *member* list, not the column list. +// `RecordMemberCount` prefers `Description::FieldCount` whenever a specialization exists, so +// a descriptor that stops at the columns hides the relation members from `EnumerateRecordMembers` — +// `ConfigureRelationAutoLoading` then installs no loader and the first access throws +// `SqlRequireLoadedError`. Column-only consumers filter by the `RecordColumnMember` concept, not by +// index, so listing relations here costs them nothing (see `RecordColumnCount` in Record.hpp). +TEST_CASE("CxxModelPrinter: Description covers relation members, not just columns", "[CxxModelPrinter]") +{ + using namespace Lightweight::SqlColumnTypeDefinitions; + + auto const tables = std::vector { + Lightweight::SqlSchema::Table { + .schema = "", + .name = "customers", + .columns = { Lightweight::SqlSchema::Column { + .name = "id", .type = Integer {}, .isNullable = false, .isPrimaryKey = true } }, + .primaryKeys = { "id" }, + }, + Lightweight::SqlSchema::Table { + .schema = "", + .name = "orders", + .columns = { Lightweight::SqlSchema::Column { + .name = "id", .type = Integer {}, .isNullable = false, .isPrimaryKey = true }, + Lightweight::SqlSchema::Column { + .name = "customer_id", .type = Integer {}, .isNullable = false, .isForeignKey = true } }, + .foreignKeys = { Lightweight::SqlSchema::ForeignKeyConstraint { + .foreignKey = { .table = { .catalog = "", .schema = "", .table = "orders" }, .columns = { "customer_id" } }, + .primaryKey = { .table = { .catalog = "", .schema = "", .table = "customers" }, .columns = { "id" } }, + } }, + .primaryKeys = { "id" }, + }, + }; + + auto const plan = CxxModelPrinter::PlanRelations(tables); + CxxModelPrinter::Config config; + config.makeAliases = true; + CxxModelPrinter printer { config }; + auto const customersRelations = plan.find("customers"); + REQUIRE(customersRelations != plan.end()); + REQUIRE(customersRelations->second.size() == 1); + printer.PrintTable(tables[0], customersRelations->second); + + auto const output = printer.ToString("Models"); + INFO(output); + + // The HasMany is emitted into the struct body ... + CHECK(output.contains("Light::HasMany orders;")); + // ... so the descriptor must count it and list it too, or relation auto-loading never runs. + CHECK(output.contains("static constexpr std::size_t FieldCount = 2;")); + CHECK(output.contains("using Members = Lightweight::RecordMemberList<&Models::Customers::id, " + "&Models::Customers::orders>;")); + // A relation has no SQL column of its own; reflection would report its C++ member name here, so + // the descriptor mirrors that to keep the two enumeration paths interchangeable. + CHECK(output.contains(R"(FieldNames = { "id", "orders" };)")); +} diff --git a/src/tests/CxxModelRelationTests.cpp b/src/tests/CxxModelRelationTests.cpp index e5cca007e..b9d2b37d7 100644 --- a/src/tests/CxxModelRelationTests.cpp +++ b/src/tests/CxxModelRelationTests.cpp @@ -217,17 +217,18 @@ 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]") { - // The join-table shape found repeatedly in the reference schema: a composite primary key over - // exactly the two foreign keys, and no payload columns. + // A join table with a key of its own, so both foreign key columns become BelongsTo members and the + // through-relation can actually resolve them. (A join table keyed on its own foreign keys is + // skipped instead - see the composite-key test below.) 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") }, + .columns = { IdColumn(), ForeignKeyColumn("project_id"), ForeignKeyColumn("user_id") }, .foreignKeys = { ForeignKey("project_user", "project_id", "project"), ForeignKey("project_user", "user_id", "user") }, - .primaryKeys = { "project_id", "user_id" } }, + .primaryKeys = { "id" } }, }; auto const plan = CxxModelPrinter::PlanRelations(tables); @@ -264,10 +265,10 @@ TEST_CASE("PlanRelations: a join table with a uniquely indexed owner key yields { .schema = "", .name = "locker", .columns = { IdColumn() }, .primaryKeys = { "id" } }, { .schema = "", .name = "employee_locker", - .columns = { ForeignKeyColumn("employee_id"), ForeignKeyColumn("locker_id", /*isUnique=*/true) }, + .columns = { IdColumn(), 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" } }, + .primaryKeys = { "id" } }, }; auto const plan = CxxModelPrinter::PlanRelations(tables); @@ -283,6 +284,37 @@ TEST_CASE("PlanRelations: a join table with a uniquely indexed owner key yields CHECK(fromLocker.referencedTable == "employee"); } +TEST_CASE("PlanRelations: a join table keyed on its own foreign keys yields no through-relation", + "[CxxModelPrinter][relations]") +{ + // The classic composite-key join table: its primary key *is* the two foreign keys. ddl2cpp emits a + // column that is both a primary key and a foreign key as a plain Field rather than a BelongsTo, and + // a through-relation can only resolve its two sides through BelongsTo members - so planning one here + // would generate a record that does not compile (#556). Chinook's PlaylistTrack is exactly this + // shape, and the relations it used to imply were unusable: relation auto-loading never reached them + // only because the generated Description<> omitted relation members altogether. + // + // Supporting this shape needs BelongsTo to be usable as a primary key, which it is not today + // (BelongsTo::IsPrimaryKey is hard-coded false). Until then, generate nothing rather than something + // broken. A join table with a key of its own keeps working - see the HasManyThrough test above. + auto const tables = std::vector { + { .schema = "", .name = "playlist", .columns = { IdColumn() }, .primaryKeys = { "id" } }, + { .schema = "", .name = "track", .columns = { IdColumn() }, .primaryKeys = { "id" } }, + { .schema = "", + .name = "playlist_track", + .columns = { ForeignKeyColumn("playlist_id"), ForeignKeyColumn("track_id") }, + .foreignKeys = { ForeignKey("playlist_track", "playlist_id", "playlist"), + ForeignKey("playlist_track", "track_id", "track") }, + .primaryKeys = { "playlist_id", "track_id" } }, + }; + + auto const plan = CxxModelPrinter::PlanRelations(tables); + + CHECK(RelationsOn(plan, "playlist").empty()); + CHECK(RelationsOn(plan, "track").empty()); + CHECK(RelationsOn(plan, "playlist_track").empty()); +} + 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 @@ -324,10 +356,10 @@ TEST_CASE("PlanRelations: a table with two foreign keys to the same table is not { .schema = "", .name = "person", .columns = { IdColumn() }, .primaryKeys = { "id" } }, { .schema = "", .name = "marriage", - .columns = { ForeignKeyColumn("spouse_a_id"), ForeignKeyColumn("spouse_b_id") }, + .columns = { IdColumn(), 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" } }, + .primaryKeys = { "id" } }, }; auto const relations = RelationsOn(CxxModelPrinter::PlanRelations(tables), "person"); @@ -457,10 +489,10 @@ TEST_CASE("CxxModelPrinter: emits HasManyThrough for a join table", "[CxxModelPr { .schema = "", .name = "user", .columns = { IdColumn() }, .primaryKeys = { "id" } }, { .schema = "", .name = "project_user", - .columns = { ForeignKeyColumn("project_id"), ForeignKeyColumn("user_id") }, + .columns = { IdColumn(), ForeignKeyColumn("project_id"), ForeignKeyColumn("user_id") }, .foreignKeys = { ForeignKey("project_user", "project_id", "project"), ForeignKey("project_user", "user_id", "user") }, - .primaryKeys = { "project_id", "user_id" } }, + .primaryKeys = { "id" } }, }; auto printer = CxxModelPrinter { CxxModelPrinter::Config {} }; diff --git a/src/tests/DataMapper/DescriptorRelationTests.cpp b/src/tests/DataMapper/DescriptorRelationTests.cpp new file mode 100644 index 000000000..0c70cf4b3 --- /dev/null +++ b/src/tests/DataMapper/DescriptorRelationTests.cpp @@ -0,0 +1,133 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// Regression coverage for issue #556: a record carrying an explicit `Lightweight::Description<>` +// specialization — as emitted by ddl2cpp — must still auto-load its relations. +// +// `RecordMemberCount` prefers `Description::FieldCount` over reflection whenever a +// specialization exists, so a descriptor that lists only the columns hides the relation members +// from `EnumerateRecordMembers`. `ConfigureRelationAutoLoading` then installs no loader and the +// first access throws `SqlRequireLoadedError`. No test instantiated that combination before, which +// is why CI stayed green while every ddl2cpp-generated model had the defect. + +#include "../Utils.hpp" + +#include + +#include + +#include +#include +#include +#include +#include + +using namespace Lightweight; + +struct DescribedArtist; +struct DescribedAlbum; + +// Mirrors what ddl2cpp emits: columns first, then the relation members, with an explicit descriptor +// specialization below. +struct DescribedArtist +{ + Field artistId {}; + Field, SqlRealName { "Name" }> name {}; + + HasMany albums {}; +}; + +struct DescribedAlbum +{ + Field albumId {}; + Field, SqlRealName { "Title" }> title {}; + BelongsTo artist {}; +}; + +template <> +struct Lightweight::Description +{ + static constexpr std::size_t FieldCount = 3; + using Members = + Lightweight::RecordMemberList<&DescribedArtist::artistId, &DescribedArtist::name, &DescribedArtist::albums>; + static constexpr std::array FieldNames = { "ArtistId", "Name", "albums" }; +}; + +template <> +struct Lightweight::Description +{ + static constexpr std::size_t FieldCount = 3; + using Members = Lightweight::RecordMemberList<&DescribedAlbum::albumId, &DescribedAlbum::title, &DescribedAlbum::artist>; + static constexpr std::array FieldNames = { "AlbumId", "Title", "ArtistId" }; +}; + +// The descriptor enumerates every member, but only the members that map onto a result-set column +// contribute to a projection - listing the relation must not widen the record's column count. +static_assert(RecordMemberCount == 3); +static_assert(RecordColumnCount == 2); +static_assert(RecordStorageFieldCount == 2); + +TEST_CASE_METHOD(SqlTestFixture, "Description-carrying record auto-loads its HasMany", "[DataMapper][relations][issue556]") +{ + auto dm = DataMapper(); + dm.CreateTables(); + + auto artist = DescribedArtist { .name = "AC/DC" }; + dm.Create(artist); + + // Create(), not CreateExplicit(): an integer AutoAssign key is computed on insert, so two + // explicit inserts would both land on the same primary key. + auto album1 = DescribedAlbum { .title = "Let There Be Rock", .artist = artist }; + dm.Create(album1); + auto album2 = DescribedAlbum { .title = "Powerage", .artist = artist }; + dm.Create(album2); + + SECTION("QuerySingle configures the loader") + { + auto const queried = dm.QuerySingle(artist.artistId); + REQUIRE(queried.has_value()); + // Unreachable after REQUIRE, but it is what makes the access below provably checked: the + // optional-access analysis does not model Catch2's assertion macros. + if (!queried.has_value()) + return; + auto const& loaded = *queried; + + auto titles = std::set {}; + for (auto const& album: loaded.albums.All()) + titles.emplace(album->title.Value()); + + CHECK(titles == std::set { "Let There Be Rock", "Powerage" }); + CHECK(loaded.albums.Count() == 2); + } + + SECTION("Query with loadRelations configures the loader") + { + auto const queried = dm.Query() + .Where(FieldNameOf, "=", "AC/DC") + .First(); + REQUIRE(queried.has_value()); + if (!queried.has_value()) + return; + + CHECK(queried->albums.Count() == 2); + } +} + +TEST_CASE_METHOD(SqlTestFixture, + "Description-carrying record still projects only its columns", + "[DataMapper][relations][issue556]") +{ + // The descriptor now lists a member that carries no storage; CREATE TABLE and the SELECT + // projection must keep skipping it. + auto dm = DataMapper(); + dm.CreateTables(); + + for (auto const& statement: dm.CreateTableString(dm.Connection().ServerType())) + CHECK(!statement.contains("albums")); + + auto artist = DescribedArtist { .name = "Aerosmith" }; + dm.Create(artist); + + auto const all = dm.Query().All(); + REQUIRE(all.size() == 1); + CHECK(all.front().name.Value() == "Aerosmith"); +} From b821fbcb327397c7d78764dfd3df03759ccb327e Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Wed, 19 Aug 2026 19:00:59 +0300 Subject: [PATCH 2/2] review: plan inverse relations only where the BelongsTo is actually emitted Applying /code-review findings on this branch. The new inverse-relation guard covered only one of the reasons PrintTable declines to emit a BelongsTo -- an FK column that is also a primary key. A non-declarable self-reference still had a HasMany planned for it. That was harmless before this PR, because the relation was inert; now that Description<> lists relation members, ConfigureRelationAutoLoading instantiates the loader, InverseBelongsToResolver static_asserts, and the generated header stops compiling. AsJoinTable had the same gap. Two such shapes are already pinned by existing column-side tests, so this is reachable from real schemas. Extract a single IsEmittedAsBelongsTo(table, constraint) predicate that mirrors the column-emission guard exactly -- composite FK, FK column that is also a primary key, and self-reference whose target is not a primary key or not declared before it -- and route both AsJoinTable and EmitInverseRelation through it. This also removes the two ad-hoc primaryKeys lookups. Extend rule 6 of the relation-generation doc to list all three cases rather than only the composite-key join table, and add regression tests for the two shapes. Chinook output is unchanged: Employee.ReportsTo -> Employee.EmployeeId is declarable, so its HasMany is still planned. Co-Authored-By: Claude Opus 5 (1M context) --- docs/ddl2cpp-relation-generation.md | 26 ++++---- src/Lightweight/Tools/CxxModelPrinter.cpp | 73 ++++++++++++++++------- src/tests/CxxModelRelationTests.cpp | 38 ++++++++++++ 3 files changed, 107 insertions(+), 30 deletions(-) diff --git a/docs/ddl2cpp-relation-generation.md b/docs/ddl2cpp-relation-generation.md index 64b7bf848..d00311592 100644 --- a/docs/ddl2cpp-relation-generation.md +++ b/docs/ddl2cpp-relation-generation.md @@ -129,16 +129,22 @@ considered throughout; composite ones are counted and skipped as before. 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. -6. **No relation at all** when the child's foreign key column is also part of that child's own - primary key. Such a column is emitted as a plain `Field`, never a `BelongsTo` (see rule 1), and - every inverse relation — `HasMany`, `HasOne` and both through-relations — resolves its other end - through exactly that `BelongsTo`. Generating one anyway produces a record that does not compile. - This rules out the classic composite-key join table, whose primary key *is* its two foreign keys; - a join table carrying a key of its own is unaffected and still yields rule 2 or 3. - - Supporting the composite-key shape needs `BelongsTo` to be usable as a primary key, which it is - not today (`BelongsTo::IsPrimaryKey` is hard-coded `false`). Until that changes, the generator - emits nothing there rather than something that cannot be built. +6. **No relation at all** whenever the child's foreign key column does *not* become a `BelongsTo` + (see rule 1). Every inverse relation — `HasMany`, `HasOne` and both through-relations — resolves + its other end through exactly that `BelongsTo`, so generating one anyway produces a record that + does not compile. The column stays a plain `Field`, and hence no relation is emitted, when: + + - it is also part of the child's own primary key. This rules out the classic composite-key join + table, whose primary key *is* its two foreign keys; a join table carrying a key of its own is + unaffected and still yields rule 2 or 3. Supporting that shape needs `BelongsTo` to be usable + as a primary key, which it is not today (`BelongsTo::IsPrimaryKey` is hard-coded `false`). + - the foreign key is composite (already true before, and unchanged). + - it is a *self*-reference that cannot name its target: a pointer-to-member may only name a + member the compiler has already seen, so a self-referencing foreign key declared before the + primary key it points at — or one pointing at a merely `UNIQUE` column rather than the primary + key — falls back to a plain `Field` too. + + In all of these the generator emits nothing rather than something that cannot be built. 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 diff --git a/src/Lightweight/Tools/CxxModelPrinter.cpp b/src/Lightweight/Tools/CxxModelPrinter.cpp index b9f10c255..bb055d961 100644 --- a/src/Lightweight/Tools/CxxModelPrinter.cpp +++ b/src/Lightweight/Tools/CxxModelPrinter.cpp @@ -420,6 +420,46 @@ namespace }); } + /// Whether the child side of @p constraint is emitted as a `BelongsTo` member rather than a plain + /// `Field`. Mirrors - and must stay in sync with - the guard at the column emission site in + /// `PrintTable`. + /// + /// Every relation this planner can emit (`HasMany`, `HasOne`, `HasOneThrough`, `HasManyThrough`) + /// resolves its other end through exactly that `BelongsTo`, so planning one for a foreign key that + /// does not become one produces a record that does not compile: `InverseBelongsToResolver` + /// static_asserts on the missing member as soon as `ConfigureRelationAutoLoading` reaches the + /// relation - which, since the generated `Description<>` lists relation members, it now does (#556). + /// + /// @param table The table declaring @p constraint. + /// @param constraint The candidate foreign key. + /// @return `true` when the foreign key column becomes a `BelongsTo` member. + bool IsEmittedAsBelongsTo(SqlSchema::Table const& table, SqlSchema::ForeignKeyConstraint const& constraint) + { + // Composite: emitted as plain fields (with a warning), so there is no BelongsTo either. + if (!IsSingleColumn(constraint)) + return false; + + // A column that is both a primary key and a foreign key stays a plain Field - `BelongsTo` cannot + // be a primary key today (`BelongsTo::IsPrimaryKey` is hard-coded `false`). + auto const& childColumn = constraint.foreignKey.columns.front(); + if (std::ranges::contains(table.primaryKeys, childColumn)) + return false; + + if (constraint.primaryKey.table != constraint.foreignKey.table) + return true; + + // A self-reference names a member of the very struct being defined, so it is declarable only + // when it points at a primary key column emitted *before* it. Otherwise the column falls back + // to a plain field as well. + auto const referenced = + std::ranges::find(table.columns, constraint.primaryKey.columns.front(), &SqlSchema::Column::name); + if (referenced == table.columns.end() || !referenced->isPrimaryKey) + return false; + + auto const child = std::ranges::find(table.columns, childColumn, &SqlSchema::Column::name); + return child != table.columns.end() && referenced < child; + } + /// 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. /// @@ -453,16 +493,12 @@ namespace if (!std::ranges::all_of(table.columns, [&](auto const& c) { return isKeyColumn(c.name); })) return std::nullopt; - // A column that is both a primary key and a foreign key is emitted as a plain Field, never a - // BelongsTo (see the `isForeignKey && !isPrimaryKey` guard at the column emission site). A - // through-relation resolves its join record's owner and far sides through exactly those BelongsTo - // members, so a join table keyed on its own foreign keys cannot satisfy the relation it would - // otherwise imply - the generated code would not compile. Skip it for the same reason - // EmitInverseRelation skips composite foreign keys: no BelongsTo, no relation (#556). - auto const keyedOnItsOwnForeignKey = [&](SqlSchema::ForeignKeyConstraint const& constraint) { - return std::ranges::contains(table.primaryKeys, constraint.foreignKey.columns.front()); - }; - if (keyedOnItsOwnForeignKey(singleColumnKeys[0]) || keyedOnItsOwnForeignKey(singleColumnKeys[1])) + // A through-relation resolves its join record's owner and far sides through exactly the two + // BelongsTo members, so a join table whose foreign keys do not become BelongsTo cannot satisfy + // the relation it would otherwise imply - the generated code would not compile. The classic + // composite-key join table (its primary key *is* its two foreign keys) is the common case here. + // Skip it for the same reason EmitInverseRelation does: no BelongsTo, no relation (#556). + if (!IsEmittedAsBelongsTo(table, singleColumnKeys[0]) || !IsEmittedAsBelongsTo(table, singleColumnKeys[1])) return std::nullopt; return std::pair { singleColumnKeys[0], singleColumnKeys[1] }; @@ -509,7 +545,7 @@ namespace } /// Emits the inverse `HasOne`/`HasMany` relation implied by one single-column foreign key, unless it - /// is composite or points outside the generated set. + /// does not become a `BelongsTo` (see @ref IsEmittedAsBelongsTo) or points outside the generated set. /// /// @param table The table declaring @p constraint. /// @param constraint The candidate foreign key. @@ -523,8 +559,12 @@ namespace IsAmbiguousFn const& isAmbiguous, CxxModelPrinter::RelationPlan& plan) { - if (!IsSingleColumn(constraint)) - return; // composite: no BelongsTo either, so no inverse + // HasMany/HasOne resolves its inverse through the child's BelongsTo member. Where the column + // emission site declines to emit one - a composite key, a column that is also a primary key, a + // self-reference that cannot name its target - the relation cannot be satisfied and the + // generated record fails to compile (#556). + if (!IsEmittedAsBelongsTo(table, constraint)) + return; auto const& ownerTable = constraint.primaryKey.table.table; if (byName(ownerTable) == nullptr) @@ -532,13 +572,6 @@ namespace auto const& childColumn = constraint.foreignKey.columns.front(); - // Same reason as the composite case: a column that is both a primary key and a foreign key is - // emitted as a plain Field, never a BelongsTo (see the `isForeignKey && !isPrimaryKey` guard at - // the column emission site). HasMany resolves its inverse through that BelongsTo, so without one - // the relation cannot be satisfied and the generated record fails to compile (#556). - if (std::ranges::contains(table.primaryKeys, childColumn)) - return; - // Scalar when the child's own foreign key is uniquely indexed: one child per owner. auto const childIsUnique = IsUniquelyIndexed(table, childColumn); diff --git a/src/tests/CxxModelRelationTests.cpp b/src/tests/CxxModelRelationTests.cpp index b9d2b37d7..e95d70117 100644 --- a/src/tests/CxxModelRelationTests.cpp +++ b/src/tests/CxxModelRelationTests.cpp @@ -315,6 +315,44 @@ TEST_CASE("PlanRelations: a join table keyed on its own foreign keys yields no t CHECK(RelationsOn(plan, "playlist_track").empty()); } +TEST_CASE("PlanRelations: a self-reference declared before its primary key yields no relation", + "[CxxModelPrinter][relations]") +{ + // The same shape CxxModelPrinterTests pins on the column side ("self-reference declared before its + // primary key stays a plain field"): a pointer-to-member may only name a member the compiler has + // already seen, so this foreign key falls back to a plain Field instead of a BelongsTo. HasMany + // resolves its inverse through exactly that BelongsTo, so planning one would emit a record whose + // ConfigureRelationAutoLoading fails to compile - now that Description<> lists relation members and + // the loader is actually instantiated (#556). + auto const tables = std::vector { + { .schema = "", + .name = "node", + .columns = { ForeignKeyColumn("parent_id"), IdColumn() }, + .foreignKeys = { ForeignKey("node", "parent_id", "node") }, + .primaryKeys = { "id" } }, + }; + + CHECK(RelationsOn(CxxModelPrinter::PlanRelations(tables), "node").empty()); +} + +TEST_CASE("PlanRelations: a self-reference into a non-primary-key column yields no relation", "[CxxModelPrinter][relations]") +{ + // PostgreSQL and SQL Server allow a foreign key to target any UNIQUE NOT NULL column, but BelongsTo + // static_asserts that the member it points at is a primary key - so the column stays a plain Field + // and, for the same reason as above, implies no inverse relation either. + auto const tables = std::vector { + { .schema = "", + .name = "doc", + .columns = { IdColumn(), + Lightweight::SqlSchema::Column { .name = "code", .type = Integer {}, .isNullable = false }, + ForeignKeyColumn("parent_code") }, + .foreignKeys = { ForeignKey("doc", "parent_code", "doc", "code") }, + .primaryKeys = { "id" } }, + }; + + CHECK(RelationsOn(CxxModelPrinter::PlanRelations(tables), "doc").empty()); +} + 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