-
Notifications
You must be signed in to change notification settings - Fork 16
Orthogonal returning(ids) + strong-typed conflict policy tags for sqlgen::insert #123
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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())))); |
Copilot
AI
Feb 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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); |
Copilot
AI
Feb 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)); | ||
| } |
Copilot
AI
Feb 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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` |
Copilot
AI
Feb 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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