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;`. Verified with
  standalone repros that this defeats NRVO in both GCC and Clang, moving the
  record to a new address on return while the composite-FK lazy loader
  installed on it still held a pointer to the old one - a use-after-free.
  Restructured both to a single return statement.
- 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.
- 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.

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.

Verified: clang-debug (ASan/UBSan) against SQLite3 - 1341/1342 test cases,
13431/13431 assertions passed (1 pre-existing SQLite-unsupported skip).
GCC/MSSQL/PostgreSQL legs not available in this environment.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Author: Yaraslaut

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

src/Lightweight/DataMapper/CompositeForeignKey.hpp

@@ -12,6 +12,7 @@
 #include <cstddef>
 #include <functional>
 #include <memory>
+#include <ranges>
 #include <tuple>
 #include <type_traits>
 #include <utility>
@@ -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 <std::size_t Slot>
     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)
@@ -385,6 +386,20 @@ class CompositeForeignKey
     struct Loader
     {
         std::function<std::shared_ptr<ReferencedRecord>()> loadReference {};
+
+        std::weak_ordering operator<=>(Loader const& /*other*/) const noexcept
+        {
+            return std::weak_ordering::equivalent; // Loader is not comparable, so we return equivalent
+        }
+
+        // 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 +410,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
     {

src/Lightweight/DataMapper/DataMapper.hpp

@@ -622,6 +622,17 @@ class DataMapper
     template <typename FieldType>
     std::optional<typename FieldType::ReferencedRecord> LoadBelongsTo(FieldType::ValueType value);
 
+    /// Queries the record referenced by a composite foreign key, without touching the relation itself.
+    ///
+    /// Shared by the eager path (@ref LoadCompositeForeignKey) and the lazy loader installed by
+    /// @ref ConfigureRelationAutoLoading, so both resolve a missing target row and wrap a found one the
+    /// same way instead of maintaining two copies of that logic.
+    ///
+    /// @param owner The record holding the foreign key.
+    /// @return The referenced record, or `nullptr` if no matching row exists.
+    template <typename FieldType, typename Record>
+    std::shared_ptr<typename FieldType::ReferencedRecord> LoadCompositeForeignKeyRecord(Record const& owner);
+
     /// Eagerly loads the record referenced by a composite foreign key.
     ///
     /// @param record The record holding the foreign key.
@@ -1112,7 +1123,7 @@ size_t SqlCoreDataMapperQueryBuilder<Record, Derived, QueryOptions>::CountImpl()
                                         this->_query.searchCondition.condition));
     auto reader = stmt.ExecuteWithVariants(_boundInputs);
     if (reader.FetchRow())
-        return reader.GetColumn<size_t>(1);
+        return reader.template GetColumn<size_t>(1);
     return 0;
 }
 
@@ -1217,7 +1228,7 @@ auto SqlCoreDataMapperQueryBuilder<Record, Derived, QueryOptions>::AllImpl() ->
         }
 
         if (!outputColumnsBound)
-            value = reader.GetColumn<value_type>(1);
+            value = reader.template GetColumn<value_type>(1);
     }
 
     return result;
@@ -1338,16 +1349,27 @@ auto SqlCoreDataMapperQueryBuilder<Record, Derived, QueryOptions>::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<size_t, MemberIndexOf<ReferencedFields>...>;
-        detail::GetAllColumns<ElementMask>(reader, record);
-    }
+        if (!outputColumnsBound)
+        {
+            using ElementMask = std::integer_sequence<size_t, MemberIndexOf<ReferencedFields>...>;
+            detail::GetAllColumns<ElementMask>(reader, record);
+        }
 
-    if constexpr (QueryOptions.loadRelations)
-        _dm.ConfigureRelationAutoLoading(record);
+        if constexpr (QueryOptions.loadRelations)
+            _dm.ConfigureRelationAutoLoading(record);
+    }
+    else
+    {
+        optionalRecord.reset();
+    }
 
     return optionalRecord;
 }
@@ -2177,18 +2199,22 @@ std::optional<Record> DataMapper::QuerySingle(PrimaryKeyTypes&&... primaryKeys)
     _stmt.Prepare(queryBuilder->First());
     auto reader = _stmt.Execute(std::forward<PrimaryKeyTypes>(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> { Record {} };
-    if (!detail::ReadSingleResult(_stmt.Connection().ServerType(), reader, *resultRecord))
-        return std::nullopt;
-
-    if (resultRecord)
+    if (detail::ReadSingleResult(_stmt.Connection().ServerType(), reader, *resultRecord))
+    {
         SetModifiedState<ModifiedState::NotModified>(resultRecord.value());
 
-    if constexpr (QueryOptions.loadRelations)
-    {
-        if (resultRecord)
+        if constexpr (QueryOptions.loadRelations)
             ConfigureRelationAutoLoading(*resultRecord);
     }
+    else
+    {
+        resultRecord.reset();
+    }
 
     return resultRecord;
 }
@@ -2484,6 +2510,22 @@ inline LIGHTWEIGHT_FORCE_INLINE void CallOnBelongsTo(Callable const& callable)
     });
 }
 
+template <typename FieldType, typename Record>
+std::shared_ptr<typename FieldType::ReferencedRecord> DataMapper::LoadCompositeForeignKeyRecord(Record const& owner)
+{
+    using ReferencedRecord = typename FieldType::ReferencedRecord;
+
+    // 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 this->template QuerySingle<ReferencedRecord>(key...); },
+                             FieldType::OrderedValuesOf(owner));
+    if (!loaded)
+        return {};
+    return std::make_shared<ReferencedRecord>(std::move(*loaded));
+}
+
 template <typename Record, typename FieldType>
 void DataMapper::LoadCompositeForeignKey(Record const& record, FieldType& field)
 {
@@ -2492,12 +2534,7 @@ void DataMapper::LoadCompositeForeignKey(Record const& record, FieldType& field)
     ZoneScopedN("DataMapper::LoadCompositeForeignKey");
     ZoneTextObject(RecordTableName<ReferencedRecord>);
 
-    // 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<ReferencedRecord>(key...); },
-                             FieldType::OrderedValuesOf(record));
+    auto loaded = LoadCompositeForeignKeyRecord<FieldType>(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 +2546,7 @@ void DataMapper::LoadCompositeForeignKey(Record const& record, FieldType& field)
         return;
     }
 
-    field.EmplaceRecord(std::make_shared<ReferencedRecord>(std::move(*loaded)));
+    field.EmplaceRecord(std::move(loaded));
 }
 
 template <typename FieldType>
@@ -2949,11 +2986,7 @@ void DataMapper::ConfigureRelationAutoLoading(Record& record)
             field.SetAutoLoader(typename FieldType::Loader {
                 .loadReference = [owner = &record]() -> std::shared_ptr<ReferencedRecord> {
                     DataMapper& dm = DataMapper::AcquireThreadLocal();
-                    auto loaded = std::apply([&dm](auto const&... key) { return dm.QuerySingle<ReferencedRecord>(key...); },
-                                             FieldType::OrderedValuesOf(*owner));
-                    if (!loaded)
-                        return {};
-                    return std::make_shared<ReferencedRecord>(std::move(*loaded));
+                    return dm.LoadCompositeForeignKeyRecord<FieldType>(*owner);
                 },
             });
         }

src/Lightweight/DataMapper/Record.hpp

@@ -2,6 +2,7 @@
 
 #pragma once
 
+#include "../DataBinder/SqlGuid.hpp"
 #include "../Utils.hpp"
 #include "BelongsTo.hpp"
 #include "Field.hpp"
@@ -402,9 +403,14 @@ namespace detail
     template <typename ValueType>
     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 <typename ValueType>
+    concept AutoAssignableKeyValue = std::same_as<ValueType, SqlGuid> || IncrementableKeyValue<ValueType>;
+
     template <typename FieldType>
     concept GeneratesAutoAssignedKey = IsField<FieldType> && IsAutoAssignPrimaryKeyField<FieldType>::value
-                                       && IncrementableKeyValue<typename FieldType::ValueType>;
+                                       && AutoAssignableKeyValue<typename FieldType::ValueType>;
 
     template <typename Record>
     constexpr std::size_t AutoAssignPrimaryKeyFieldCount =
@@ -455,29 +461,24 @@ constexpr bool HasCompositePrimaryKey = RecordPrimaryKeyCount<Record> > 1;
 template <typename Record>
 [[nodiscard]] RecordPrimaryKeyTuple<Record> GetPrimaryKeyFields(Record const& record)
 {
-    auto result = RecordPrimaryKeyTuple<Record> {};
-    auto slot = std::size_t { 0 };
-    EnumerateRecordMembers(record, [&]<size_t I, typename FieldType>(FieldType const& field) {
-        if constexpr (IsField<FieldType>)
-            if constexpr (FieldType::IsPrimaryKey)
+    // Mirrors RecordPrimaryKeyTupleHelper's compile-time tuple_cat construction (one std::tuple<> or
+    // std::tuple<ValueType> 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 []<std::size_t... I>(Record const& record, std::index_sequence<I...>) {
+        return std::tuple_cat([&record]<std::size_t J>() {
+            using FieldType = RecordMemberTypeOf<J, Record>;
+            if constexpr (IsField<FieldType>)
             {
-                // 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::size_t... J>(std::index_sequence<J...>) {
-                    (
-                        [&] {
-                            if constexpr (std::assignable_from<std::tuple_element_t<J, RecordPrimaryKeyTuple<Record>>&,
-                                                               typename FieldType::ValueType const&>)
-                                if (J == slot)
-                                    std::get<J>(result) = field.Value();
-                        }(),
-                        ...);
-                }(std::make_index_sequence<RecordPrimaryKeyCount<Record>> {});
-                ++slot;
+                if constexpr (FieldType::IsPrimaryKey)
+                    return std::tuple<typename FieldType::ValueType> { GetRecordMemberAt<J>(record).Value() };
+                else
+                    return std::tuple<> {};
             }
-    });
-    return result;
+            else
+                return std::tuple<> {};
+        }.template operator()<I>()...);
+    }(record, std::make_index_sequence<RecordMemberCount<Record>> {});
 }
 
 /// Returns the first primary key field of the record.
@@ -490,13 +491,18 @@ inline LIGHTWEIGHT_FORCE_INLINE RecordPrimaryKeyType<Record> GetPrimaryKeyField(
     static_assert(HasPrimaryKey<Record>, "Record must have a primary key");
 
     auto result = RecordPrimaryKeyType<Record> {};
+    bool found = false;
     EnumerateRecordMembers(record, [&]<size_t I, typename FieldType>(FieldType const& field) {
         // std::same_as<typename FieldType::ValueType, RecordPrimaryKeyType<Record>>condition is for the case where there are
         // multiple primary keys, we want to return the first one
         if constexpr (IsField<FieldType>)
             if constexpr (IsPrimaryKey<FieldType>)
                 if constexpr (std::same_as<typename FieldType::ValueType, RecordPrimaryKeyType<Record>>)
-                    result = field.Value();
+                    if (!found)
+                    {
+                        result = field.Value();
+                        found = true;
+                    }
     });
     return result;
 }