Orthogonal returning(ids) + strong-typed conflict policy tags for sqlgen::insert#123
Orthogonal returning(ids) + strong-typed conflict policy tags for sqlgen::insert#123Perdixky wants to merge 1 commit intogetml:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements Issue #109 by extending sqlgen::insert with orthogonal, composable modifiers: strong-typed conflict policy tags (or_replace, or_ignore) and returning(ids) to collect auto-generated IDs across supported backends. It updates statement transpilation, backend SQL generation, and connection insert execution paths to support returning IDs, plus adds extensive cross-backend tests and documentation updates.
Changes:
- Add typed insert modifiers:
or_replace,or_ignore, andreturning(ids)(pipe style and direct style). - Extend
dynamic::InsertwithConflictPolicyandreturningcolumns; update transpilation and all backendto_sqlimplementations accordingly. - Plumb returning-id collection through SQLite/Postgres/MySQL/DuckDB connection insert implementations and add new tests/docs.
Reviewed changes
Copilot reviewed 38 out of 38 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/sqlite/test_to_insert_returning.cpp | Adds dry SQL test for SQLite INSERT ... RETURNING. |
| tests/sqlite/test_to_insert_or_replace_tag.cpp | Adds dry SQL test for SQLite replace conflict policy tag. |
| tests/sqlite/test_to_insert_or_ignore.cpp | Adds dry SQL test for SQLite OR IGNORE. |
| tests/sqlite/test_insert_returning_ids.cpp | Adds runtime SQLite test validating collected returned IDs. |
| tests/sqlite/test_insert_or_replace.cpp | Migrates test to `insert(...) |
| tests/postgres/test_insert_returning_ids.cpp | Adds runtime Postgres test validating collected returned IDs. |
| tests/postgres/test_insert_returning_dry.cpp | Adds dry SQL test for Postgres RETURNING. |
| tests/postgres/test_insert_or_replace.cpp | Migrates test to new insert(..., or_replace) usage. |
| tests/postgres/test_insert_or_ignore_dry.cpp | Adds dry SQL test for Postgres ignore (ON CONFLICT DO NOTHING). |
| tests/mysql/test_insert_returning_ids.cpp | Adds runtime MySQL test for single-row returned ID collection. |
| tests/mysql/test_insert_returning_dry.cpp | Adds dry SQL test ensuring MySQL emits no RETURNING. |
| tests/mysql/test_insert_or_replace.cpp | Migrates test to `insert(...) |
| tests/mysql/test_insert_or_ignore_dry.cpp | Adds dry SQL test for MySQL INSERT IGNORE. |
| tests/duckdb/test_to_insert_returning.cpp | Adds dry SQL test for DuckDB RETURNING. |
| tests/duckdb/test_to_insert_or_ignore.cpp | Adds dry SQL test for DuckDB OR IGNORE. |
| tests/duckdb/test_insert_returning_ids.cpp | Adds runtime DuckDB test validating collected returned IDs. |
| tests/duckdb/test_insert_or_replace.cpp | Migrates test to `insert(...) |
| src/sqlgen/sqlite/to_sql.cpp | Implements ignore prefix + returning clause support for SQLite inserts. |
| src/sqlgen/sqlite/Connection.cpp | Adds optional returned-ids capture for INSERT ... RETURNING. |
| src/sqlgen/postgres/to_sql.cpp | Updates upsert generation for replace/ignore + returning clause support. |
| src/sqlgen/postgres/Connection.cpp | Adds optional returned-ids capture from INSERT ... RETURNING results. |
| src/sqlgen/mysql/to_sql.cpp | Implements INSERT IGNORE and updates replace handling to new policy enum. |
| src/sqlgen/mysql/Connection.cpp | Adds returning-id collection via mysql_insert_id (single-row only). |
| src/sqlgen/duckdb/to_sql.cpp | Adds returning clause support and ignore/replace policy handling. |
| include/sqlgen/transpilation/value_t.hpp | Switches to std::ranges::range_value_t for range value extraction. |
| include/sqlgen/transpilation/to_sql.hpp | Updates ToSQL specialization for new Insert<..., ConflictPolicy, IDsType>. |
| include/sqlgen/transpilation/to_insert_or_write.hpp | Transpiles new conflict policy + returning columns into dynamic::Insert. |
| include/sqlgen/sqlite/Connection.hpp | Adds returning capability flags + optional returned-ids parameter to insert. |
| include/sqlgen/postgres/Connection.hpp | Adds returning capability flags + optional returned-ids parameter; formatting fixes. |
| include/sqlgen/mysql/Connection.hpp | Adds returning capability flags + optional returned-ids parameter. |
| include/sqlgen/internal/has_auto_incr_primary_key.hpp | Adds trait to detect auto-incrementing primary keys at compile time. |
| include/sqlgen/insert.hpp | Implements modifiers parsing/validation, returning(ids), and deprecates insert_or_replace. |
| include/sqlgen/dynamic/Insert.hpp | Replaces or_replace bool with ConflictPolicy enum; adds returning columns. |
| include/sqlgen/duckdb/Connection.hpp | Adds returning capability flags and implements returning-id insert path for DuckDB. |
| include/sqlgen/Transaction.hpp | Plumbs returned-ids capability/parameter through transaction wrapper. |
| include/sqlgen/Session.hpp | Plumbs returned-ids capability/parameter through session wrapper. |
| docs/insert.md | Updates docs for conflict policy modifiers and returning(ids) behavior/rules. |
| docs/README.md | Updates docs index entry to include sqlgen::returning. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _returned_ids->emplace_back(std::to_string( | ||
| static_cast<unsigned long long>(mysql_insert_id(conn_.get())))); |
There was a problem hiding this comment.
When returning(ids) is used together with or_replace (MySQL ON DUPLICATE KEY UPDATE), mysql_insert_id() returns 0 if the statement performed an UPDATE rather than an INSERT. This will populate ids with incorrect values on conflicts. Consider adjusting the generated SQL to use the LAST_INSERT_ID(pk) trick in the UPDATE clause when returning is enabled, or disallow/clearly document returning(ids) with or_replace on MySQL.
| _returned_ids->emplace_back(std::to_string( | |
| static_cast<unsigned long long>(mysql_insert_id(conn_.get())))); | |
| const auto insert_id = | |
| static_cast<unsigned long long>(mysql_insert_id(conn_.get())); | |
| if (insert_id != 0) { | |
| _returned_ids->emplace_back(std::to_string(insert_id)); | |
| } else { | |
| // No new auto-increment ID was generated (e.g., UPDATE in | |
| // INSERT ... ON DUPLICATE KEY UPDATE). Represent this as nullopt | |
| // rather than an incorrect "0" ID. | |
| _returned_ids->emplace_back(std::nullopt); | |
| } |
There was a problem hiding this comment.
That's right, I'm going to fix it.
| std::ranges::range<T> && requires(T& t, typename T::value_type v) { | ||
| t.clear(); | ||
| t.push_back(v); |
There was a problem hiding this comment.
OutputIDRange uses typename T::value_type v inside the requires-clause, which (1) assumes the range has a nested value_type typedef (not required by std::ranges::range) and (2) requires it to be default-constructible. Use std::ranges::range_value_t<T> together with std::declval (or a placeholder parameter) so the concept matches the documented requirements (clear + push_back) without over-constraining container types.
| std::ranges::range<T> && requires(T& t, typename T::value_type v) { | |
| t.clear(); | |
| t.push_back(v); | |
| std::ranges::range<T> && | |
| requires(T& t) { | |
| t.clear(); | |
| t.push_back(std::declval<std::ranges::range_value_t<T>>()); |
| if (_conflict_policy == dynamic::Insert::ConflictPolicy::replace) { | ||
| result.constraints = sqlgen::internal::collect::vector( | ||
| columns | filter(is_constraint) | transform(get_name)); | ||
| full_columns | filter(is_constraint) | transform(get_name)); | ||
| } |
There was a problem hiding this comment.
For or_replace, constraints are currently collected as all primary+unique columns and later used as the conflict target (e.g., ON CONFLICT (col1, col2)). This can generate SQL that doesn't match any real unique/PK constraint when the table has more than one independent constraint (common case: auto-increment PK + a separate UNIQUE column), causing runtime failures in SQLite/Postgres. Consider selecting a single conflict target (e.g., prefer the primary key, else a single unique constraint) or requiring the user to specify the conflict target when multiple constraints exist.
| Behavior by backend: | ||
|
|
||
| - The table type must have a primary key or at least one unique constraint. This is enforced at compile time via a static_assert: | ||
| - SQLite: `OR REPLACE`, `OR IGNORE` |
There was a problem hiding this comment.
The SQLite backend description says or_replace emits OR REPLACE, but the current SQLite SQL generator for ConflictPolicy::replace emits ON CONFLICT (...) DO UPDATE ... (and only uses OR IGNORE for ignore). Please update this section to match the actual generated SQL/semantics to avoid misleading users.
| - SQLite: `OR REPLACE`, `OR IGNORE` | |
| - SQLite: `ON CONFLICT (...) DO UPDATE ...`, `OR IGNORE` |
#109