Skip to content

storage: investigate eliminating PostRow (hand-written FromRow for PostRecord) #619

Description

@mdorman

Problem / question

After #616, storage/src/helpers.rs has one PostRow struct (raw DB-row decode target)
and storage/src/posts.rs has PostRecord (the domain record). They share the same 14
field names and differ only at four fields:

field PostRow (raw) PostRecord (domain)
post_id / user_id i64 PostId / UserId
rendered_html String (raw column) RenderedHtml (via gated from_trusted)
tags String (raw JSON aggregate) Vec<PostTag> (parsed, fallible)
other 10 same same

build_post_record(PostRow) -> sqlx::Result<PostRecord> is the seam: it wraps the ids,
parses the tags JSON (fallibly), and rebuilds rendered_html through
RenderedHtml::from_trusted. So PostRow is not redundant the way PostRecordParts
was
— it's the untrusted/unparsed shape, and build_post_record is a
parse-don't-validate boundary. But two near-identical types is still a smell worth a
hard look.

Investigate: eliminate PostRow

#[derive(sqlx::FromRow)] on PostRecord is not an option — it would require
RenderedHtml: Decode, and #502 deliberately made that bridge write-only ("a Decode
would bless any text column as trusted HTML"). That constraint is non-negotiable:
from_trusted must stay the single audited path that blesses HTML.

The candidate that respects it: a hand-written impl<'r, R: Row> FromRow<'r, R> for PostRecord that does what build_post_record does today — try_get::<i64>
PostId::from, try_get::<String>("rendered_html")RenderedHtml::from_trusted(...),
try_get::<String>("tags")parse_post_tags_json(...), the rest by name. FromRow's
own -> Result carries the fallible tag parse. Then the 21 query_as::<_, PostRow> sites
become query_as::<_, PostRecord>, PostRow + build_post_record +
post_record_from_row all disappear, and from_trusted is still the only blessing site
(now inside the FromRow impl instead of the builder).

Decide (this issue's deliverable)

Weigh eliminate (one fewer type; query_as yields the domain record directly) against
keep the split (the domain type PostRecord stays free of sqlx-decode + trust-blessing
logic; the raw→validated boundary is explicit). Trade-offs to judge:

  • Does putting from_trusted + JSON parsing inside PostRecord's FromRow impl couple
    a public domain type to storage/sqlx decoding in a way we don't want? (PostRecord is
    pub in storage.)
  • Is the raw PostRow genuinely useful as a distinct untrusted stage, or just ceremony?
  • Any other consumer that wants the raw row without the parse/blessing?

Acceptance

  • A recorded decision: either (a) eliminate PostRow via a hand-written
    FromRow for PostRecord (21 sites → PostRecord, PostRow/build_post_record/
    post_record_from_row deleted, from_trusted still the sole blessing path, dual-backend
    tests + the malformed-column ColumnDecode tests green), or (b) keep the split with a
    one-paragraph rationale (e.g. a doc comment on PostRow) explaining why the two types
    are intentional so it doesn't get re-flagged.
  • No change to wire, DB representation, PostRecord's public shape, or the types: align RenderedHtml with the StrNewtype trailer (owned unwrap, Borrow, PartialEq) #502 invariant.

Notes

Follow-up from #616 review. The #502 write-only-RenderedHtml-bridge invariant is the hard
constraint — any solution must keep from_trusted the single audited HTML-blessing site.

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    Projects

    Status
    Todo

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions