diff --git a/docs/ddl2cpp-relation-generation.md b/docs/ddl2cpp-relation-generation.md index 610ba5b5b..d00311592 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,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** 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/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..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,6 +493,14 @@ namespace if (!std::ranges::all_of(table.columns, [&](auto const& c) { return isKeyColumn(c.name); })) return std::nullopt; + // 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] }; } @@ -497,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. @@ -511,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) @@ -1083,6 +1135,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..e95d70117 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,75 @@ 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 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 @@ -324,10 +394,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 +527,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"); +}