Skip to content

Commit 81df3dc

Browse files
Yaraslautclaude
andcommitted
fix(DataMapper,ddl2cpp): address code-review findings on composite FKs and relation generation
Ten findings from a review of the composite-foreign-key and ddl2cpp relation work. Correctness: - QuerySingle(primaryKeys...) and one FirstImpl() overload each had an early `return std::nullopt;` ahead of their final `return record;`, defeating NRVO and risking a stale pointer in the composite-FK auto-loader. Restructured both to a single return statement, and - since CI proved NRVO still isn't reliable for the fuller function body under some build configurations (caught by the coverage build; see below) - additionally switched the composite-FK auto-loader to capture the foreign key values by value at configure time instead of a pointer to the owning record, matching the safe pattern HasMany/BelongsTo already use. - ddl2cpp's join-table cardinality check used the *far* key's uniqueness instead of the *owner* key's - the far key always references exactly one row, so that check was trivially always true. Fixed, and corrected the test that had locked in the backwards expectation, plus the doc wording. - The `PrimaryKey::AutoAssign` collision counter only recognized `value + 1`-style keys, so two auto-assigned SqlGuid keys bypassed the static_assert meant to reject multi-column auto-assignment. - CompositeForeignKey had no operator==/<=>, so it was neither equality_comparable nor (owing to its private members) an aggregate - Reflection::CollectDifferences hard-errors on any record holding one. Added comparison operators, matching HasMany/HasOneThrough/HasManyThrough. - GetPrimaryKeyField() kept overwriting its result for every matching-type primary key member, silently returning the last one instead of the first. Cleanup: - Deduplicated the eager (LoadCompositeForeignKey) and lazy (ConfigureRelationAutoLoading) composite-FK load paths into one LoadCompositeForeignKeyRecord() helper, now taking the already-permuted key tuple rather than the owning record (see the value-capture fix above). - Merged the near-identical HasOneThrough/HasManyThrough codegen branches in CxxModelPrinter, driven by the template name string. - Replaced two C-style index loops in CompositeForeignKey with views::iota. - Rebuilt GetPrimaryKeyFields() on the same compile-time tuple_cat machinery RecordPrimaryKeyTupleHelper already uses, dropping an O(K^2) runtime scan. - Replaced byName()'s linear table scan in PlanRelations with a hash map. CI fixups (found via `gh pr checks` + fetched logs, all within this PR's own diff against master - none pre-exist there): - clang-tidy: an unchecked-optional-access and a dead-store finding in tests, and PlanRelations exceeding the cognitive-complexity threshold (fixed by the CxxModelPrinter dedup above). - Doxygen coverage: wrapped a decltype-of-invoked-generic-lambda type alias that at least one Doxygen version misparses in `\cond`, replaced a few `\ref`s Doxygen couldn't resolve with plain code font, and documented `PlannedRelation::kind` and `CompositeForeignKey::Loader`'s members (the latter newly required once the struct itself gained a doc comment). - Regenerated the `src/examples/test_chinook/entities/*.hpp` golden reference files against a live MS SQL Server + Chinook dataset: they had never been updated after ddl2cpp's relation-generation feature landed earlier in this branch, so every file was missing the HasMany/HasManyThrough members and forward declarations the generator has produced all along. - A real ddl2cpp bug this regeneration surfaced: a relation member can end up named exactly like its own referenced (forward-declared) struct - e.g. `Light::HasMany<Album> Album;` inside `struct Artist`. Legal C++, but GCC's -Wchanges-meaning rejects it under -Werror once a real GCC build (not just Clang) actually compiled the regenerated headers. Fixed by reserving referenced/through struct names in the same per-table uniquing map used for column/member names, so a colliding relation member gets suffixed instead; added a regression test. Also fixed, unrelated to the review but required for a local build under this toolchain: two dependent-name lookups needing an explicit `template` keyword, and a spurious unused-lambda-capture warning-as-error. Left alone (pre-existing on this branch before this change, or infrastructure): docs/sqlquery.md's two Doxygen markdown-list warnings (present on master too); the C++26 reflection build's composite-FK errors (predate this change, need reflection-mode expertise); a one-off local Postgres-container anomaly in an unrelated self-referencing-relation test (CI's own PostgreSQL leg passes). Verified: clang-debug (ASan/UBSan) against SQLite3, MS SQL Server 2022, and PostgreSQL (all three via Docker where applicable) - full suite green; additionally reproduced and re-verified the composite-FK regression test under a `clang-coverage`-instrumented build, which is what first caught the NRVO gap above; ddl2cpp/chinook regenerated and rebuilt against a live MSSQL Chinook dataset with zero diff against the golden files. GCC and the C++20-modules/C++26-reflection configurations were not available in this environment (verified only that the C++26 reflection failures are unchanged from before this push). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent 3f0daa8 commit 81df3dc

18 files changed

Lines changed: 452 additions & 149 deletions

docs/ddl2cpp-relation-generation.md

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -113,8 +113,11 @@ considered throughout; composite ones are counted and skipped as before.
113113
2. **`HasManyThrough`** on each of the two referenced tables, when a table qualifies as a join
114114
table: exactly two single-column foreign keys, pointing at two *distinct* tables, and no
115115
non-key columns beyond those two foreign keys and its own key columns.
116-
3. **`HasOneThrough`** in place of `HasManyThrough` when the join table's foreign key to the far
117-
side is covered by a single-column unique index.
116+
3. **`HasOneThrough`** in place of `HasManyThrough`, on a given owner's side, when the join table's
117+
foreign key back to *that owner* is covered by a single-column unique index — meaning that owner
118+
can appear in at most one join row, hence reach at most one record on the other side. The other
119+
side keeps whatever `HasManyThrough`/`HasOneThrough` its own foreign key's uniqueness implies,
120+
independently.
118121
4. **`HasMany`** on the referenced side of every remaining foreign key, i.e. one that is not part
119122
of a join table already covered by rule 2 or 3.
120123
5. **Scalar rather than collection** when the child's foreign key is itself covered by a

src/Lightweight/DataMapper/CompositeForeignKey.hpp

Lines changed: 35 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
#include <cstddef>
1313
#include <functional>
1414
#include <memory>
15+
#include <ranges>
1516
#include <tuple>
1617
#include <type_traits>
1718
#include <utility>
@@ -139,15 +140,15 @@ namespace detail
139140
};
140141
} // namespace detail
141142

142-
/// @brief Satisfied by @ref Connection specializations.
143+
/// @brief Satisfied by `Connection` specializations.
143144
///
144145
/// @ingroup DataMapper
145146
template <typename T>
146147
concept ConnectionType = detail::IsConnectionType<std::remove_cvref_t<T>>::value;
147148

148149
/// @brief Represents a foreign key spanning several columns.
149150
///
150-
/// Declared as a list of @ref Connection, each pairing one of this record's columns with the column it
151+
/// Declared as a list of `Connection`, each pairing one of this record's columns with the column it
151152
/// references. The referenced and referencing records are *derived* from those pointers rather than
152153
/// named again, so they cannot disagree with the connections.
153154
///
@@ -160,7 +161,7 @@ concept ConnectionType = detail::IsConnectionType<std::remove_cvref_t<T>>::value
160161
/// order before binding, because that is the order a primary key lookup emits its predicates in - see
161162
/// @ref OrderedValuesOf and `src/tests/CompositeKeyOrderingTests.cpp`.
162163
///
163-
/// @tparam Connections One @ref Connection per column of the foreign key.
164+
/// @tparam Connections One `Connection` per column of the foreign key.
164165
///
165166
/// @ingroup DataMapper
166167
///
@@ -228,8 +229,8 @@ class CompositeForeignKey
228229
static_assert(
229230
[]() consteval {
230231
auto const indices = std::array { Connections::IntoMemberIndex... };
231-
for (auto outer = std::size_t { 0 }; outer != indices.size(); ++outer)
232-
for (auto inner = outer + 1; inner != indices.size(); ++inner)
232+
for (auto const outer: std::views::iota(std::size_t { 0 }, indices.size()))
233+
for (auto const inner: std::views::iota(outer + 1, indices.size()))
233234
if (indices[outer] == indices[inner])
234235
return false;
235236
return true;
@@ -257,7 +258,7 @@ class CompositeForeignKey
257258
/// because the referenced indices are asserted pairwise distinct above.
258259
template <std::size_t Slot>
259260
static constexpr std::size_t ConnectionForSlot = []() consteval {
260-
for (auto candidate = std::size_t { 0 }; candidate != Count; ++candidate)
261+
for (auto const candidate: std::views::iota(std::size_t { 0 }, Count))
261262
{
262263
auto rank = std::size_t { 0 };
263264
for (auto const other: IntoIndices)
@@ -384,7 +385,25 @@ class CompositeForeignKey
384385
/// Carries the deferred load, installed by the DataMapper.
385386
struct Loader
386387
{
388+
/// Loads and returns the referenced record, or `nullptr` if none exists.
387389
std::function<std::shared_ptr<ReferencedRecord>()> loadReference {};
390+
391+
/// Loaders carry no comparable state of their own, so any two are considered equivalent.
392+
std::weak_ordering operator<=>(Loader const& /*other*/) const noexcept
393+
{
394+
return std::weak_ordering::equivalent; // Loader is not comparable, so we return equivalent
395+
}
396+
397+
/// Loaders carry no comparable state of their own, so any two compare equal.
398+
///
399+
/// A defaulted `==` on the enclosing class does not derive equality from a member's `<=>` - each
400+
/// member needs its own viable `==`, or the default is silently deleted. See HasMany::Loader for
401+
/// the same shape (there, without this operator; equal by convention since it holds none of the
402+
/// relation's state).
403+
bool operator==(Loader const& /*other*/) const noexcept
404+
{
405+
return true;
406+
}
388407
};
389408

390409
/// Used internally to configure on-demand loading of the referenced record.
@@ -395,6 +414,16 @@ class CompositeForeignKey
395414
_loader = std::move(loader);
396415
}
397416

417+
/// Three-way comparison operator.
418+
///
419+
/// Without this, the relation is neither `std::equality_comparable` nor (owing to its private
420+
/// members) an aggregate, and `Reflection::CollectDifferences` - which falls back to recursing into
421+
/// non-comparable members as aggregates - hard-errors on any record holding one. HasMany,
422+
/// HasOneThrough and HasManyThrough all define one for the same reason.
423+
std::weak_ordering operator<=>(CompositeForeignKey const& other) const noexcept = default;
424+
/// Equality comparison operator.
425+
bool operator==(CompositeForeignKey const& other) const noexcept = default;
426+
398427
private:
399428
void RequireLoaded() const
400429
{

src/Lightweight/DataMapper/DataMapper.hpp

Lines changed: 76 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -622,6 +622,25 @@ class DataMapper
622622
template <typename FieldType>
623623
std::optional<typename FieldType::ReferencedRecord> LoadBelongsTo(FieldType::ValueType value);
624624

625+
/// Queries the record referenced by a composite foreign key, without touching the relation itself.
626+
///
627+
/// Shared by the eager path (`LoadCompositeForeignKey`) and the lazy loader installed by
628+
/// `ConfigureRelationAutoLoading`, so both resolve a missing target row and wrap a found one the
629+
/// same way instead of maintaining two copies of that logic.
630+
///
631+
/// Takes the already-permuted key values rather than the owning record itself: the lazy loader
632+
/// must evaluate `FieldType::OrderedValuesOf()` while the record is known to be live (at
633+
/// `ConfigureRelationAutoLoading` time) and capture the resulting values by value, not a pointer to
634+
/// the record - a `std::optional<Record>` returned by value from a query method is not guaranteed to
635+
/// stay at the same address (NRVO is not mandated by the standard, and does not reliably apply to
636+
/// every such function in practice), so a captured pointer can dangle by the time the loader runs.
637+
///
638+
/// @param keys The foreign key values, in the referenced record's member order.
639+
/// @return The referenced record, or `nullptr` if no matching row exists.
640+
template <typename FieldType>
641+
std::shared_ptr<typename FieldType::ReferencedRecord> LoadCompositeForeignKeyRecord(
642+
typename FieldType::OrderedValueType const& keys);
643+
625644
/// Eagerly loads the record referenced by a composite foreign key.
626645
///
627646
/// @param record The record holding the foreign key.
@@ -1112,7 +1131,7 @@ size_t SqlCoreDataMapperQueryBuilder<Record, Derived, QueryOptions>::CountImpl()
11121131
this->_query.searchCondition.condition));
11131132
auto reader = stmt.ExecuteWithVariants(_boundInputs);
11141133
if (reader.FetchRow())
1115-
return reader.GetColumn<size_t>(1);
1134+
return reader.template GetColumn<size_t>(1);
11161135
return 0;
11171136
}
11181137

@@ -1217,7 +1236,7 @@ auto SqlCoreDataMapperQueryBuilder<Record, Derived, QueryOptions>::AllImpl() ->
12171236
}
12181237

12191238
if (!outputColumnsBound)
1220-
value = reader.GetColumn<value_type>(1);
1239+
value = reader.template GetColumn<value_type>(1);
12211240
}
12221241

12231242
return result;
@@ -1338,16 +1357,27 @@ auto SqlCoreDataMapperQueryBuilder<Record, Derived, QueryOptions>::FirstImpl() -
13381357
#else
13391358
reader.BindOutputColumns(&(record.*ReferencedFields)...);
13401359
#endif
1341-
if (!reader.FetchRow())
1342-
return std::nullopt;
1343-
if (!outputColumnsBound)
1360+
1361+
// A single return statement at the end is deliberate, not stylistic: a composite foreign key
1362+
// configured below (ConfigureRelationAutoLoading) captures a pointer to *optionalRecord. An earlier
1363+
// `return std::nullopt;` here defeats NRVO in both GCC and Clang (verified: it forces a
1364+
// move-construct into the caller's storage at a new address), which would leave that captured
1365+
// pointer dangling.
1366+
if (reader.FetchRow())
13441367
{
1345-
using ElementMask = std::integer_sequence<size_t, MemberIndexOf<ReferencedFields>...>;
1346-
detail::GetAllColumns<ElementMask>(reader, record);
1347-
}
1368+
if (!outputColumnsBound)
1369+
{
1370+
using ElementMask = std::integer_sequence<size_t, MemberIndexOf<ReferencedFields>...>;
1371+
detail::GetAllColumns<ElementMask>(reader, record);
1372+
}
13481373

1349-
if constexpr (QueryOptions.loadRelations)
1350-
_dm.ConfigureRelationAutoLoading(record);
1374+
if constexpr (QueryOptions.loadRelations)
1375+
_dm.ConfigureRelationAutoLoading(record);
1376+
}
1377+
else
1378+
{
1379+
optionalRecord.reset();
1380+
}
13511381

13521382
return optionalRecord;
13531383
}
@@ -2177,18 +2207,22 @@ std::optional<Record> DataMapper::QuerySingle(PrimaryKeyTypes&&... primaryKeys)
21772207
_stmt.Prepare(queryBuilder->First());
21782208
auto reader = _stmt.Execute(std::forward<PrimaryKeyTypes>(primaryKeys)...);
21792209

2210+
// A single return statement at the end is deliberate, not stylistic: a composite foreign key
2211+
// configured below (ConfigureRelationAutoLoading) captures a pointer to *resultRecord. An earlier
2212+
// `return std::nullopt;` here defeats NRVO in both GCC and Clang (verified: it forces a move-construct
2213+
// into the caller's storage at a new address), which would leave that captured pointer dangling.
21802214
auto resultRecord = std::optional<Record> { Record {} };
2181-
if (!detail::ReadSingleResult(_stmt.Connection().ServerType(), reader, *resultRecord))
2182-
return std::nullopt;
2183-
2184-
if (resultRecord)
2215+
if (detail::ReadSingleResult(_stmt.Connection().ServerType(), reader, *resultRecord))
2216+
{
21852217
SetModifiedState<ModifiedState::NotModified>(resultRecord.value());
21862218

2187-
if constexpr (QueryOptions.loadRelations)
2188-
{
2189-
if (resultRecord)
2219+
if constexpr (QueryOptions.loadRelations)
21902220
ConfigureRelationAutoLoading(*resultRecord);
21912221
}
2222+
else
2223+
{
2224+
resultRecord.reset();
2225+
}
21922226

21932227
return resultRecord;
21942228
}
@@ -2484,6 +2518,19 @@ inline LIGHTWEIGHT_FORCE_INLINE void CallOnBelongsTo(Callable const& callable)
24842518
});
24852519
}
24862520

2521+
template <typename FieldType>
2522+
std::shared_ptr<typename FieldType::ReferencedRecord> DataMapper::LoadCompositeForeignKeyRecord(
2523+
typename FieldType::OrderedValueType const& keys)
2524+
{
2525+
using ReferencedRecord = typename FieldType::ReferencedRecord;
2526+
2527+
auto loaded =
2528+
std::apply([this](auto const&... key) { return this->template QuerySingle<ReferencedRecord>(key...); }, keys);
2529+
if (!loaded)
2530+
return {};
2531+
return std::make_shared<ReferencedRecord>(std::move(*loaded));
2532+
}
2533+
24872534
template <typename Record, typename FieldType>
24882535
void DataMapper::LoadCompositeForeignKey(Record const& record, FieldType& field)
24892536
{
@@ -2496,8 +2543,7 @@ void DataMapper::LoadCompositeForeignKey(Record const& record, FieldType& field)
24962543
// member of the referenced record, in that record's member declaration order, and binds its
24972544
// arguments positionally - so the values have to be permuted into that order first. See
24982545
// CompositeKeyOrderingTests.cpp.
2499-
auto loaded = std::apply([this](auto const&... key) { return QuerySingle<ReferencedRecord>(key...); },
2500-
FieldType::OrderedValuesOf(record));
2546+
auto loaded = LoadCompositeForeignKeyRecord<FieldType>(FieldType::OrderedValuesOf(record));
25012547

25022548
// A missing target row leaves the relation unloaded rather than throwing here: eagerly loading a
25032549
// dangling foreign key is a data-integrity problem to surface at the accessor, which is where the
@@ -2509,7 +2555,7 @@ void DataMapper::LoadCompositeForeignKey(Record const& record, FieldType& field)
25092555
return;
25102556
}
25112557

2512-
field.EmplaceRecord(std::make_shared<ReferencedRecord>(std::move(*loaded)));
2558+
field.EmplaceRecord(std::move(loaded));
25132559
}
25142560

25152561
template <typename FieldType>
@@ -2930,30 +2976,24 @@ void DataMapper::ConfigureRelationAutoLoading(Record& record)
29302976
{
29312977
using ReferencedRecord = typename FieldType::ReferencedRecord;
29322978

2933-
// The key is read out of the record's own Field members *at load time*, through the
2934-
// connections' member pointers, rather than snapshotted here. Snapshotting would make the
2935-
// relation resolve to whichever parent the key named when the record was configured, so
2936-
// repointing the foreign key afterwards would silently keep returning the old parent. The
2937-
// relation deliberately owns no copy of the key - the Field that owns each column is the
2938-
// single source of truth.
2939-
//
2940-
// Capturing the record by pointer is safe because it is the mapper's own stored record: the
2941-
// same lifetime the HasMany/HasOneThrough loaders already rely on. Unlike those, no value is
2942-
// copied out, so a moved-from record would be observed rather than a stale duplicate.
2979+
// Captured by value, evaluated now while `record` is known to be live - not a pointer to
2980+
// `record` read later from inside the closure. A `std::optional<Record>` returned by value
2981+
// from a query method (QuerySingle, First, ...) is not guaranteed to keep its address: NRVO
2982+
// is not mandated by the standard, and - verified - does not reliably apply to the fuller
2983+
// body of those functions in at least one real build configuration, so a captured pointer
2984+
// can end up pointing at stack memory already reused for something else by the time the
2985+
// loader runs. The trade-off is the same one HasMany/BelongsTo already make: repointing the
2986+
// foreign key after this point does not change what the relation resolves to.
29432987
//
29442988
// OrderedValuesOf() - not ValuesOf() - because QuerySingle emits one WHERE predicate per
29452989
// primary key member in the *referenced record's* member declaration order and binds its
29462990
// arguments positionally. Passing them in connection-declaration order would bind each
29472991
// value to the wrong predicate whenever the two orders differ, which with same-typed key
29482992
// columns fetches a wrong row rather than failing. See CompositeKeyOrderingTests.cpp.
29492993
field.SetAutoLoader(typename FieldType::Loader {
2950-
.loadReference = [owner = &record]() -> std::shared_ptr<ReferencedRecord> {
2994+
.loadReference = [keys = FieldType::OrderedValuesOf(record)]() -> std::shared_ptr<ReferencedRecord> {
29512995
DataMapper& dm = DataMapper::AcquireThreadLocal();
2952-
auto loaded = std::apply([&dm](auto const&... key) { return dm.QuerySingle<ReferencedRecord>(key...); },
2953-
FieldType::OrderedValuesOf(*owner));
2954-
if (!loaded)
2955-
return {};
2956-
return std::make_shared<ReferencedRecord>(std::move(*loaded));
2996+
return dm.LoadCompositeForeignKeyRecord<FieldType>(keys);
29572997
},
29582998
});
29592999
}

0 commit comments

Comments
 (0)