feat(properties): listing OpenAPI specs, query sanitization, search refactor and read retry

docs/api/listings-input-sanitization.md

@@ -0,0 +1,51 @@
+# Listing API input sanitization
+
+Search and bulk-filter values are attacker-influenced and are used three times
+over: they become Prisma `equals` / `contains` / `has` predicates, they are
+hashed into cache keys, and they are written to the search log line. All three
+want the same thing — a value with no control characters and no stray
+whitespace.
+
+`sanitize-listing-query.util.ts` provides `TrimListingQuery`, wired in as
+`class-transformer` `@Transform` callbacks. The global `ValidationPipe` already
+runs with `transform: true`, so no pipe configuration changed.
+
+| Applied to | Fields |
+| --- | --- |
+| `SearchPropertiesDto` | `city`, `state`, `zipCode`, `country`, `location`, `propertyType`, `sortBy`, `category`, `tag`, `amenityType` |
+| `BulkPropertyExportDto` | `filter` |
+
+Effect: NFKC normalisation, C0/C1 control characters (including NUL) stripped,
+then trimmed.
+
+## Why this is deliberately weaker than the write path
+
+The property write DTOs (`properties-input-sanitization.md`) additionally strip
+HTML tags. Listing queries deliberately do **not**, because a query is a *filter*,
+not content:
+
+- a search for `a < b` or `2 < 3` must not silently change meaning,
+- `contains` and `equals` are already bound as parameters by Prisma, so a tag in
+  a filter has nothing to execute against,
+- stripping markup from a filter only makes the result set *less* predictable.
+
+Tag stripping belongs where a value is stored and rendered, not where it selects
+rows.
+
+## `sortBy` is now constrained to the declared allow-list
+
+`SearchPropertiesDto` has always declared `PROPERTY_SORT_FIELDS`, but `sortBy`
+was validated with `@IsString()` alone and then interpolated straight into
+`orderBy: { [sortBy]: sortOrder }`. A request for `?sortBy=nonexistent` therefore
+reached Prisma with a column that does not exist and came back as a `500`,
+rather than as the `400` a caller could act on.
+
+`sortBy` is now `@IsIn(PROPERTY_SORT_FIELDS)`, which is what the constant was
+there for. Besides producing the right status code, it stops an arbitrary column
+from being used as a sort key.
+
+## Cache-key interaction
+
+Normalising before the value reaches `listingSearchCacheKey` means `?city= Austin`
+and `?city=Austin` produce the same key. Without this, the two spellings would
+occupy separate cache entries and each would pay a full database round trip.

docs/api/listings-openapi.md

@@ -0,0 +1,55 @@
+# Listing API OpenAPI documentation
+
+The two public listing routes were undocumented: `GET /properties` had no
+operation, no description and no response codes, and `GET /properties/search`
+exposed 25 filters that appeared nowhere in the spec — so the only way to discover
+them was to read `SearchPropertiesDto`.
+
+This documents the **listing** surface. The property resource routes (create,
+update, delete, restore, status, agents, amenities, report) are documented
+separately in `properties-openapi.md`.
+
+## `GET /properties`
+
+`listProperties` — with a description that states the two facts a caller cannot
+guess from the signature (only ACTIVE, non-deleted listings; newest first by
+default), and a `429` response for the 100/minute limit.
+
+## `GET /properties/search`
+
+`searchProperties` — 34 `@ApiQuery` entries, one per filter, covering all five
+groups:
+
+| Group | Filters |
+| --- | --- |
+| Price | `minPrice`, `maxPrice` |
+| Location | `city`, `state`, `zipCode`, `country`, `location` |
+| Type / category | `propertyType`, `category`, `tag`, `amenityType` |
+| Size / age | `bedrooms`, `minBedrooms`, `maxBedrooms`, `bathrooms`, `minBathrooms`, `maxBathrooms`, `minYearBuilt`, `maxYearBuilt` |
+| Geo | `lat`, `lng`, `radiusKm`, `minLat`, `maxLat`, `minLng`, `maxLng` |
+| Quality | `minNeighborhoodTrustScore`, `maxNeighborhoodTrustScore` |
+| Paging / sorting | `page`, `limit`, `sortBy`, `sortOrder` |
+| Dates | `minExpiryDate`, `maxExpiryDate` |
+
+### The behaviours the description now spells out
+
+- Filters combine with **AND**; `location` alone is an **OR** across address,
+  city, state and zipCode.
+- Results are always ACTIVE and non-deleted **regardless of the `status`
+  parameter** — a caller passing `status=DRAFT` still gets ACTIVE listings.
+- `bedrooms`/`bathrooms` exact values take precedence over the `min`/`max` ranges
+  when both are supplied.
+- `radiusKm` is **approximated as a bounding box**, not a true radius.
+- `lat` and `lng` must be supplied together, and `radiusKm` needs either a
+  centre or a bounding box — otherwise `400`.
+- The trust-score bounds are applied **in memory after fetching**, because the
+  score is derived from the neighborhood row rather than stored. A listing with
+  no neighborhood scores `null` and is excluded whenever either bound is set.
+
+Those last three were each a plausible source of a "the API is wrong" bug report,
+and none of them were discoverable before.
+
+The project does not enable the `@nestjs/swagger` CLI plugin, so query DTOs need
+explicit `@ApiProperty`. `SearchPropertiesDto` is a query DTO consumed through
+`@Query()` and is documented with `@ApiQuery` on the handler rather than with
+`@ApiProperty`, which is the correct modelling for query parameters.

docs/api/listings-refactor.md

@@ -0,0 +1,46 @@
+# Listing API refactor: search
+
+`PropertiesService.searchProperties` was an 80-line method containing two
+completely different strategies, the row annotation and the response shaping. It
+has been split so that each concern can be read — and changed — on its own.
+
+```
+searchProperties(dto)              resolve where / page / limit / sort, then pick a strategy
+├── hasTrustScoreFilter(dto)
+├── searchByTrustScore(...)        fetch all, filter in memory, page in memory
+└── searchByPaging(...)            skip/take + COUNT in one transaction
+```
+
+Shared pieces, each now named:
+
+| Member | Responsibility |
+| --- | --- |
+| `hasTrustScoreFilter` | the one branch condition |
+| `searchRelations` | the owner/neighborhood payload every row carries, defined once instead of twice |
+| `annotateTrustScore` | attach `neighborhoodTrustScore` to each row |
+| `matchesTrustScoreRange` | the trust-score bound predicate |
+| `buildPage` | the `{ items, total, page, limit, totalPages }` shape, computed once |
+
+## Why the split helps
+
+The two strategies have opposite paging models. `searchByPaging` pushes
+`skip`/`take` into the database; `searchByTrustScore` cannot, because the score
+is derived from the neighborhood row rather than stored, so it fetches every
+matching row and pages in memory. That asymmetry used to be invisible, buried in
+the middle of one method — and it is exactly the kind of thing that gets
+"optimised" by accident by someone adding a `take` to the wrong branch.
+
+`searchRelations` is the other quiet win: the owner and neighborhood `include`
+was duplicated verbatim in both branches, so a field added to one was easy to
+forget in the other.
+
+## Behaviour is unchanged
+
+- `bedrooms`/`bathrooms` exact values still win over the ranges.
+- The public search still returns only ACTIVE, non-deleted listings whatever
+  `status` was passed.
+- A row with no neighborhood still scores `null` and is still excluded whenever
+  either trust-score bound is set.
+- The trust-score branch still fetches without `skip`/`take`, and the ordinary
+  branch still reads page and count in one `$transaction`.
+- Paging defaults are unchanged: `page` 1, `limit` 20, `createdAt desc`.

docs/api/listings-retry.md

@@ -0,0 +1,56 @@
+# Listing API retry with backoff
+
+`retryListingQuery` (`src/properties/listing-retry.util.ts`) retries a failed
+listing read with exponential backoff — doubling from the base delay, capped,
+with up to 50% jitter — and rethrows the last error once the attempts run out.
+
+Both search branches run their database reads through it: the transaction that
+pages and counts, and the trust-score branch's unbounded fetch.
+
+## What counts as transient
+
+`isTransientListingReadError` retries only failures that mean *try again*, and
+nothing else:
+
+| Retried | Not retried |
+| --- | --- |
+| Prisma `P1001` (cannot reach server), `P1002` (server timed out), `P1008` (operations timed out), `P1017` (pool closed) | every other Prisma code, including `PrismaClientKnownRequestError` |
+| `PrismaClientInitializationError` | |
+| `ECONNRESET`, `ECONNREFUSED`, `EPIPE`, `ETIMEDOUT`, `EAI_AGAIN`, `ENOTFOUND`, `socket hang up`, pool/connection timeouts | |
+
+The distinction matters: a query the database rejected on its own terms — a bad
+filter, a type mismatch, a missing relation — will fail identically on the next
+attempt, so retrying it only adds latency to a request that was always going to
+fail.
+
+## Deliberately short
+
+**Two attempts**, 50 ms base delay, 400 ms ceiling. This sits in front of an
+interactive request, so the goal is to ride out a single blip, not to survive an
+outage. A client that waits longer than about half a second would rather get an
+error and retry than hold a connection and a worker occupied.
+
+The predicate and the schedule are both tuned for that reason — a generic retry
+helper with the settings commonly used for outbound HTTP calls would be far too
+patient for an interactive read.
+
+## Not retried, deliberately
+
+- **Writes.** Retrying an `update`/`create` after a dropped connection risks
+  applying it twice. Only reads are wrapped.
+- **The whole transaction.** `$transaction([...])` is retried as a unit, since
+  the page and the count must agree.
+
+`sleep` and `random` are injectable, so the delay schedule is testable without
+waiting in real time.
+
+## Relationship to the geocoding helper
+
+`properties-retry.md` documents a sibling helper, `retryWithBackoff`
+(`src/properties/retry.util.ts`), for the outbound geocoding call. The two share
+a backoff schedule but differ deliberately in their `isRetryable` predicate and
+their timeouts: a provider `4xx` should not be repeated, a database `4xx` should
+not be repeated either, but a provider timeout and a database timeout warrant
+different amounts of patience. They are kept as separate modules so each can be
+tuned for its own dependency; consolidating them once both land is a reasonable
+follow-up.

src/properties/dto/bulk-operations.dto.ts

@@ -24,6 +24,7 @@ export class BulkPropertyExportDto {
   @IsUUID('4', { each: true })
   propertyIds!: string[];
 
+  @Transform(TrimListingQuery)
   @IsOptional()
   @IsString()
   filter?: string;

src/properties/dto/search-properties.dto.ts

@@ -1,6 +1,7 @@
 import { IsIn, IsInt, IsNumber, IsOptional, IsString, Max, Min } from 'class-validator';
-import { Type } from 'class-transformer';
+import { Transform, Type } from 'class-transformer';
 import { PROPERTY_STATUS_ENUM } from '../../common/common.types';
+import { TrimListingQuery } from '../sanitize-listing-query.util';
 
 export const PROPERTY_SORT_FIELDS = [
   'price',
@@ -41,28 +42,34 @@ export class SearchPropertiesDto {
 
   // ----- Location -----
   @IsOptional()
+  @Transform(TrimListingQuery)
   @IsString()
   city?: string;
 
   @IsOptional()
+  @Transform(TrimListingQuery)
   @IsString()
   state?: string;
 
   @IsOptional()
+  @Transform(TrimListingQuery)
   @IsString()
   zipCode?: string;
 
   @IsOptional()
+  @Transform(TrimListingQuery)
   @IsString()
   country?: string;
 
   /** Free-text search across address, city, state and zipCode (case-insensitive). */
   @IsOptional()
+  @Transform(TrimListingQuery)
   @IsString()
   location?: string;
 
   // ----- Property type -----
   @IsOptional()
+  @Transform(TrimListingQuery)
   @IsString()
   propertyType?: string;
 
@@ -161,8 +168,12 @@ export class SearchPropertiesDto {
   maxNeighborhoodTrustScore?: number;
 
   // ----- Sorting -----
+  // Restricted to PROPERTY_SORT_FIELDS. It was @IsString() only, so the value
+  // went straight into `orderBy: { [sortBy]: sortOrder }` and an unknown column
+  // produced a Prisma error (500) instead of a validation error (400).
   @IsOptional()
-  @IsString()
+  @Transform(TrimListingQuery)
+  @IsIn(PROPERTY_SORT_FIELDS)
   sortBy?: string;
 
   @IsOptional()
@@ -171,14 +182,17 @@ export class SearchPropertiesDto {
 
   // ----- Category / tag / amenity -----
   @IsOptional()
+  @Transform(TrimListingQuery)
   @IsString()
   category?: string;
 
   @IsOptional()
+  @Transform(TrimListingQuery)
   @IsString()
   tag?: string;
 
   @IsOptional()
+  @Transform(TrimListingQuery)
   @IsString()
   amenityType?: string;
 

src/properties/listing-retry.util.ts

@@ -0,0 +1,100 @@
+/**
+ * Retry helper for the listing read path.
+ *
+ * Scoped deliberately to database reads: the predicate here is about the
+ * connection and the pool, not about an upstream HTTP provider. Anything the
+ * database rejected on its own terms - a bad filter, a missing row, a type
+ * mismatch - is not retried, because the same query will fail identically next
+ * time and retrying only adds latency to a request that was always going to fail.
+ */
+export interface ListingRetryOptions {
+  /** Total attempts including the first. Default 2. */
+  attempts?: number;
+  /** Delay before the first retry in ms; doubles each retry. Default 50. */
+  baseDelayMs?: number;
+  /** Ceiling on any single delay in ms. Default 400. */
+  maxDelayMs?: number;
+  /** Injectable for tests. */
+  sleep?: (ms: number) => Promise<void>;
+  /** Injectable for tests; returns a value in [0, 1). */
+  random?: () => number;
+}
+
+const defaultSleep = (ms: number) => new Promise<void>((resolve) => setTimeout(resolve, ms));
+
+/**
+ * Prisma error codes that mean "try again", as opposed to "this query is
+ * wrong". `P1001` cannot reach the server, `P1002` the server timed out,
+ * `P1008` operations timed out, `P1017` the pool was closed.
+ */
+const TRANSIENT_PRISMA_CODES = new Set(['P1001', 'P1002', 'P1008', 'P1017']);
+
+const TRANSIENT_NETWORK =
+  /ECONNRESET|ECONNREFUSED|EPIPE|ETIMEDOUT|EAI_AGAIN|ENOTFOUND|socket hang up|Client has encountered a connection error|connection pool timeout|Timeout expired/i;
+
+/**
+ * Whether a failed listing read looks transient — a dropped connection, a
+ * timeout, or the pool running dry under load — and so is worth one more try.
+ */
+export function isTransientListingReadError(error: unknown): boolean {
+  if (!error) return false;
+
+  const code = (error as { code?: string }).code;
+  if (typeof code === 'string' && TRANSIENT_PRISMA_CODES.has(code)) return true;
+
+  const name = (error as { name?: string }).name ?? '';
+  if (name === 'PrismaClientInitializationError' || name === 'PrismaClientKnownRequestError') {
+    // A known request error carries an explicit code; if it is not one of the
+    // transient ones the query itself is at fault, so only the initialisation
+    // variant (connection unavailable) is retryable here.
+    if (name === 'PrismaClientKnownRequestError') return false;
+  }
+
+  const message = (error as { message?: string }).message ?? '';
+  return TRANSIENT_NETWORK.test(message);
+}
+
+/** Delay before retry number `retry` (1-based): exponential, capped, with jitter. */
+export function listingBackoffDelayMs(
+  retry: number,
+  baseDelayMs: number,
+  maxDelayMs: number,
+  random: () => number,
+): number {
+  const exponential = Math.min(maxDelayMs, baseDelayMs * 2 ** (retry - 1));
+  return Math.round(exponential * (0.5 + random() * 0.5));
+}
+
+/**
+ * Run a listing read, retrying transient failures with exponential backoff and
+ * jitter. The last error is rethrown once the attempts run out.
+ *
+ * Only two attempts by default and a much shorter ceiling than a generic
+ * retry helper: this sits in front of an interactive request, so the point is
+ * to ride out a blip, not to survive an outage. A client that waits longer than
+ * a second would rather get an error and retry than hold a connection open.
+ */
+export async function retryListingQuery<T>(
+  fn: () => Promise<T>,
+  options: ListingRetryOptions = {},
+): Promise<T> {
+  const {
+    attempts = 2,
+    baseDelayMs = 50,
+    maxDelayMs = 400,
+    sleep = defaultSleep,
+    random = Math.random,
+  } = options;
+
+  let lastError: unknown;
+  for (let attempt = 1; attempt <= Math.max(1, attempts); attempt++) {
+    try {
+      return await fn();
+    } catch (error) {
+      lastError = error;
+      if (attempt >= attempts || !isTransientListingReadError(error)) throw error;
+      await sleep(listingBackoffDelayMs(attempt, baseDelayMs, maxDelayMs, random));
+    }
+  }
+  throw lastError;
+}