Skip to content

Conversation

@hawkw
Copy link
Member

@hawkw hawkw commented Nov 10, 2025

Depends on #9335.

In this comment on PR #9335, @smklein suggested using the raw_query_builder API for writing CTEs, rather than using Diesel's QueryFragment API. This turns out to be a lot less unpleasant and, despite performing less validation of the generated SQL, actually is a lot less likely to introduce hard-to-debug SQL syntax errors. Since most of the query can be written in a single string literal, you actually see more or less what you're gonna get, which is much less obvious with Diesel's API. The InsertSitrepVersionQuery CTE in the same module still uses the QueryFragment API, rather than the raw_query_builder module, since I didn't want to change the existing code in that branch.

This PR refactors the CTE for inserting a new sitrep version to also use raw_query_builder, which is much nicer and should be easier to modify in the future. There should be no functional changes here, just a change to use a different API to generate the query.

Incidentally, this was one of the first times I had real success getting Claude to do a fairly trivial "transform this code from using one API to using another" task --- it wrote most of the first commit, and I tweaked some of its style decisions I didn't love.

@hawkw hawkw requested a review from smklein November 10, 2025 21:39
@hawkw hawkw changed the title Eliza/sitrep insert cte raw [nexus] use raw query builder for sitrep version insertion CTE Nov 10, 2025
Base automatically changed from eliza/sitrep-gc to main November 10, 2025 21:51
@hawkw hawkw force-pushed the eliza/sitrep-insert-cte-raw branch from bf16ab3 to b3663cd Compare November 10, 2025 22:03
// version).
builder.sql(
"WITH current_sitrep AS ( \
SELECT version AS version, sitrep_id AS sitrep_id \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick - and I bet this was copied from blueprint insertion, because I made the same change there - but SELECT foo as foo is a bit silly; we can just SELECT version, sitrep_id here

@hawkw hawkw enabled auto-merge (squash) November 10, 2025 22:17
@hawkw hawkw merged commit 0eb83da into main Nov 11, 2025
16 checks passed
@hawkw hawkw deleted the eliza/sitrep-insert-cte-raw branch November 11, 2025 02:49
@hawkw hawkw added the fault-management Everything related to the fault-management initiative (RFD480 and others) label Nov 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fault-management Everything related to the fault-management initiative (RFD480 and others)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants