Skip to content

Commit 450975b

Browse files
authored
feat(properties): listing OpenAPI specs, query sanitization, search refactor, read retry (#1476)
merged
1 parent 37ad2b0 commit 450975b

10 files changed

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

‎docs/api/listings-openapi.md‎

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

‎docs/api/listings-refactor.md‎

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

‎docs/api/listings-retry.md‎

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

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ export class BulkPropertyExportDto {
2424
@IsUUID('4', { each: true })
2525
propertyIds!: string[];
2626

27+
@Transform(TrimListingQuery)
2728
@IsOptional()
2829
@IsString()
2930
filter?: string;

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

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { IsIn, IsInt, IsNumber, IsOptional, IsString, Max, Min } from 'class-validator';
2-
import { Type } from 'class-transformer';
2+
import { Transform, Type } from 'class-transformer';
33
import { PROPERTY_STATUS_ENUM } from '../../common/common.types';
4+
import { TrimListingQuery } from '../sanitize-listing-query.util';
45

56
export const PROPERTY_SORT_FIELDS = [
67
'price',
@@ -41,28 +42,34 @@ export class SearchPropertiesDto {
4142

4243
// ----- Location -----
4344
@IsOptional()
45+
@Transform(TrimListingQuery)
4446
@IsString()
4547
city?: string;
4648

4749
@IsOptional()
50+
@Transform(TrimListingQuery)
4851
@IsString()
4952
state?: string;
5053

5154
@IsOptional()
55+
@Transform(TrimListingQuery)
5256
@IsString()
5357
zipCode?: string;
5458

5559
@IsOptional()
60+
@Transform(TrimListingQuery)
5661
@IsString()
5762
country?: string;
5863

5964
/** Free-text search across address, city, state and zipCode (case-insensitive). */
6065
@IsOptional()
66+
@Transform(TrimListingQuery)
6167
@IsString()
6268
location?: string;
6369

6470
// ----- Property type -----
6571
@IsOptional()
72+
@Transform(TrimListingQuery)
6673
@IsString()
6774
propertyType?: string;
6875

@@ -161,8 +168,12 @@ export class SearchPropertiesDto {
161168
maxNeighborhoodTrustScore?: number;
162169

163170
// ----- Sorting -----
171+
// Restricted to PROPERTY_SORT_FIELDS. It was @IsString() only, so the value
172+
// went straight into `orderBy: { [sortBy]: sortOrder }` and an unknown column
173+
// produced a Prisma error (500) instead of a validation error (400).
164174
@IsOptional()
165-
@IsString()
175+
@Transform(TrimListingQuery)
176+
@IsIn(PROPERTY_SORT_FIELDS)
166177
sortBy?: string;
167178

168179
@IsOptional()
@@ -171,14 +182,17 @@ export class SearchPropertiesDto {
171182

172183
// ----- Category / tag / amenity -----
173184
@IsOptional()
185+
@Transform(TrimListingQuery)
174186
@IsString()
175187
category?: string;
176188

177189
@IsOptional()
190+
@Transform(TrimListingQuery)
178191
@IsString()
179192
tag?: string;
180193

181194
@IsOptional()
195+
@Transform(TrimListingQuery)
182196
@IsString()
183197
amenityType?: string;
184198

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,100 @@
1+
/**
2+
* Retry helper for the listing read path.
3+
*
4+
* Scoped deliberately to database reads: the predicate here is about the
5+
* connection and the pool, not about an upstream HTTP provider. Anything the
6+
* database rejected on its own terms - a bad filter, a missing row, a type
7+
* mismatch - is not retried, because the same query will fail identically next
8+
* time and retrying only adds latency to a request that was always going to fail.
9+
*/
10+
export interface ListingRetryOptions {
11+
/** Total attempts including the first. Default 2. */
12+
attempts?: number;
13+
/** Delay before the first retry in ms; doubles each retry. Default 50. */
14+
baseDelayMs?: number;
15+
/** Ceiling on any single delay in ms. Default 400. */
16+
maxDelayMs?: number;
17+
/** Injectable for tests. */
18+
sleep?: (ms: number) => Promise<void>;
19+
/** Injectable for tests; returns a value in [0, 1). */
20+
random?: () => number;
21+
}
22+
23+
const defaultSleep = (ms: number) => new Promise<void>((resolve) => setTimeout(resolve, ms));
24+
25+
/**
26+
* Prisma error codes that mean "try again", as opposed to "this query is
27+
* wrong". `P1001` cannot reach the server, `P1002` the server timed out,
28+
* `P1008` operations timed out, `P1017` the pool was closed.
29+
*/
30+
const TRANSIENT_PRISMA_CODES = new Set(['P1001', 'P1002', 'P1008', 'P1017']);
31+
32+
const TRANSIENT_NETWORK =
33+
/ECONNRESET|ECONNREFUSED|EPIPE|ETIMEDOUT|EAI_AGAIN|ENOTFOUND|socket hang up|Client has encountered a connection error|connection pool timeout|Timeout expired/i;
34+
35+
/**
36+
* Whether a failed listing read looks transient — a dropped connection, a
37+
* timeout, or the pool running dry under load — and so is worth one more try.
38+
*/
39+
export function isTransientListingReadError(error: unknown): boolean {
40+
if (!error) return false;
41+
42+
const code = (error as { code?: string }).code;
43+
if (typeof code === 'string' && TRANSIENT_PRISMA_CODES.has(code)) return true;
44+
45+
const name = (error as { name?: string }).name ?? '';
46+
if (name === 'PrismaClientInitializationError' || name === 'PrismaClientKnownRequestError') {
47+
// A known request error carries an explicit code; if it is not one of the
48+
// transient ones the query itself is at fault, so only the initialisation
49+
// variant (connection unavailable) is retryable here.
50+
if (name === 'PrismaClientKnownRequestError') return false;
51+
}
52+
53+
const message = (error as { message?: string }).message ?? '';
54+
return TRANSIENT_NETWORK.test(message);
55+
}
56+
57+
/** Delay before retry number `retry` (1-based): exponential, capped, with jitter. */
58+
export function listingBackoffDelayMs(
59+
retry: number,
60+
baseDelayMs: number,
61+
maxDelayMs: number,
62+
random: () => number,
63+
): number {
64+
const exponential = Math.min(maxDelayMs, baseDelayMs * 2 ** (retry - 1));
65+
return Math.round(exponential * (0.5 + random() * 0.5));
66+
}
67+
68+
/**
69+
* Run a listing read, retrying transient failures with exponential backoff and
70+
* jitter. The last error is rethrown once the attempts run out.
71+
*
72+
* Only two attempts by default and a much shorter ceiling than a generic
73+
* retry helper: this sits in front of an interactive request, so the point is
74+
* to ride out a blip, not to survive an outage. A client that waits longer than
75+
* a second would rather get an error and retry than hold a connection open.
76+
*/
77+
export async function retryListingQuery<T>(
78+
fn: () => Promise<T>,
79+
options: ListingRetryOptions = {},
80+
): Promise<T> {
81+
const {
82+
attempts = 2,
83+
baseDelayMs = 50,
84+
maxDelayMs = 400,
85+
sleep = defaultSleep,
86+
random = Math.random,
87+
} = options;
88+
89+
let lastError: unknown;
90+
for (let attempt = 1; attempt <= Math.max(1, attempts); attempt++) {
91+
try {
92+
return await fn();
93+
} catch (error) {
94+
lastError = error;
95+
if (attempt >= attempts || !isTransientListingReadError(error)) throw error;
96+
await sleep(listingBackoffDelayMs(attempt, baseDelayMs, maxDelayMs, random));
97+
}
98+
}
99+
throw lastError;
100+
}

0 commit comments

Comments
 (0)