Lots of code cleanups - #285
Merged
Marius Volkhart (MariusVolkhart) merged 17 commits intoSep 7, 2026
Merged
Conversation
Norm's model types were originally generated Wire protobuf messages carried
over from sqlc. That pipeline is gone — `proto/` no longer exists and the model
is plain Kotlin data classes — but the fields the proto schema had defined
stayed behind. The JDBC analyzer never assigned them and no generator code ever
read them, so anyone reading Model.kt had to work out for themselves which
properties were live. A field nothing populates is worse than a missing one: it
reads as a supported concept and invites callers to depend on it.
Deleted from the model: Column.{isNamedParam, isFuncCall, scope, tableAlias,
isSqlcSlice, unsigned, length}, Query.insertIntoTable, Catalog.{name, comment,
defaultSchema}, Schema.{comment, compositeTypes}, Identifier.catalog, and the
CompositeType class, which only ever existed to be the element type of
Schema.compositeTypes.
Also dropped three pieces of bookkeeping that were computed and then discarded:
CteDefinition.hasColumnList, ParsedCteClause.isRecursive, and the alias-name
Set half of parseOldNewAliasPrologue's return value, which now returns the
item-list start index alone. The parsers still do the same work — a column list
is still skipped, RECURSIVE is still consumed with the same word-boundary
check, the prologue still yields the same index — they just stop recording
facts no caller consults.
Column.embedTable and the embed machinery are deliberately untouched; embed
support is planned.
isRecursive and defaultSchema were each asserted on by tests despite no
production reader, so those four assertions go with them. This costs the only
pin on the RECURSIVE word-boundary parse, which is an accepted trade rather
than an oversight.
Documentation caught up in the same pass: JdbcAnalyzer's KDoc no longer claims
to produce Wire protobuf types, PgNodeExpression points at SqlCteClause.kt
instead of a SqlUtils.kt that does not exist, and CLAUDE.md drops the proto/
module and the Wire dependency from its description of the pipeline.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three entry points each derived "how do I resolve a Var in this query block" with the same six-step fallback chain hand-copied into a lambda. Three comments said "See analyzeNodeTree's identical guard" — an admission that any new rule (an RTE kind, a narrowing exception) had to land in three places and could drift. It already had: the per-site differences in CTE lookup, merge handling and the DML qual gate were incidental rather than deliberate, and analyzeQueryBlockNullability had silently lost the GROUP RTE remap step the other two carry. QueryBlockScope holds everything one query block needs to answer that question, and buildQueryBlockScope derives it once. The three sites keep only what is genuinely theirs — analyzeNodeTree's target-list substitution and RETURNING-first read. Behavior is unchanged. The recovered remap step at the third site is unreachable in practice because substituteGroupRteVars already rewrites a *GROUP* RTE Var before the chain runs; the new derived-table test passes against the pre-change analyzer as well, and pins that both paths stay correct. - Delete buildCteColumnNotNull and buildInnerCteNotNull, which existed only to give two of the lambdas their own CTE maps - Lift isProvenByQuals to a top-level function; it captured no state - Move the three guards' reasoning onto buildQueryBlockScope's KDoc - Add tests for a ctelevelsup-1 CTE read through a FROM subquery, a SubLink narrowed by its own WHERE, and a plain GROUP BY key read through a derived table Closes #274 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three functions each carried an exhaustive `when` over every PgNodeExpression subtype that did nothing but enumerate children, so the structure of the node tree was written down three times and could drift three ways. It had: substituteGroupRteVars skipped JsonConstructorExpr.function while the other two walked it, and each function's KDoc spent paragraphs explaining how its child coverage differed from the others' — documentation that only existed because the duplication existed. The structural question "what are this node's direct children?" has one answer, and it belongs on the sealed type. `children` and `mapChildren` answer it. Only they enumerate subtypes now, so a new subtype fails compilation in one place instead of silently going unwalked in another. The remaining `when`s in NodeTreeNullabilityAnalyzer branch on semantics, not structure, and stay. safetyWalkChildren keeps three deliberate exclusions — Aggref and GroupingFunc are terminal for the domination check, JsonExpr is lossy-parsed and hardcoded unsafe by its callers — but states them as a three-name exception list rather than by re-listing the other 29. One behavior change follows: substituteGroupRteVars now descends into JsonConstructorExpr.function. On PostgreSQL 18, a WindowFunc inside JSON_ARRAYAGG(x) OVER (...) whose argument is a grouping key was left as an unresolved GROUP-RTE Var and fell through to nullable; it is now substituted exactly as the same WindowFunc outside a JSON constructor already was. That is the 16/17 parity the substitution exists to restore. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
SqlLexer.skipLexicalToken already knows how to step over a string literal,
an E'' escape string, a quoted identifier, a dollar-quoted string, and a
line or block comment. Four other places re-implemented subsets of that by
hand, and each one had a different gap: the placeholder scanners missed
dollar quotes, quoted identifiers, and E'' strings; the named-parameter
scanner missed block comments, quoted identifiers, and dollar quotes. A
scanner that only mostly knows where a token ends is worse than no scanner,
because the SQL it gets wrong is silently rewritten rather than rejected.
Collapse the two placeholder scanners into one replaceParameterPlaceholders
that takes a replacement lambda over the 0-based parameter index, so the
NULL-substituting caller and the sentinel-substituting caller share one
lexer-aware pass. Rewrite convertNamedParameters around the same call.
Keep one identifier-quoting rule. JdbcAnalyzer and TypeRepository carried
two copies of it; TypeRepository's KDoc admitted as much. The JdbcAnalyzer
body survives in SqlIdentifiers, and unquoteIdentifier now delegates to the
existing unescape and truncate helpers rather than re-deriving them.
Fix the mixed-style guard while here, because the old check cannot be
expressed once the scanner is lexer-aware: it ran require('?' !in sql) over
raw text, so a query containing both 'really?' and a :name parameter was
rejected as mixing placeholder styles. The count now only rises for a ?
seen outside a lexical token, during the same pass that does the conversion.
Two behavior changes fall out and are intended, since both match what
PostgreSQL and pgjdbc do: nested block comments now nest, and a ? or :name
after the inner */ of /* a /* b */ c ? */ is left alone; and a backslash-
escaped quote inside an E'' string no longer ends the literal early.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A void transaction is a transaction whose result nobody reads. That is the only thing distinguishing it from transactionWithResult, so it should be the only thing the code says. Previously the two entry points ran separate outermost and nested execution paths whose rollback, savepoint release, parent poisoning, and cleanup logic were duplicated line for line — four copies of the transaction protocol that had to be kept in agreement by hand. The two paths already terminated RollbackException at the same place; they only disagreed on what to do afterward. The result path rethrows so the caller learns there is no value to return; the void path had nothing to return and so returned. Catching RollbackException at the delegation boundary makes that the whole difference. The catch sits in the same stack frame the old `return` did, so a nested rollback still ends at its own nesting level instead of escaping to an enclosing void frame. Renames the surviving pair to executeOutermost/executeNested — the WithResult suffix no longer distinguishes them from anything. Adds the one connection-lifecycle assertion the void entry point was missing: an outermost explicit rollback issues exactly one rollback, restores autoCommit, and closes the connection. It uses a recording wrapper over a real JDBC connection rather than a mock, so the assertions are about observed driver calls. The other three void cases were already covered. Closes #277 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Five methods each scanned :rtable for exactly one rtekind, duplicating the same extract-split-index loop and the same skip-on-missing-field rules that parseRangeTableEntries already implemented for every kind. Keeping them meant every new range-table fact had to be taught to whichever subset of the six loops happened to need it, and a fix applied to one loop silently did not reach the others. parseRangeTableEntries is now the only method that iterates :rtable. The four single-kind maps and the GROUP-RTE map become extension functions over its result, so they cannot drift from it. RangeTableEntry gains Group, holding :groupexprs as raw blocks rather than parsed expressions, because the two GROUP-RTE views disagree on purpose: groupRteMap reads the first textual :varno/:varattno, reaching a VAR nested inside a FUNCEXPR, while groupExpressions parses the block and yields the FuncExpr itself. Only the raw text can serve both. A new test pins that disagreement so a later "simplification" to parseExpression fails loudly. rtekind 9 keeps its varno key whether or not :groupexprs parses — a caller takes singleOrNull() over the map, so dropping the entry would change an answer with no golden-file symptom. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
sqlFunction and mapperFunction exist to state a query method's signature once — the name, the @throws rule, one parameter per statement parameter, the T type variable, the trailing mapper lambda. addManyImplementation ignored them and hand-rebuilt that signature three times: for the private helper, for the public Many override, and for the Dynamically override. Renaming a parameter or changing how its type resolves meant four edits, and nothing but review caught a builder left behind. All three now start from mapperFunction(statement) and state only what makes them different: the helper adds the Return type variable, the processor parameter, and a Return return type; the Many override adds nothing but the modifier and the delegating body; the dynamic variant renames via toBuilder(name) and replaces the return type. The @throws rule already excludes Command.MANY, so reusing the shared builder adds no annotation. Extracts ClassName("norm", "ManyProcessor") to MANY_PROCESSOR beside the other file-level runtime references. Generated output is unchanged: regenerating every scenario leaves test-scenarios and test-scenarios-frameworks byte-identical. Closes #279 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
TypeRepository.kt had grown to hold three unrelated concerns bundled together only because they all touched Postgres-to-Kotlin type mapping at some point: the base-type resolution table, the KDoc-rendering helpers for generated data classes, and a lexer utility for cosmetic whitespace. Splitting them into PostgresBaseTypes.kt, KdocRendering.kt, and (for the lexer helper) SqlLexer.kt lets each file be found and read on its own, and keeps TypeRepository.kt itself to the class it's named for. Pure move: no function bodies changed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
BASE_TYPE_RESOLVERS and resolveJdbcTypeInfo were two separate maps keyed by the same Postgres type names, kept in sync only by a test sweep asserting every key in one had an entry in the other. That sweep could catch a missing entry but not a mismatched one, and every base type still needed its facts written twice, once per table. Replacing both with a single POSTGRES_BASE_TYPES map, keyed once per type and holding both the plain-column SqlMappable and the wire-level JdbcTypeInfo together, makes the two facts inseparable by construction instead of by test coverage. A `register` helper's `check` guards against a name silently repeated across two calls, since `vararg` alone wouldn't catch that. No behavior change: verified before committing by dumping resolveJdbcTypeInfo and the mappable's klass/typeName/statementAction/resultSetAction for every key on both sides of the merge and diffing byte-for-byte. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
JdbcTypeInfo carried isPrimitive, useSqlTypeHint, getterClassHint and convertOffsetDateTimeToInstant as independent booleans, so the read and write paths had to reconstruct the access pattern by branching on combinations of them: AdaptedTypeSqlMappable spelled out four write lambdas and three read lambdas, and getterClassHint needed fourteen lines of KDoc to explain when it interacts with the other three. The flags were never independent — each real combination is one kind of JDBC access — and a fifth wire type would have meant a fifth flag rather than a fifth implementation. The plain (adapterless) mappables encoded that same knowledge a second time in a different shape, which is how the two could disagree: nothing tied JsonSqlMappable's setObject(i, v, Types.OTHER) to the json row of the type table beyond a comment asking the reader to keep them in sync. WireCodec names the access pattern instead, one implementation per kind, and both paths now read from it. ENUM_CODEC is the json row's codec object rather than a structural copy, so an enum column and a json column cannot drift apart. Generated code is unchanged, byte for byte. Two shapes exist today for a nullable write — the norm.setInt extension for a plain primitive column, the ?.let/setNull form for an adapted one — and they are preserved as they are: writeNullable defaults to the bare setter, PrimitiveCodec overrides it with the extension call, and the adapted path never calls it at all. Unifying those shapes is a separate, visible decision. - Replace PostgresBaseType.jdbcTypeInfo and .mappable with one .codec - Collapse JdbcTypes, NullablePrimitiveDecorator, PostgresSupportedTypes, InstantSqlMappable and JsonSqlMappable into ScalarSqlMappable(codec) - Drop SqlMappable.klass, which had no consumer and which the two adapted mappables implemented only by throwing - Rewrite the type-metadata tests to assert on rendered CodeBlocks, since the flags they inspected no longer exist Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PgCatalogLoader constructed ColumnNullabilityAnalyzer(this), and the analyzer read eleven facts back out through pass-through getters. That forced the loader to expose `internal val connection` and `internal val nodeTreeParser` for a single consumer, and made the split between the two classes nominal: neither could be understood without the other. The nine pg_catalog facts the analyzer actually needs are a value, not a service. NullabilityCatalog now owns them, lazily loaded once per connection, and the analyzer takes it as a constructor argument alongside the connection. PgCatalogLoader composes both rather than being reached back into, so the three concerns — catalog facts, per-query analysis, schema introspection — are now separable. - move nine `by lazy` properties plus isStrictFunction and their nine private load* functions to NullabilityCatalog, SQL and KDoc unchanged - ColumnNullabilityAnalyzer takes (Connection, NullabilityCatalog) and owns its own PgNodeTreeParser; the parser is stateless, so sharing one bought nothing - delete nodeTreeParser from the loader rather than privatizing it: nothing reads it once the analyzer has its own - make the loader's connection private - retarget KDoc and comments naming the moved members to their new owner - point the tests at NullabilityCatalog, preserving each site that depends on getting a fresh analyzer rather than a warmed one Fixes #282 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nalyzer b327a07 extracted ColumnNullabilityAnalyzer from PgCatalogLoader, moving analyzeNodeTree, buildSubqueryColumnNotNull, forcesNewNullable, and mergeAbsentVarnos to the new class without updating the 11 KDoc/comment references that named their old home. Each reference is retargeted with no other wording changed, since the described behaviour did not move. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
5de7a61 routed plain SELECT through prosqlbody and swept the whole probe/stub/DML-conversion subsystem these comments and KDocs described, but left the prose narrating deleted mechanisms (a SELECT-FROM-target probe, ResultSetMetaData nullability, a derived-table wrapper, a format_type cast, DML-to-SELECT conversion) as if they were still current. The tests these blocks document still pass — the prosqlbody node-tree route reaches the same answers — so only the explanation was wrong. Each rewritten block now names the current ColumnNullabilityAnalyzer mechanism that produces the answer (analyzeNodeTree's :targetList-to- :returningList substitution, isSubstitutionSafeForRelation's inverted- polarity safety check, isColumnNotNull's system-column handling, buildQueryBlockScope's DML qual-narrowing suppression), and every historical regression this test still guards against is kept but marked explicitly as history, not current behaviour. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Commit 5de7a61 deleted the probe/stub/DML-to-SELECT-conversion subsystem and replaced it with a single, unconditional route through prosqlbody: every statement (SELECT or DML, top-level or CTE-wrapped) is parsed by PostgreSQL itself inside a temporary `BEGIN ATOMIC` function, and per-column nullability comes from that real node tree (`:varnullingrels` for outer joins, `:varreturningtype` for OLD/NEW, `EXPLAIN` for MERGE match-optionality) rather than from text scanning, SQL re-composition, or a metadata-only fallback. The tests already exercised this new route and still pass, but roughly 90 lines of comments kept describing the deleted machinery (`convertDmlToSelect`, `tryPrepareStub`, `forceAllNullable`, `computeDangerousSiblingNames`, the `OldOrNewStarFailSafe` item-count cross-check, and more) as if it still existed, which misleads anyone reading the test to understand current behavior. Every stale block is rewritten to cite the live symbol that produces today's answer, or, where a block only pins a historical regression with no live successor, to say so explicitly and make no mechanism claim. Five test names that embedded now-dead terminology ("stub path", "true-scope probe", "join-based net", "prosqlbody fallback") are renamed to describe the observed behavior instead. No assertion, fixture, or production code changed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Five rewritten blocks named mechanisms that never run for their inputs. A MERGE whose USING source is a subquery cannot be named by mergeSourceRelationNameCandidates, which returns null; mergeAbsentVarnos propagates that and queryColumnNullabilityViaProsqlbody gives up, so every column falls back to nullable. The prose instead credited :varnullingrels, PgNodeTreeParser.hasGroupingSets, and per-column join reasoning -- none of which is consulted on that path. The tests pass either way, so only reading the sentences against the source catches it. merge_action() was likewise called an ordinary FuncExpr whose nullability came from the safe-list and strictness legs. PostgreSQL emits it as MERGESUPPORTFUNC, a node kind parseExpression has no case for, so it becomes Unknown and is reported nullable without those legs ever being reached -- the node carries no :funcid to consult them with. Separately, the NullabilityCatalog equality test compared two full functionStrictnessByOid maps on the premise that pg_proc is unchanging between the loads. It is not: tests share one container and run in parallel, and a sibling creating and dropping a function mid-test made oid 20259 appear in one map and not the other. Restricted to OIDs below FirstNormalObjectId, which no test can add or drop, keeping what the assertion proves. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The set of ColumnAdapter constructor parameters PostgresQueries needs can only be read off TypeRepository after every query has been resolved -- discovered enums and domains accumulate as a side effect of type resolution, and a type referenced only by a query parameter is not resolved until sqlFunction runs during interface generation. That requirement lived in an inline comment next to one of its several consumers, where nothing tied it to the code that depends on it. Extracting adapterParameters() gives the constraint a single place to be stated and a single place to be violated. generateQueryImplementation no longer decides what the adapters are, only how to render them, which drops it from nine positional parameters to four. The AdapterParam class it declared inline becomes a file-level AdapterParameter, spelled out per the project's naming rule. The new GenerateCodeTest case is what makes the constraint enforceable: it uses an enum solely as a parameter of an :exec UPDATE, so no result column can discover it. The golden scenarios cannot catch a too-early call, because their CRUD-synthesized SELECT * queries reach every table enum through result columns regardless. JdbcAnalyzer.buildParameters carried four optional parameters and a nullable catalog purely so the CALL fallback could call it with one argument, which cost every other caller two null guards to read. The fallback now passes empty maps and the catalog it already has in scope; the parameters it produces are unchanged, since with no inferred parameters the catalog is never consulted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A KDoc paragraph explaining why an alternative was rejected, naming which other function handles a case, or describing what a previous bug looked like goes stale the moment the code it points at moves — and it buries the contract a reader actually came for. A concrete SQL snippet stays true as long as PostgreSQL does, and can be re-run. Each block in the eight comment-heaviest generator files now keeps only its contract, the SQL counterexample when the contract rests on a non-obvious PostgreSQL fact, and the invariant the caller must uphold. Cross-references to other KDoc and to test names are gone: the counterexample carries the fact instead, so there is nothing left to keep in sync. No behavior changes — stripping comments and blank lines leaves every touched file byte-identical to its previous version. A load-bearing counterexample was kept even where no test pins it, and no version string was written that was not already verified against a live server. Also retargets one test comment that named a file deleted in an earlier commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Marius Volkhart (MariusVolkhart)
requested review from
Matthew Foster (MatthewFoster624) and
Ryan LuMaye (RyanLuMaye)
as code owners
September 7, 2026 12:01
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Marius Volkhart (MariusVolkhart)
deleted the
issue-273-remove-dead-model-fields
branch
September 7, 2026 12:26
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.