Skip to content

FromRow impls are untestable because tokio_postgres::Row cannot be constructed #546

Description

@0w3n-d

Status

FromRow in crates/database/src/postgres/postgres_db_row_parsing.rs maps
query results to domain types across roughly 98 row.get / row.try_get
calls. None of it is unit tested, and it cannot be: every impl takes
&tokio_postgres::Row, and Row has no public constructor, so a test cannot
fabricate one. The only way to reach this code today is a live query, which
puts the workspace's most fiddly parsing logic behind an external service (see
#545).

Target structure: a narrow accessor trait so the parsing logic is independent
of where the values come from.

trait RowSource {
    fn get<'a, T: FromSql<'a>>(&'a self, col: &str) -> Result<T, DatabaseError>;
}

Implemented for &tokio_postgres::Row in production, and for a
HashMap-backed fake in tests. Each FromRow impl then becomes a
dependency-free unit test: build a fake row, parse it, assert the domain type,
and assert the error when a column is missing or has the wrong type.

Why now

Column-name typos, wrong types and missing null handling are exactly the bugs
this code has, and today none of them can be caught without a database. It
also blocks making the parsing logic reusable outside helix-database.

Behavior must not change

The production path stays &Row, only reached through the new trait. The
integration tests in postgres_db_service_tests.rs cover the round trip and
must keep passing unchanged — they are the characterisation tests for this
refactor, so #545 Step 1 should land first so they can be run deliberately.

Affected surface

crates/database/src/postgres/postgres_db_row_parsing.rs, and the call sites
that construct domain types from rows.

Steps (each becomes one PR)

  • Step 1: Add the RowSource trait, implement it for &tokio_postgres::Row, and add the test fake. Migrate one FromRow impl as the worked example, with its unit tests. (tests: unit tests for the migrated impl, including the missing-column and wrong-type cases) (PR: )
  • Step 2: Migrate the remaining FromRow impls, a few per PR, each with unit tests. (tests: per impl) (PR: )

Open questions

Should RowSource be indexed by column name only, or by position too? Name
only is safer and matches how the current impls read, but a couple of sites
may use positional access.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    refactorRestructuring with no intended behavior change

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions