diff --git a/docs/api/listings-input-sanitization.md b/docs/api/listings-input-sanitization.md new file mode 100644 index 00000000..0da2febd --- /dev/null +++ b/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. diff --git a/docs/api/listings-openapi.md b/docs/api/listings-openapi.md new file mode 100644 index 00000000..8482662c --- /dev/null +++ b/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. diff --git a/docs/api/listings-refactor.md b/docs/api/listings-refactor.md new file mode 100644 index 00000000..b895161a --- /dev/null +++ b/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`. diff --git a/docs/api/listings-retry.md b/docs/api/listings-retry.md new file mode 100644 index 00000000..55d0c116 --- /dev/null +++ b/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. diff --git a/src/properties/dto/bulk-operations.dto.ts b/src/properties/dto/bulk-operations.dto.ts index 75df476e..0f874b3e 100644 --- a/src/properties/dto/bulk-operations.dto.ts +++ b/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; diff --git a/src/properties/dto/search-properties.dto.ts b/src/properties/dto/search-properties.dto.ts index 2bfd9d1a..6d7ffad4 100644 --- a/src/properties/dto/search-properties.dto.ts +++ b/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; diff --git a/src/properties/listing-retry.util.ts b/src/properties/listing-retry.util.ts new file mode 100644 index 00000000..0f8f0e0f --- /dev/null +++ b/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; + /** Injectable for tests; returns a value in [0, 1). */ + random?: () => number; +} + +const defaultSleep = (ms: number) => new Promise((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( + fn: () => Promise, + options: ListingRetryOptions = {}, +): Promise { + 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; +} diff --git a/src/properties/properties.controller.ts b/src/properties/properties.controller.ts index 90da43da..d2918ccf 100644 --- a/src/properties/properties.controller.ts +++ b/src/properties/properties.controller.ts @@ -34,7 +34,13 @@ import { import { CreateAmenityDto, UpdateAmenityDto } from './dto/amenity.dto'; import { PropertyReportService } from './report/property-report.service'; import { Response } from 'express'; -import { ApiTags, ApiBearerAuth } from '@nestjs/swagger'; +import { + ApiBearerAuth, + ApiOperation, + ApiQuery, + ApiResponse, + ApiTags, +} from '@nestjs/swagger'; @ApiTags('Properties') @Controller('properties') @@ -52,6 +58,16 @@ export class PropertiesController { } @Get() + @ApiOperation({ + summary: 'List published property listings', + description: + 'Unauthenticated. Returns only ACTIVE, non-deleted listings, newest first by default. + +Rate limited to 100 requests per minute per client; see `X-RateLimit-*` response headers.', + operationId: 'listProperties', + }) + @ApiResponse({ status: 200, description: 'The published listings' }) + @ApiResponse({ status: 429, description: 'Rate limit exceeded' }) findAll() { return this.propertiesService.findAll(); } @@ -66,6 +82,97 @@ export class PropertiesController { * Defined before `:id` so the static path is matched first. */ @Get('search') + @ApiOperation({ + summary: 'Search property listings', + description: + 'Unauthenticated. Every filter is optional and they combine with AND. + +' + + 'The response is `{ items, total, page, limit, totalPages }`. Results are always ' + + 'ACTIVE, non-deleted listings regardless of the `status` filter. + +' + + '`minNeighborhoodTrustScore` / `maxNeighborhoodTrustScore` filter on a score computed ' + + 'from the neighborhood row, so it is applied in memory after fetching; a listing with ' + + 'no neighborhood scores `null` and is excluded whenever either bound is set. + +' + + 'Rate limited to 60 searches per minute per client.', + operationId: 'searchProperties', + }) + @ApiQuery({ name: 'minPrice', required: false, type: Number, description: 'Lowest asking price.' }) + @ApiQuery({ name: 'maxPrice', required: false, type: Number, description: 'Highest asking price.' }) + @ApiQuery({ name: 'city', required: false, description: 'City, case-insensitive.' }) + @ApiQuery({ name: 'state', required: false, description: 'State/province/region, case-insensitive.' }) + @ApiQuery({ name: 'zipCode', required: false, description: 'Postal code.' }) + @ApiQuery({ name: 'country', required: false, description: 'Country, case-insensitive.' }) + @ApiQuery({ + name: 'location', + required: false, + description: 'Free text matched against address, city, state and zipCode (OR).', + }) + @ApiQuery({ name: 'propertyType', required: false, description: 'HOUSE, APARTMENT, CONDO, LAND, ...' }) + @ApiQuery({ name: 'bedrooms', required: false, type: Number, description: 'Exact bedroom count.' }) + @ApiQuery({ name: 'minBedrooms', required: false, type: Number, description: 'Overrides `bedrooms` when both are given.' }) + @ApiQuery({ name: 'maxBedrooms', required: false, type: Number }) + @ApiQuery({ name: 'bathrooms', required: false, type: Number, description: 'Exact count; half-baths allowed.' }) + @ApiQuery({ name: 'minBathrooms', required: false, type: Number }) + @ApiQuery({ name: 'maxBathrooms', required: false, type: Number }) + @ApiQuery({ name: 'category', required: false, description: 'Listing category, case-insensitive.' }) + @ApiQuery({ name: 'tag', required: false, description: 'Exact tag match (array containment).' }) + @ApiQuery({ name: 'amenityType', required: false, description: 'Requires at least one amenity of this type.' }) + @ApiQuery({ name: 'minYearBuilt', required: false, type: Number }) + @ApiQuery({ name: 'maxYearBuilt', required: false, type: Number }) + @ApiQuery({ name: 'minNeighborhoodTrustScore', required: false, type: Number, minimum: 0, maximum: 100 }) + @ApiQuery({ name: 'maxNeighborhoodTrustScore', required: false, type: Number, minimum: 0, maximum: 100 }) + @ApiQuery({ + name: 'lat', + required: false, + type: Number, + description: 'Latitude. Must be supplied together with `lng`.', + }) + @ApiQuery({ + name: 'lng', + required: false, + type: Number, + description: 'Longitude. Must be supplied together with `lat`.', + }) + @ApiQuery({ + name: 'radiusKm', + required: false, + type: Number, + description: 'Approximated as a bounding box (1 degree of latitude is about 111 km).', + }) + @ApiQuery({ name: 'minLat', required: false, type: Number, description: 'Bounding box south edge.' }) + @ApiQuery({ name: 'maxLat', required: false, type: Number, description: 'Bounding box north edge.' }) + @ApiQuery({ name: 'minLng', required: false, type: Number, description: 'Bounding box west edge.' }) + @ApiQuery({ name: 'maxLng', required: false, type: Number, description: 'Bounding box east edge.' }) + @ApiQuery({ name: 'minExpiryDate', required: false, type: String, format: 'date-time' }) + @ApiQuery({ name: 'maxExpiryDate', required: false, type: String, format: 'date-time' }) + @ApiQuery({ + name: 'page', + required: false, + type: Number, + minimum: 1, + description: '1-based page number. Default 1.', + }) + @ApiQuery({ + name: 'limit', + required: false, + type: Number, + minimum: 1, + maximum: 100, + description: 'Listings per page. Default 20.', + }) + @ApiQuery({ + name: 'sortBy', + required: false, + description: 'price, createdAt, updatedAt, bedrooms, bathrooms, squareFeet, yearBuilt.', + }) + @ApiQuery({ name: 'sortOrder', required: false, enum: ['asc', 'desc'] }) + @ApiResponse({ status: 200, description: 'A page of matching listings' }) + @ApiResponse({ status: 400, description: 'A filter is out of range, or the geo parameters contradict' }) + @ApiResponse({ status: 429, description: 'Rate limit exceeded' }) search(@Query() searchDto: SearchPropertiesDto) { return this.propertiesService.searchProperties(searchDto); } diff --git a/src/properties/properties.service.ts b/src/properties/properties.service.ts index 5a46cd7d..e92f72f7 100644 --- a/src/properties/properties.service.ts +++ b/src/properties/properties.service.ts @@ -16,6 +16,7 @@ import { CreateAmenityDto, UpdateAmenityDto } from './dto/amenity.dto'; import { FraudService } from '../fraud/fraud.service'; import { GeocodingService } from './geocoding.service'; import { PropertyStatus, UserRole } from '../types/prisma.types'; +import { retryListingQuery } from './listing-retry.util'; import { CacheService } from '../cache/cache.service'; import { CACHE_TAGS } from '../cache/cache.config'; import { @@ -578,87 +579,131 @@ export class PropertiesService { /** * Advanced property search with filters, pagination and sorting. * Returns a paginated result so clients can render result counts and pages. + * + * Split into the two strategies it actually has. A trust-score range cannot + * be expressed as a SQL predicate, because the score is computed from five + * neighborhood columns, so it has to filter in memory; everything else pages + * in the database. Both branches plus the row annotation and the result + * shaping used to live in one 80-line method, which made the paging rules and + * the trust-score rules impossible to change independently. */ async searchProperties(dto: SearchPropertiesDto): Promise { const where = this.buildSearchWhereClause(dto); - const page = dto.page && dto.page > 0 ? dto.page : 1; const limit = dto.limit && dto.limit > 0 ? dto.limit : 20; - const sortBy = dto.sortBy ?? 'createdAt'; - const sortOrder = dto.sortOrder ?? 'desc'; + const sort = { field: dto.sortBy ?? 'createdAt', order: dto.sortOrder ?? 'desc' } as { + field: string; + order: 'asc' | 'desc'; + }; - const hasTrustScoreFilter = - dto.minNeighborhoodTrustScore !== undefined || dto.maxNeighborhoodTrustScore !== undefined; + return this.hasTrustScoreFilter(dto) + ? this.searchByTrustScore(dto, where, page, limit, sort) + : this.searchByPaging(where, page, limit, sort); + } - if (hasTrustScoreFilter) { - // Fetch all matching properties with neighborhood to filter by trust score in-memory - const allItems = await this.prisma.property.findMany({ - where, - orderBy: { [sortBy]: sortOrder }, - include: { - owner: { select: { id: true, firstName: true, lastName: true, email: true } }, - neighborhood: true, - }, - }); + private hasTrustScoreFilter(dto: SearchPropertiesDto): boolean { + return dto.minNeighborhoodTrustScore !== undefined || dto.maxNeighborhoodTrustScore !== undefined; + } - const annotated = allItems.map((p: any) => ({ - ...p, - neighborhoodTrustScore: p.neighborhood - ? this.computeNeighborhoodTrustScore(p.neighborhood) - : null, - })); - - const filtered = annotated.filter((p: any) => { - const score = p.neighborhoodTrustScore; - if ( - dto.minNeighborhoodTrustScore !== undefined && - (score === null || score < dto.minNeighborhoodTrustScore) - ) - return false; - if ( - dto.maxNeighborhoodTrustScore !== undefined && - (score === null || score > dto.maxNeighborhoodTrustScore) - ) - return false; - return true; - }); + /** Owner and neighborhood payload that every listing row is returned with. */ + private get searchRelations() { + return { + owner: { select: { id: true, firstName: true, lastName: true, email: true } }, + neighborhood: true, + }; + } - const total = filtered.length; - const skip = (page - 1) * limit; - const items = filtered.slice(skip, skip + limit); + /** Attach the computed trust score to each row. No neighborhood scores null. */ + private annotateTrustScore(rows: any[]): any[] { + return rows.map((p) => ({ + ...p, + neighborhoodTrustScore: p.neighborhood + ? this.computeNeighborhoodTrustScore(p.neighborhood) + : null, + })); + } - return { items, total, page, limit, totalPages: limit > 0 ? Math.ceil(total / limit) : 0 }; + /** + * A row without a neighborhood has no trust score, so it is excluded whenever + * either bound is set - including a bound the score would have satisfied, + * because "unknown" is not the same as "low". + */ + private matchesTrustScoreRange(score: number | null, dto: SearchPropertiesDto): boolean { + if ( + dto.minNeighborhoodTrustScore !== undefined && + (score === null || score < dto.minNeighborhoodTrustScore) + ) { + return false; } + if ( + dto.maxNeighborhoodTrustScore !== undefined && + (score === null || score > dto.maxNeighborhoodTrustScore) + ) { + return false; + } + return true; + } - const skip = (page - 1) * limit; - const [rawItems, total] = await this.prisma.$transaction([ + private buildPage( + items: T[], + total: number, + page: number, + limit: number, + ): PaginatedPropertiesResult { + return { items, total, page, limit, totalPages: limit > 0 ? Math.ceil(total / limit) : 0 }; + } + + /** + * Trust-score search: fetch every row matching the other filters, then filter + * and page in memory. `skip`/`take` cannot be pushed down here because the + * score is derived rather than stored. + */ + private async searchByTrustScore( + dto: SearchPropertiesDto, + where: Record, + page: number, + limit: number, + sort: { field: string; order: 'asc' | 'desc' }, + ): Promise { + const allItems = await retryListingQuery(() => this.prisma.property.findMany({ where, - skip, - take: limit, - orderBy: { [sortBy]: sortOrder }, - include: { - owner: { select: { id: true, firstName: true, lastName: true, email: true } }, - neighborhood: true, - }, + orderBy: { [sort.field]: sort.order }, + include: this.searchRelations, }), - this.prisma.property.count({ where }), - ]); + ); - const items = (rawItems as any[]).map((p) => ({ - ...p, - neighborhoodTrustScore: p.neighborhood - ? this.computeNeighborhoodTrustScore(p.neighborhood) - : null, - })); + const filtered = this.annotateTrustScore(allItems as any[]).filter((p) => + this.matchesTrustScoreRange(p.neighborhoodTrustScore, dto), + ); - return { - items, - total, - page, - limit, - totalPages: limit > 0 ? Math.ceil(total / limit) : 0, - }; + const skip = (page - 1) * limit; + return this.buildPage(filtered.slice(skip, skip + limit), filtered.length, page, limit); + } + + /** The common case: page and count in one transaction so they cannot disagree. */ + private async searchByPaging( + where: Record, + page: number, + limit: number, + sort: { field: string; order: 'asc' | 'desc' }, + ): Promise { + const skip = (page - 1) * limit; + + const [rawItems, total] = await retryListingQuery(() => + this.prisma.$transaction([ + this.prisma.property.findMany({ + where, + skip, + take: limit, + orderBy: { [sort.field]: sort.order }, + include: this.searchRelations, + }), + this.prisma.property.count({ where }), + ]), + ); + + return this.buildPage(this.annotateTrustScore(rawItems as any[]), total, page, limit); } computeNeighborhoodTrustScore(neighborhood: any): number { diff --git a/src/properties/sanitize-listing-query.util.ts b/src/properties/sanitize-listing-query.util.ts new file mode 100644 index 00000000..ef645558 --- /dev/null +++ b/src/properties/sanitize-listing-query.util.ts @@ -0,0 +1,27 @@ +import { TransformFnParams } from 'class-transformer'; + +// Matches C0/C1 control characters (including NUL) except tab/newline, which +// collapse into spaces for free text. +// eslint-disable-next-line no-control-regex +const CONTROL_CHARS = /[\u0000-\u0008\u000B\u000C\u000E-\u001F\u007F-\u009F]/g; + +/** + * Normalise a listing *query* value. + * + * Search and bulk filters are echoed back in logs and cache keys, and they are + * interpolated into `contains` / `has` / `equals` predicates, so they get + * trimmed and stripped of control characters before anything else looks at them. + * + * Unlike the write-path helper, HTML tags are **not** stripped: a listing + * description legitimately contains `<`, and a search for `"a < b"` must not + * silently change meaning. Tag stripping belongs where the value is stored and + * rendered, not where it is used to filter. + */ +export function sanitizeListingQuery(value: unknown): unknown { + if (typeof value !== 'string') return value; + return value.normalize('NFKC').replace(CONTROL_CHARS, '').trim(); +} + +/** class-transformer callback for listing query DTO fields. */ +export const TrimListingQuery = ({ value }: TransformFnParams): unknown => + sanitizeListingQuery(value);