From 9c974201390e2165a1b0b7be9423c695148f16c5 Mon Sep 17 00:00:00 2001 From: Joaquim de Souza Date: Mon, 21 Sep 2026 18:53:31 +0100 Subject: [PATCH 1/3] fix: write geography columns explicitly instead of inferring points from value shape The PointPlugin converted any object with `lat` and `lng` keys into a PostGIS point before every query. A data source whose records have columns named `lat` and `lng` therefore had its whole JSON row turned into `SRID=4326;POINT(...)`, which Postgres rejected for the jsonb column ("invalid input syntax for type json"). Geometry is no longer inferred from a value's shape. Geography columns are typed as `GeographyColumn<...>` so their insert/update type is a SQL expression, and every write goes through `toGeography(point)`. The plugin now only parses geography values on read. Repositories that accept a domain object (`upsertDataRecords`, `upsertPlacedMarker`) wrap the point themselves so callers are unchanged. Adds a regression test importing a CSV with `lat`/`lng` columns and a round-trip test for `toGeography`. Co-Authored-By: Claude Fable 5.1 --- AGENTS.md | 10 +- src/server/commands/populateGeocodeCache.ts | 10 +- src/server/jobs/importDataRecords.ts | 3 +- src/server/mapping/geocode.ts | 6 +- src/server/models/DataRecord.ts | 10 +- src/server/models/GeocodeCache.ts | 3 +- src/server/models/PlacedMarker.ts | 10 +- src/server/models/geography.ts | 23 +++ src/server/repositories/DataRecord.ts | 12 +- src/server/repositories/PlacedMarker.ts | 13 +- src/server/services/database/geography.ts | 22 +++ .../services/database/plugins/PointPlugin.ts | 143 ++---------------- tests/feature/geocodeCache.test.ts | 3 +- tests/feature/geography.test.ts | 133 ++++++++++++++++ tests/resources/lat_lng_columns.csv | 3 + 15 files changed, 251 insertions(+), 153 deletions(-) create mode 100644 src/server/models/geography.ts create mode 100644 src/server/services/database/geography.ts create mode 100644 tests/feature/geography.test.ts create mode 100644 tests/resources/lat_lng_columns.csv diff --git a/AGENTS.md b/AGENTS.md index f3aa9d572..a47b4f8b7 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -137,12 +137,14 @@ await db.insertInto("dataSource").values({ config: JSON.stringify({ type: "csv", ### PointPlugin -The `PointPlugin` handles serialisation of PostGIS geometry/geography columns: +PostGIS geography columns are handled in two halves: -- **Writing**: pass `{ lat: number, lng: number }` and the plugin converts it to `SRID=4326;POINT(lng lat)` WKT automatically. The same applies to `Polygon` and `MultiPolygon` GeoJSON objects. -- **Reading**: WKB hex strings returned by PostGIS are automatically parsed back to `{ lat, lng }` (or GeoJSON Polygon/MultiPolygon). +- **Reading**: the `PointPlugin` parses WKB hex strings returned by PostGIS back to `{ lat, lng }` (or GeoJSON Polygon/MultiPolygon for the `polygon` and `geography` columns). +- **Writing**: never pass a plain `{ lat, lng }` object. Build the value with `toGeography(point)` from `@/server/services/database/geography`, which returns a `ST_SetSRID(ST_MakePoint(...), 4326)::geography` expression. Polygons are written with an explicit `ST_GeomFromGeoJSON(...)` expression (see `upsertTurf`). -If a new PostGIS geometry column does not appear to be working (values come back as raw hex strings, or writes fail silently), check whether the column name/type is covered by the plugin's detection logic in `src/server/services/database/plugins/PointPlugin.ts`. +Geometry is deliberately **not** inferred from a value's shape: data record JSON can legitimately contain `lat` and `lng` keys, and guessing turned that JSON into a point. Type every geography column as `GeographyColumn<...>` (from `@/server/models/geography`) in its `*Table` type so that TypeScript rejects a raw object at the write site. + +If a new PostGIS geometry column does not appear to be working on read (values come back as raw hex strings), check whether the column name/type is covered by the parsing logic in `src/server/services/database/plugins/PointPlugin.ts`. ## tRPC diff --git a/src/server/commands/populateGeocodeCache.ts b/src/server/commands/populateGeocodeCache.ts index b64c556e6..cf57b5a5f 100644 --- a/src/server/commands/populateGeocodeCache.ts +++ b/src/server/commands/populateGeocodeCache.ts @@ -1,6 +1,7 @@ import { sql } from "kysely"; import { GeocodingType } from "@/models/DataSource"; import { db } from "@/server/services/database"; +import { toGeography } from "@/server/services/database/geography"; import logger from "@/server/services/logger"; import type { AddressGeocodingConfig } from "@/models/DataSource"; import type { Point } from "@/models/shared"; @@ -36,7 +37,7 @@ export default async function populateGeocodeCache() { .trim(); if (!address) continue; - entries.push({ address, point: record.geocodePoint as Point | null }); + entries.push({ address, point: record.geocodePoint }); } if (entries.length === 0) { @@ -55,7 +56,12 @@ export default async function populateGeocodeCache() { const batch = deduplicated.slice(i, i + batchSize); await db .insertInto("geocodeCache") - .values(batch) + .values( + batch.map((entry) => ({ + ...entry, + point: toGeography(entry.point), + })), + ) .onConflict((oc) => oc.column("address").doNothing()) .execute(); inserted += batch.length; diff --git a/src/server/jobs/importDataRecords.ts b/src/server/jobs/importDataRecords.ts index 85c0d1e12..eb04bab35 100644 --- a/src/server/jobs/importDataRecords.ts +++ b/src/server/jobs/importDataRecords.ts @@ -9,7 +9,6 @@ import logger from "@/server/services/logger"; import { batchAsync } from "../utils"; import { importBatch, inferColumnSemanticTypes } from "./importDataSource"; import type { GeocodeResult } from "@/models/DataRecord"; -import type { Point } from "@/models/shared"; const importDataRecords = async (args: object | null): Promise => { if (!args || !("dataSourceId" in args)) { @@ -60,7 +59,7 @@ const importDataRecords = async (args: object | null): Promise => { { json: r.json as Record, geocodeResult: r.geocodeResult as GeocodeResult | null, - geocodePoint: r.geocodePoint as Point | null, + geocodePoint: r.geocodePoint, }, ]), ); diff --git a/src/server/mapping/geocode.ts b/src/server/mapping/geocode.ts index d2dc2ad3f..605a10cd5 100644 --- a/src/server/mapping/geocode.ts +++ b/src/server/mapping/geocode.ts @@ -15,6 +15,7 @@ import { findAreasByPoint, } from "@/server/repositories/Area"; import { db } from "@/server/services/database"; +import { toGeography } from "@/server/services/database/geography"; import logger from "@/server/services/logger"; import { geojsonPointToPoint } from "../utils/geo"; import type { GeocodeContext, GeocodeResult } from "@/models/DataRecord"; @@ -439,13 +440,14 @@ const mapboxGeocode = async ( : null; const context = feature ? parseContext(feature.properties?.context) : null; + const pointExpr = toGeography(point); await db .insertInto("geocodeCache") - .values({ address, point, context }) + .values({ address, point: pointExpr, context }) .onConflict((oc) => oc .column("address") - .doUpdateSet({ point, context, createdAt: sql`now()` }), + .doUpdateSet({ point: pointExpr, context, createdAt: sql`now()` }), ) .execute(); diff --git a/src/server/models/DataRecord.ts b/src/server/models/DataRecord.ts index 0b23691c9..8c4558fa1 100644 --- a/src/server/models/DataRecord.ts +++ b/src/server/models/DataRecord.ts @@ -1,9 +1,17 @@ import type { DataRecord } from "@/models/DataRecord"; +import type { Point } from "@/models/shared"; +import type { GeographyColumn } from "@/server/models/geography"; import type { ColumnType, Generated, Insertable, Updateable } from "kysely"; -export type DataRecordTable = DataRecord & { +export type DataRecordTable = Omit & { id: Generated; createdAt: ColumnType; + geocodePoint: GeographyColumn; }; export type NewDataRecord = Insertable; export type DataRecordUpdate = Updateable; + +/** A data record to upsert, with its point as a plain `{ lat, lng }`. */ +export type NewDataRecordInput = Omit & { + geocodePoint?: Point | null; +}; diff --git a/src/server/models/GeocodeCache.ts b/src/server/models/GeocodeCache.ts index 175ff6fd7..e6797e842 100644 --- a/src/server/models/GeocodeCache.ts +++ b/src/server/models/GeocodeCache.ts @@ -1,10 +1,11 @@ import type { GeocodeContext } from "@/models/DataRecord"; import type { Point } from "@/models/shared"; +import type { GeographyColumn } from "@/server/models/geography"; import type { ColumnType, Insertable } from "kysely"; export interface GeocodeCacheTable { address: string; - point: Point | null; + point: GeographyColumn; context: GeocodeContext | null; createdAt: ColumnType; } diff --git a/src/server/models/PlacedMarker.ts b/src/server/models/PlacedMarker.ts index ee1fe4fa3..c9e69a22b 100644 --- a/src/server/models/PlacedMarker.ts +++ b/src/server/models/PlacedMarker.ts @@ -1,8 +1,16 @@ import type { PlacedMarker } from "@/models/PlacedMarker"; +import type { Point } from "@/models/shared"; +import type { GeographyColumn } from "@/server/models/geography"; import type { Generated, Insertable, Updateable } from "kysely"; -export type PlacedMarkerTable = PlacedMarker & { +export type PlacedMarkerTable = Omit & { id: Generated; + point: GeographyColumn; }; export type NewPlacedMarker = Insertable; export type PlacedMarkerUpdate = Updateable; + +/** A placed marker to upsert, with its point as a plain `{ lat, lng }`. */ +export type NewPlacedMarkerInput = Omit & { + point: Point; +}; diff --git a/src/server/models/geography.ts b/src/server/models/geography.ts new file mode 100644 index 000000000..5d1c334b1 --- /dev/null +++ b/src/server/models/geography.ts @@ -0,0 +1,23 @@ +import type { Expression } from "kysely"; +import type { ColumnType } from "kysely"; + +/** + * Insert/update type for a PostGIS geography column: an explicit SQL + * expression (see `toGeography` in `@/server/services/database/geography`), + * never a plain object. Nullable columns also accept `null`. + */ +export type GeographyValue = null extends Read + ? Expression | null + : Expression; + +/** + * A PostGIS geography column. Reads come back parsed by the PointPlugin + * (e.g. `{ lat, lng }`); writes must go through `toGeography`, so that a + * plain `{ lat, lng }` object can never be mistaken for geometry when it is + * actually destined for a JSONB column. + */ +export type GeographyColumn = ColumnType< + Read, + GeographyValue, + GeographyValue +>; diff --git a/src/server/repositories/DataRecord.ts b/src/server/repositories/DataRecord.ts index 8388a3815..b56992fa3 100644 --- a/src/server/repositories/DataRecord.ts +++ b/src/server/repositories/DataRecord.ts @@ -7,11 +7,12 @@ import { import { FilterOperator, FilterType } from "@/models/MapView"; import { InspectorComparisonStat } from "@/models/shared"; import { db } from "@/server/services/database"; +import { toGeography } from "@/server/services/database/geography"; import { monthKeyRangeToDates } from "@/utils/dataRecord"; import type { ExternalRecordUpdate } from "@/models/DataRecord"; import type { RecordFilterInput, SortInput } from "@/models/MapView"; import type { Point } from "@/models/shared"; -import type { NewDataRecord } from "@/server/models/DataRecord"; +import type { NewDataRecordInput } from "@/server/models/DataRecord"; import type { Database } from "@/server/services/database"; import type { AliasableExpression, @@ -408,11 +409,16 @@ function getDataRecordByDataSourceAndAreaCodeQuery( .selectAll(); } -export function upsertDataRecords(dataRecords: NewDataRecord[]) { +export function upsertDataRecords(dataRecords: NewDataRecordInput[]) { if (dataRecords.length === 0) return []; return db .insertInto("dataRecord") - .values(dataRecords) + .values( + dataRecords.map((record) => ({ + ...record, + geocodePoint: toGeography(record.geocodePoint), + })), + ) .onConflict((oc) => oc.columns(["externalId", "dataSourceId"]).doUpdateSet((eb) => ({ json: eb.ref("excluded.json"), diff --git a/src/server/repositories/PlacedMarker.ts b/src/server/repositories/PlacedMarker.ts index fa9298cd9..9755f8b5e 100644 --- a/src/server/repositories/PlacedMarker.ts +++ b/src/server/repositories/PlacedMarker.ts @@ -1,5 +1,6 @@ import { db } from "@/server/services/database"; -import type { NewPlacedMarker } from "@/server/models/PlacedMarker"; +import { toGeography } from "@/server/services/database/geography"; +import type { NewPlacedMarkerInput } from "@/server/models/PlacedMarker"; export function findPlacedMarkersByMapId(mapId: string) { return db @@ -22,11 +23,15 @@ export async function deletePlacedMarkersByFolderId(folderId: string) { .execute(); } -export async function upsertPlacedMarker(placedMarker: NewPlacedMarker) { +export async function upsertPlacedMarker(placedMarker: NewPlacedMarkerInput) { + const values = { + ...placedMarker, + point: toGeography(placedMarker.point), + }; return db .insertInto("placedMarker") - .values(placedMarker) - .onConflict((oc) => oc.columns(["id"]).doUpdateSet(placedMarker)) + .values(values) + .onConflict((oc) => oc.columns(["id"]).doUpdateSet(values)) .returningAll() .executeTakeFirstOrThrow(); } diff --git a/src/server/services/database/geography.ts b/src/server/services/database/geography.ts new file mode 100644 index 000000000..2215141fd --- /dev/null +++ b/src/server/services/database/geography.ts @@ -0,0 +1,22 @@ +import { sql } from "kysely"; +import type { Point } from "@/models/shared"; +import type { RawBuilder } from "kysely"; + +/** + * Build the SQL expression for writing a `{ lat, lng }` point to a PostGIS + * geography column. Geometry is never inferred from a value's shape (a data + * record's JSON can legitimately contain `lat` and `lng` keys), so every + * geography write goes through this helper. + */ +export function toGeography(point: Point): RawBuilder; +export function toGeography( + point: Point | null | undefined, +): RawBuilder | null; +export function toGeography( + point: Point | null | undefined, +): RawBuilder | null { + if (!point) { + return null; + } + return sql`ST_SetSRID(ST_MakePoint(${point.lng}, ${point.lat}), 4326)::geography`; +} diff --git a/src/server/services/database/plugins/PointPlugin.ts b/src/server/services/database/plugins/PointPlugin.ts index f5b3e2579..4520e0eb5 100644 --- a/src/server/services/database/plugins/PointPlugin.ts +++ b/src/server/services/database/plugins/PointPlugin.ts @@ -1,4 +1,3 @@ -import { OperationNodeTransformer } from "kysely"; import logger from "../../../services/logger"; // Relative import required for Kysely CLI import type { Point } from "@/models/shared"; import type { MultiPolygon, Polygon } from "geojson"; @@ -6,18 +5,24 @@ import type { KyselyPlugin, PluginTransformQueryArgs, PluginTransformResultArgs, - PrimitiveValueListNode, QueryResult, RootOperationNode, UnknownRow, - ValueNode, } from "kysely"; +/** + * Parses PostGIS geography values on *read*: WKB hex strings become + * `{ lat, lng }` points, or GeoJSON Polygon/MultiPolygon for the `polygon` + * and `geography` columns. + * + * Writes are deliberately not handled here. Geometry cannot be inferred from + * a value's shape (a data record's JSON may itself contain `lat` and `lng` + * keys), so geography columns are typed as `GeographyColumn` and written via + * `toGeography` in `@/server/services/database/geography`. + */ export class PointPlugin implements KyselyPlugin { - readonly #transformer = new PointTransformer(); - transformQuery(args: PluginTransformQueryArgs): RootOperationNode { - return this.#transformer.transformNode(args.node); + return args.node; } async transformResult( @@ -29,132 +34,6 @@ export class PointPlugin implements KyselyPlugin { } } -class PointTransformer extends OperationNodeTransformer { - protected transformValue(node: ValueNode): ValueNode { - return { - ...node, - value: this.maybeTransformGeometry(node.value), - }; - } - - protected transformPrimitiveValueList( - node: PrimitiveValueListNode, - ): PrimitiveValueListNode { - return { - ...node, - values: node.values.map((v) => this.maybeTransformGeometry(v)), - }; - } - - private maybeTransformGeometry(value: unknown) { - if (isPoint(value)) { - return mapPoint(value); - } - if (isMultiPolygon(value)) { - return mapMultiPolygon(value); - } - if (isPolygon(value)) { - return mapPolygon(value); - } - return value; - } -} - -function mapPoint(point: Point): string { - return `SRID=4326;POINT(${point.lng} ${point.lat})`; -} - -function mapPolygon(polygon: Polygon): string { - // Convert coordinates array to WKT format - const rings = polygon.coordinates - .map((ring) => { - const coords = ring.map((coord) => `${coord[0]} ${coord[1]}`).join(", "); - return `(${coords})`; - }) - .join(", "); - - return `SRID=4326;POLYGON(${rings})`; -} - -function mapMultiPolygon(multiPolygon: MultiPolygon): string { - // Convert MultiPolygon coordinates to WKT format - const polygons = multiPolygon.coordinates - .map((polygon) => { - const rings = polygon - .map((ring) => { - const coords = ring - .map((coord) => `${coord[0]} ${coord[1]}`) - .join(", "); - return `(${coords})`; - }) - .join(", "); - return `(${rings})`; - }) - .join(", "); - - return `SRID=4326;MULTIPOLYGON(${polygons})`; -} - -function isPoint(point: unknown): point is Point { - return ( - typeof point === "object" && - point !== null && - "lat" in point && - "lng" in point - ); -} - -function isPolygon(polygon: unknown): polygon is Polygon { - const isP = - typeof polygon === "object" && - polygon !== null && - "type" in polygon && - polygon.type === "Polygon" && - "coordinates" in polygon && - Array.isArray(polygon.coordinates) && - polygon.coordinates.length > 0 && - polygon.coordinates.every( - (ring) => - Array.isArray(ring) && - ring.every( - (coord) => - Array.isArray(coord) && - coord.length === 2 && - typeof coord[0] === "number" && - typeof coord[1] === "number", - ), - ); - return isP; -} - -function isMultiPolygon(multiPolygon: unknown): multiPolygon is MultiPolygon { - return ( - typeof multiPolygon === "object" && - multiPolygon !== null && - "type" in multiPolygon && - multiPolygon.type === "MultiPolygon" && - "coordinates" in multiPolygon && - Array.isArray(multiPolygon.coordinates) && - multiPolygon.coordinates.length > 0 && - multiPolygon.coordinates.every( - (polygon) => - Array.isArray(polygon) && - polygon.every( - (ring) => - Array.isArray(ring) && - ring.every( - (coord) => - Array.isArray(coord) && - coord.length === 2 && - typeof coord[0] === "number" && - typeof coord[1] === "number", - ), - ), - ) - ); -} - -// --- New: handle reading from DB --- function mapDbRowPoints(row: Record): Record { const mapped: Record = {}; for (const [key, value] of Object.entries(row)) { diff --git a/tests/feature/geocodeCache.test.ts b/tests/feature/geocodeCache.test.ts index 723b62772..285564974 100644 --- a/tests/feature/geocodeCache.test.ts +++ b/tests/feature/geocodeCache.test.ts @@ -8,6 +8,7 @@ import { import { getEnrichedColumn } from "@/server/mapping/enrich"; import { geocodeRecord, mapboxReverseGeocode } from "@/server/mapping/geocode"; import { db } from "@/server/services/database"; +import { toGeography } from "@/server/services/database/geography"; const MOCK_CONTEXT = { place: { name: "London" }, @@ -136,7 +137,7 @@ describe("geocode cache", () => { .insertInto("geocodeCache") .values({ address, - point: { lat: 0, lng: 0 }, + point: toGeography({ lat: 0, lng: 0 }), }) .execute(); await db diff --git a/tests/feature/geography.test.ts b/tests/feature/geography.test.ts new file mode 100644 index 000000000..a7fda4e3a --- /dev/null +++ b/tests/feature/geography.test.ts @@ -0,0 +1,133 @@ +import { afterAll, describe, expect, test } from "vitest"; +import { DataSourceRecordType, DataSourceType } from "@/models/DataSource"; +import { GeocodingType } from "@/models/DataSource"; +import { FilterType } from "@/models/MapView"; +import importDataSource from "@/server/jobs/importDataSource"; +import { + streamDataRecordsByDataSource, + upsertDataRecords, +} from "@/server/repositories/DataRecord"; +import { + createDataSource, + deleteDataSource, +} from "@/server/repositories/DataSource"; +import { upsertOrganisation } from "@/server/repositories/Organisation"; +import { db } from "@/server/services/database"; +import { toGeography } from "@/server/services/database/geography"; + +/** + * Geography values are written explicitly via `toGeography` and parsed on + * read by the PointPlugin. A JSONB value that happens to have `lat`/`lng` + * keys must be stored as JSON, not turned into a point. + */ +describe("geography columns", () => { + const toRemove: string[] = []; + + afterAll(async () => { + for (const id of toRemove) { + await deleteDataSource(id); + } + }); + + const createSource = async (config: { + type: DataSourceType.CSV; + url: string; + }) => { + const org = await upsertOrganisation({ name: "Test Geography Org" }); + const dataSource = await createDataSource({ + name: "Test Geography Source", + autoEnrich: false, + autoImport: false, + recordType: DataSourceRecordType.Other, + config, + columnDefs: [], + columnMetadata: [], + columnRoles: { nameColumns: [] }, + enrichments: [], + geocodingConfig: { type: GeocodingType.None }, + organisationId: org.id, + public: false, + }); + toRemove.push(dataSource.id); + return dataSource; + }; + + const readRecords = async (dataSourceId: string) => { + const stream = streamDataRecordsByDataSource( + dataSourceId, + { type: FilterType.MULTI }, + "", + ); + const records = []; + for await (const record of stream) { + records.push(record); + } + return records.sort((a, b) => a.externalId.localeCompare(b.externalId)); + }; + + test("toGeography round-trips a point through a geography column", async () => { + // Config is unique per organisation, so use a distinct (unused) URL + const dataSource = await createSource({ + type: DataSourceType.CSV, + url: "file://tests/resources/members.csv", + }); + const point = { lat: 51.5074, lng: -0.1278 }; + await upsertDataRecords([ + { + externalId: "with-point", + dataSourceId: dataSource.id, + json: { name: "London" }, + geocodePoint: point, + geocodeResult: null, + }, + { + externalId: "without-point", + dataSourceId: dataSource.id, + json: { name: "Nowhere" }, + geocodePoint: null, + geocodeResult: null, + }, + ]); + + const records = await readRecords(dataSource.id); + expect(records.map((r) => r.geocodePoint)).toEqual([point, null]); + + const row = await db + .selectFrom("geocodeCache") + .select("point") + .where("address", "=", "geography-test") + .executeTakeFirst(); + expect(row).toBeUndefined(); + await db + .insertInto("geocodeCache") + .values({ address: "geography-test", point: toGeography(point) }) + .onConflict((oc) => oc.column("address").doNothing()) + .execute(); + const cached = await db + .selectFrom("geocodeCache") + .select("point") + .where("address", "=", "geography-test") + .executeTakeFirstOrThrow(); + expect(cached.point).toEqual(point); + await db + .deleteFrom("geocodeCache") + .where("address", "=", "geography-test") + .execute(); + }); + + test("record JSON with lat and lng keys is stored as JSON, not a point", async () => { + const dataSource = await createSource({ + type: DataSourceType.CSV, + url: "file://tests/resources/lat_lng_columns.csv", + }); + + const ok = await importDataSource({ dataSourceId: dataSource.id }); + expect(ok).toBe(true); + + const records = await readRecords(dataSource.id); + expect(records.map((r) => r.json)).toEqual([ + { name: "HMP Thameside", lat: 51.49451, lng: 0.08685 }, + { name: "HMP North Sea Camp", lat: 52.93997, lng: 0.06344 }, + ]); + }); +}); diff --git a/tests/resources/lat_lng_columns.csv b/tests/resources/lat_lng_columns.csv new file mode 100644 index 000000000..f418f4b70 --- /dev/null +++ b/tests/resources/lat_lng_columns.csv @@ -0,0 +1,3 @@ +name,lat,lng +HMP Thameside,51.49451,0.08685 +HMP North Sea Camp,52.93997,0.06344 From 5b8cc978f5e214bf0dfae5ead836d14d067db1af Mon Sep 17 00:00:00 2001 From: Joaquim de Souza Date: Mon, 21 Sep 2026 18:57:10 +0100 Subject: [PATCH 2/3] refactor: keep one insert and one update type per geography-bearing table Co-Authored-By: Claude Fable 5.1 --- src/server/models/DataRecord.ts | 16 ++++++++++++---- src/server/models/PlacedMarker.ts | 13 +++++++++---- src/server/repositories/DataRecord.ts | 4 ++-- src/server/repositories/PlacedMarker.ts | 4 ++-- 4 files changed, 25 insertions(+), 12 deletions(-) diff --git a/src/server/models/DataRecord.ts b/src/server/models/DataRecord.ts index 8c4558fa1..dac5d14d6 100644 --- a/src/server/models/DataRecord.ts +++ b/src/server/models/DataRecord.ts @@ -8,10 +8,18 @@ export type DataRecordTable = Omit & { createdAt: ColumnType; geocodePoint: GeographyColumn; }; -export type NewDataRecord = Insertable; -export type DataRecordUpdate = Updateable; -/** A data record to upsert, with its point as a plain `{ lat, lng }`. */ -export type NewDataRecordInput = Omit & { +// Repositories take the point as a plain `{ lat, lng }` and convert it with +// `toGeography` themselves, so callers never build the SQL expression. +export type NewDataRecord = Omit< + Insertable, + "geocodePoint" +> & { + geocodePoint?: Point | null; +}; +export type DataRecordUpdate = Omit< + Updateable, + "geocodePoint" +> & { geocodePoint?: Point | null; }; diff --git a/src/server/models/PlacedMarker.ts b/src/server/models/PlacedMarker.ts index c9e69a22b..9f8092138 100644 --- a/src/server/models/PlacedMarker.ts +++ b/src/server/models/PlacedMarker.ts @@ -7,10 +7,15 @@ export type PlacedMarkerTable = Omit & { id: Generated; point: GeographyColumn; }; -export type NewPlacedMarker = Insertable; -export type PlacedMarkerUpdate = Updateable; -/** A placed marker to upsert, with its point as a plain `{ lat, lng }`. */ -export type NewPlacedMarkerInput = Omit & { +// Repositories take the point as a plain `{ lat, lng }` and convert it with +// `toGeography` themselves, so callers never build the SQL expression. +export type NewPlacedMarker = Omit, "point"> & { point: Point; }; +export type PlacedMarkerUpdate = Omit< + Updateable, + "point" +> & { + point?: Point; +}; diff --git a/src/server/repositories/DataRecord.ts b/src/server/repositories/DataRecord.ts index b56992fa3..a22f00a03 100644 --- a/src/server/repositories/DataRecord.ts +++ b/src/server/repositories/DataRecord.ts @@ -12,7 +12,7 @@ import { monthKeyRangeToDates } from "@/utils/dataRecord"; import type { ExternalRecordUpdate } from "@/models/DataRecord"; import type { RecordFilterInput, SortInput } from "@/models/MapView"; import type { Point } from "@/models/shared"; -import type { NewDataRecordInput } from "@/server/models/DataRecord"; +import type { NewDataRecord } from "@/server/models/DataRecord"; import type { Database } from "@/server/services/database"; import type { AliasableExpression, @@ -409,7 +409,7 @@ function getDataRecordByDataSourceAndAreaCodeQuery( .selectAll(); } -export function upsertDataRecords(dataRecords: NewDataRecordInput[]) { +export function upsertDataRecords(dataRecords: NewDataRecord[]) { if (dataRecords.length === 0) return []; return db .insertInto("dataRecord") diff --git a/src/server/repositories/PlacedMarker.ts b/src/server/repositories/PlacedMarker.ts index 9755f8b5e..b6aba6d1d 100644 --- a/src/server/repositories/PlacedMarker.ts +++ b/src/server/repositories/PlacedMarker.ts @@ -1,6 +1,6 @@ import { db } from "@/server/services/database"; import { toGeography } from "@/server/services/database/geography"; -import type { NewPlacedMarkerInput } from "@/server/models/PlacedMarker"; +import type { NewPlacedMarker } from "@/server/models/PlacedMarker"; export function findPlacedMarkersByMapId(mapId: string) { return db @@ -23,7 +23,7 @@ export async function deletePlacedMarkersByFolderId(folderId: string) { .execute(); } -export async function upsertPlacedMarker(placedMarker: NewPlacedMarkerInput) { +export async function upsertPlacedMarker(placedMarker: NewPlacedMarker) { const values = { ...placedMarker, point: toGeography(placedMarker.point), From 7d29cb1a4a2d47aff180bf244ca26ebb192b3025 Mon Sep 17 00:00:00 2001 From: Joaquim de Souza Date: Mon, 21 Sep 2026 18:59:23 +0100 Subject: [PATCH 3/3] test: clean up geocode cache row unconditionally; fix stale JSONPlugin comment Co-Authored-By: Claude Fable 5.1 --- .../services/database/plugins/JSONPlugin.ts | 5 +++-- tests/feature/geography.test.ts | 22 ++++++++----------- 2 files changed, 12 insertions(+), 15 deletions(-) diff --git a/src/server/services/database/plugins/JSONPlugin.ts b/src/server/services/database/plugins/JSONPlugin.ts index 0e8a04862..30f68597d 100644 --- a/src/server/services/database/plugins/JSONPlugin.ts +++ b/src/server/services/database/plugins/JSONPlugin.ts @@ -15,8 +15,9 @@ import type { /** * Better handling of JSON serialization. Only works if *all* object/array fields - * in the database are `jsonb` columns (excluding geometry types which are handled - * by the higher priority PointPlugin). + * in the database are `jsonb` columns. Geography columns are never written as + * plain objects: use `toGeography` from `@/server/services/database/geography`, + * which produces a SQL expression this plugin leaves untouched. * * See: https://github.com/kysely-org/kysely/pull/138 */ diff --git a/tests/feature/geography.test.ts b/tests/feature/geography.test.ts index a7fda4e3a..d6c36aa2c 100644 --- a/tests/feature/geography.test.ts +++ b/tests/feature/geography.test.ts @@ -22,11 +22,16 @@ import { toGeography } from "@/server/services/database/geography"; */ describe("geography columns", () => { const toRemove: string[] = []; + const cacheAddress = "geography-test"; + + const deleteCacheRow = () => + db.deleteFrom("geocodeCache").where("address", "=", cacheAddress).execute(); afterAll(async () => { for (const id of toRemove) { await deleteDataSource(id); } + await deleteCacheRow(); }); const createSource = async (config: { @@ -92,27 +97,18 @@ describe("geography columns", () => { const records = await readRecords(dataSource.id); expect(records.map((r) => r.geocodePoint)).toEqual([point, null]); - const row = await db - .selectFrom("geocodeCache") - .select("point") - .where("address", "=", "geography-test") - .executeTakeFirst(); - expect(row).toBeUndefined(); + // Clear any row left behind by an aborted earlier run + await deleteCacheRow(); await db .insertInto("geocodeCache") - .values({ address: "geography-test", point: toGeography(point) }) - .onConflict((oc) => oc.column("address").doNothing()) + .values({ address: cacheAddress, point: toGeography(point) }) .execute(); const cached = await db .selectFrom("geocodeCache") .select("point") - .where("address", "=", "geography-test") + .where("address", "=", cacheAddress) .executeTakeFirstOrThrow(); expect(cached.point).toEqual(point); - await db - .deleteFrom("geocodeCache") - .where("address", "=", "geography-test") - .execute(); }); test("record JSON with lat and lng keys is stored as JSON, not a point", async () => {