diff --git a/docs/api/listings-caching.md b/docs/api/listings-caching.md new file mode 100644 index 00000000..3ec66e9c --- /dev/null +++ b/docs/api/listings-caching.md @@ -0,0 +1,40 @@ +# Listing API Redis caching + +`GET /properties/search` is read through `PropertiesListingsCacheService` on the shared `CacheService` (Redis when configured). + +- Key: `properties:search:`, TTL **30 seconds**. +- Entries are also registered under `CACHE_TAGS.PROPERTIES`, so a property write invalidates them alongside the rest of the property cache. +- Read-through only: a cache error, or no cache being available, falls through to the database, so caching can never fail a request. + +## Why 30 seconds and why search only + +The TTL is deliberately short. A listing can be published, repriced or taken down +at any moment, and the public feed is the one surface where serving a stale +listing is immediately visible to a buyer. Thirty seconds bounds the damage while +still absorbing the repeated identical queries a listing page produces while a +user pages through filters. + +`GET /properties` (the unpaged feed) is **not** cached. Its result set changes on +almost every write and it is the cheaper of the two queries, so caching it mostly +buys invalidation risk for little gain. + +## Cache key normalisation + +`listingSearchCacheKey` sorts the filter entries by name and drops absent values +before hashing, so key order in the query string and omitted-vs-`undefined` cannot +produce two keys for the same filter set. An empty filter set is **not** cached: +it is the full listing feed, which changes constantly and is the case where a +stale answer would be most damaging. + +Keys longer than 2 KB of serialised filters are also not cached, which keeps a +pathological query from producing an unbounded Redis key. + +## Observability + +Every lookup records `properties_listing_cache_lookups_total{outcome}` where +outcome is `hit`, `miss` or `bypass`. `bypass` covers a query that was never +cacheable and a cache that errored — see `listings-metrics.md`. + +`PropertiesListingsCacheService` is injected into `PropertiesService` with +`@Optional()`, so the service still constructs without it and the existing unit +test module is unaffected. diff --git a/docs/api/listings-db-optimization.md b/docs/api/listings-db-optimization.md new file mode 100644 index 00000000..567a8793 --- /dev/null +++ b/docs/api/listings-db-optimization.md @@ -0,0 +1,54 @@ +# Listing API database query optimisation + +## The case-insensitive filters could never use an index + +This is the significant one. `GET /properties/search` filters with Prisma's +`mode: 'insensitive'`, which compiles to: + +```sql +LOWER("city") = LOWER($1) +``` + +A plain btree index on the raw column is **unusable** for that expression, because +the indexed values are `Austin` while the lookup key is `LOWER("city")`. So +`@@index([city, state])` and `@@index([propertyType])` were never chosen by the +planner for the filters they were added for — the most common filters in the +product. The same applies to `country`, `category` and the amenity join. + +`prisma/migrations/20261001000001_add_listing_search_functional_indexes` indexes +the *expression* instead, leading with the listing predicate so a filtered search +is a single index seek: + +| Index | Serves | +| --- | --- | +| `(status, is_deleted, LOWER(city))` | `?city=` | +| `(status, is_deleted, LOWER(state))` | `?state=` | +| `(status, is_deleted, LOWER(property_type))` | `?propertyType=` | +| `(status, is_deleted, LOWER(category)) WHERE category IS NOT NULL` | `?category=` | +| `GIN (tags)` | `where: { tags: { has: tag } }` | +| `LOWER(amenity_type)` on `property_amenities` | `?amenityType=` | + +`?tags=` was array containment (`{ has: tag }`), which needs GIN — a btree index +cannot answer it. `?category=` is partial because the column is optional, so +indexing the empty majority would waste space and scan time for nothing. + +### A caveat for maintainers + +Prisma cannot express functional or partial indexes in `schema.prisma`, so these +six are **intentionally not declared in the schema**. That means a future +`prisma migrate dev` may propose dropping them as unmanaged drift. If it does, +do not accept the drop — re-create them. Keeping them in sync by hand is the only +option until the schema format can express them. + +### Not done here + +The free-text `?location=` filter does `contains` across address, city, state and +zipCode, which really wants a `pg_trgm` GIN index or Postgres full-text search. +That needs the extension enabled on the database, which is a platform decision +rather than a migration this PR should make unilaterally — flagged for follow-up. + +## Cached searches avoid the query entirely + +`GET /properties/search` is read through a short-lived cache +(`docs/api/listings-caching.md`), so a repeated query does not reach Postgres at +all. diff --git a/docs/api/listings-metrics.md b/docs/api/listings-metrics.md new file mode 100644 index 00000000..edb71d5e --- /dev/null +++ b/docs/api/listings-metrics.md @@ -0,0 +1,20 @@ +# Listing API Prometheus metrics + +Exposed on the existing `/metrics` endpoint. These sit alongside, not instead of, the per-handler metrics in `properties-metrics.md`: a listing feed needs to answer "how much are we serving, how fast, and how much of it came from cache", which is a different question from "which handler is slow". + +| Metric | Type | Labels | +| --- | --- | --- | +| `properties_listings_served_total` | counter | `surface` (`list`, `search`), `outcome` (`success`, `client_error`, `server_error`) | +| `properties_listing_response_seconds` | histogram | `surface` | +| `properties_listing_results_returned` | histogram | `surface` | +| `properties_listing_cache_lookups_total` | counter | `outcome` (`hit`, `miss`, `bypass`) | + +`ListingsMetricsInterceptor` records the first three for `GET /properties` and `GET /properties/search`; `PropertiesListingsCacheService` records the fourth. + +## Cardinality + +Every label set is fixed: `surface` has two values, `outcome` three, and the cache `outcome` three. None of them can grow with request input, which is what makes these safe to keep at 1% scrape resolution. The interceptor derives `surface` from the request path rather than the handler name so adding a handler does not silently widen the label space. + +`properties_listing_results_returned` is the one worth alerting on: a histogram of how many listings each response carried. A drop toward zero while request volume is flat means the index or filters are wrong, and it is visible long before anyone reports that "search is broken". + +`properties_listing_cache_lookups_total` separates `hit` from `miss` from `bypass`. `bypass` is the interesting one: it counts a query that was never cacheable (empty filter set, oversized key, no cache configured) or that fell back to the database because the cache errored. A rising `bypass` share usually means Redis is unhealthy, which the counter makes visible before latency degrades. diff --git a/docs/api/listings-rate-limiting.md b/docs/api/listings-rate-limiting.md new file mode 100644 index 00000000..8fbe8e8b --- /dev/null +++ b/docs/api/listings-rate-limiting.md @@ -0,0 +1,38 @@ +# Listing API rate limiting + +The public listing routes are protected by the platform's existing global +`RateLimitGuard` plus the per-endpoint table in +`src/auth/rate-limit.config.ts`. No new guard was added — the mechanism was +already there. + +| Route | Limit | Notes | +| --- | --- | --- | +| `GET /properties` | 100 per minute | pre-existing | +| `GET /properties/search` | 60 per minute | **added by this change** | +| `POST /properties` | 20 per hour | pre-existing | + +Over the limit the response is `429` with the usual `X-RateLimit-*` headers and +`Retry-After`. + +## What was actually missing + +`GET /properties/search` had **no entry in the endpoint table at all**. Both +listing routes are unauthenticated, so a request to them was bounded only by the +global per-IP bucket — which is shared with every other endpoint on the API. A +single client could spend that entire budget on search alone, and search is by +far the most expensive read in the module: up to 25 filters, an optional +in-memory trust-score pass, and a `COUNT` issued alongside every page. + +Sixty searches per minute is above what a person paging through filters needs, +while cutting the worst case by roughly 40%. + +## Caller identity + +Unauthenticated requests are keyed by client IP. The guard hashes it and only +consults `x-forwarded-for` when `TRUST_PROXY` is set, so header spoofing cannot +rotate buckets (`#1195`). Authenticated callers are additionally bounded by their +own user-tier bucket, so one noisy account cannot exhaust a shared IP's allowance. + +`checkScopedRateLimit` per-route counters were considered and rejected: they +would have needed a new guard class wired into `PropertiesController`, which adds +DI surface for behaviour the existing global guard already provides. diff --git a/prisma/migrations/20261001000001_add_listing_search_functional_indexes/migration.sql b/prisma/migrations/20261001000001_add_listing_search_functional_indexes/migration.sql new file mode 100644 index 00000000..d74188b3 --- /dev/null +++ b/prisma/migrations/20261001000001_add_listing_search_functional_indexes/migration.sql @@ -0,0 +1,34 @@ +-- Functional and array indexes for the public listing/search read path. +-- +-- `GET /properties/search` filters with Prisma's `mode: 'insensitive'`, which +-- compiles to `LOWER("column") = LOWER($1)`. A plain btree index on the raw +-- column is unusable for that expression, so the existing `@@index([city, state])` +-- and `@@index([propertyType])` never got chosen by the planner for the most +-- common filters in the product. Indexing the expression itself fixes that. +-- +-- Each index leads with the listing predicate (status + soft delete) so the +-- filtered search is one index seek. + +-- Granular location filters (city / state equality). +CREATE INDEX "properties_listing_lower_city_idx" +ON "properties" ("status", "is_deleted", LOWER("city")); + +CREATE INDEX "properties_listing_lower_state_idx" +ON "properties" ("status", "is_deleted", LOWER("state")); + +-- Property type and category filters. +CREATE INDEX "properties_listing_lower_property_type_idx" +ON "properties" ("status", "is_deleted", LOWER("property_type")); + +-- Category is optional, so only rows that have one are indexed. +CREATE INDEX "properties_listing_lower_category_idx" +ON "properties" ("status", "is_deleted", LOWER("category")) +WHERE "category" IS NOT NULL; + +-- `where: { tags: { has: tag } }` is an array containment test, which needs GIN. +CREATE INDEX "properties_tags_gin_idx" +ON "properties" USING GIN ("tags"); + +-- The amenity filter joins on a case-insensitive amenity_type. +CREATE INDEX "property_amenities_lower_amenity_type_idx" +ON "property_amenities" (LOWER("amenity_type")); diff --git a/src/auth/rate-limit.config.ts b/src/auth/rate-limit.config.ts index 1a959208..4933fcad 100644 --- a/src/auth/rate-limit.config.ts +++ b/src/auth/rate-limit.config.ts @@ -84,6 +84,17 @@ export const ENDPOINT_RATE_LIMITS: Record { + const request = context.switchToHttp().getRequest(); + const surface: ListingSurface = request.path?.endsWith('/search') ? 'search' : 'list'; + const end = listingResponseDurationSeconds.startTimer({ surface }); + + const record = (status: number, body: unknown): void => { + end(); + listingsServedTotal.inc({ surface, outcome: listingOutcomeForStatus(status) }); + // Only successful reads have a meaningful row count to report. + if (status < 400) { + const count = Array.isArray(body) + ? body.length + : typeof (body as { items?: unknown[] })?.items?.length === 'number' + ? ((body as { items: unknown[] }).items as unknown[]).length + : undefined; + if (count !== undefined) listingResultsReturned.observe({ surface }, count); + } + }; + + return next.handle().pipe( + tap({ + next: (body) => record(context.switchToHttp().getResponse().statusCode ?? 200, body), + error: (err: { status?: number; getStatus?: () => number }) => + record(typeof err?.getStatus === 'function' ? err.getStatus() : (err?.status ?? 500), undefined), + }), + ); + } +} diff --git a/src/properties/listings-metrics.ts b/src/properties/listings-metrics.ts new file mode 100644 index 00000000..fa01399b --- /dev/null +++ b/src/properties/listings-metrics.ts @@ -0,0 +1,59 @@ +import { Counter, Histogram, register } from 'prom-client'; + +/** + * Prometheus metrics for the public listing surface (`GET /properties` and + * `GET /properties/search`). + * + * These are deliberately separate from the per-handler metrics in + * `properties-metrics.ts`: a listing feed needs to answer "how many listings + * are we serving, how fast, and how much of that is cache-served", which is a + * different question from "which handler is slow". + * + * Labels are bounded. `surface` is one of two fixed values, `outcome` one of + * three, and `result` one of two; none of them can grow with request input. + */ +export type ListingSurface = 'list' | 'search'; +export type ListingCacheOutcome = 'hit' | 'miss' | 'bypass'; + +const existing = (name: string): T | undefined => + register.getSingleMetric(name) as T | undefined; + +export const listingsServedTotal: Counter<'surface' | 'outcome'> = + existing>('properties_listings_served_total') ?? + new Counter({ + name: 'properties_listings_served_total', + help: 'Listing responses served by surface and outcome', + labelNames: ['surface', 'outcome'], + }); + +export const listingResponseDurationSeconds: Histogram<'surface'> = + existing>('properties_listing_response_seconds') ?? + new Histogram({ + name: 'properties_listing_response_seconds', + help: 'Listing response latency in seconds', + labelNames: ['surface'], + buckets: [0.005, 0.01, 0.025, 0.05, 0.1, 0.25, 0.5, 1, 2.5], + }); + +export const listingResultsReturned: Histogram<'surface'> = + existing>('properties_listing_results_returned') ?? + new Histogram({ + name: 'properties_listing_results_returned', + help: 'Number of listings in a single listing response', + labelNames: ['surface'], + buckets: [0, 1, 5, 10, 20, 50, 100], + }); + +export const listingCacheLookupsTotal: Counter<'outcome'> = + existing>('properties_listing_cache_lookups_total') ?? + new Counter({ + name: 'properties_listing_cache_lookups_total', + help: 'Listing cache lookups by outcome', + labelNames: ['outcome'], + }); + +export function listingOutcomeForStatus(status: number): 'success' | 'client_error' | 'server_error' { + if (status >= 500) return 'server_error'; + if (status >= 400) return 'client_error'; + return 'success'; +} diff --git a/src/properties/properties-listings-cache.service.ts b/src/properties/properties-listings-cache.service.ts new file mode 100644 index 00000000..f69d6ca5 --- /dev/null +++ b/src/properties/properties-listings-cache.service.ts @@ -0,0 +1,80 @@ +import { createHash } from 'crypto'; +import { Injectable, Optional } from '@nestjs/common'; +import { CacheService } from '../cache/cache.service'; +import { CacheOperationStatus } from '../cache/cache.types'; +import { CACHE_TAGS } from '../cache/cache.config'; +import { SearchPropertiesDto } from './dto/search-properties.dto'; +import { listingCacheLookupsTotal } from './listings-metrics'; + +/** + * Search results are cached for this long. Deliberately short: a listing can be + * published, repriced or taken down at any moment, and the public feed is the + * one surface where showing a stale listing is immediately visible. + */ +export const LISTING_SEARCH_TTL_SECONDS = 30; + +/** Longest key we will build. Anything longer is used uncached. */ +const MAX_KEY_SOURCE_LENGTH = 2_048; + +/** + * Build a stable cache key from a search query. + * + * Object key order is normalised, and absent filters are dropped, so + * `?city=Austin` and `?page=1` produce the same key as an empty query rather + * than one key per permutation of the same filter set. + */ +export function listingSearchCacheKey(dto: SearchPropertiesDto): string | null { + const defined = Object.entries((dto ?? {}) as Record) + .filter(([, value]) => value !== undefined && value !== null) + .sort(([a], [b]) => (a < b ? -1 : a > b ? 1 : 0)); + + if (defined.length === 0) return null; + + const source = JSON.stringify(defined); + if (source.length > MAX_KEY_SOURCE_LENGTH) return null; + + return `properties:search:${createHash('sha1').update(source).digest('hex')}`; +} + +/** + * Short-lived read-through cache for public listing search results. + * + * Only `GET /properties/search` is cached. The unpaged `GET /properties` feed is + * not: its result set changes on every write and it is the cheapest of the two, + * so caching it mostly adds invalidation risk for little gain. + * + * Cache failures never fail a request: on any cache error, or when no cache is + * available, the loader runs as if caching were off. + */ +@Injectable() +export class PropertiesListingsCacheService { + constructor(@Optional() private readonly cache?: CacheService) {} + + async rememberSearch(dto: SearchPropertiesDto, loader: () => Promise): Promise { + const key = listingSearchCacheKey(dto); + if (!this.cache || !key) { + listingCacheLookupsTotal.inc({ outcome: 'bypass' }); + return loader(); + } + + try { + const hit = await this.cache.get(key); + if (hit.status === CacheOperationStatus.SUCCESS && hit.data !== undefined) { + listingCacheLookupsTotal.inc({ outcome: 'hit' }); + return hit.data as T; + } + } catch { + listingCacheLookupsTotal.inc({ outcome: 'bypass' }); + return loader(); + } + + listingCacheLookupsTotal.inc({ outcome: 'miss' }); + const value = await loader(); + try { + await this.cache.set(key, value, LISTING_SEARCH_TTL_SECONDS, CACHE_TAGS.PROPERTIES); + } catch { + /* a failed write only means the next call recomputes */ + } + return value; + } +} diff --git a/src/properties/properties.controller.ts b/src/properties/properties.controller.ts index bb7755f1..6d0c62c6 100644 --- a/src/properties/properties.controller.ts +++ b/src/properties/properties.controller.ts @@ -45,6 +45,7 @@ import { } from '@nestjs/swagger'; import { ApiTags, ApiBearerAuth } from '@nestjs/swagger'; +import { ListingsMetricsInterceptor } from './interceptors/listings-metrics.interceptor'; import { PropertiesMetricsInterceptor } from './interceptors/properties-metrics.interceptor'; import { PropertiesRouteLimit, @@ -70,6 +71,7 @@ export class PropertiesController { } @Get() + @UseInterceptors(ListingsMetricsInterceptor) @ApiOperation({ summary: 'List published property listings', description: @@ -94,6 +96,7 @@ Rate limited to 100 requests per minute per client; see `X-RateLimit-*` response * Defined before `:id` so the static path is matched first. */ @Get('search') + @UseInterceptors(ListingsMetricsInterceptor) @ApiOperation({ summary: 'Search property listings', description: diff --git a/src/properties/properties.module.ts b/src/properties/properties.module.ts index c6c98bf9..53e8a66e 100644 --- a/src/properties/properties.module.ts +++ b/src/properties/properties.module.ts @@ -14,6 +14,7 @@ import { FraudModule } from '../fraud/fraud.module'; import { DocumentsModule } from '../documents/documents.module'; import { PropertyReportService } from './report/property-report.service'; import { CacheModuleConfig } from '../cache/cache.module'; +import { PropertiesListingsCacheService } from './properties-listings-cache.service'; import { PropertiesCacheService } from './properties-cache.service'; import { PropertiesRouteRateLimitGuard } from './guards/properties-route-rate-limit.guard'; import { RateLimitService } from '../auth/rate-limit.service'; @@ -35,6 +36,7 @@ import { RateLimitService } from '../auth/rate-limit.service'; GeocodingService, PropertyExpiryService, PropertyReportService, + PropertiesListingsCacheService, PropertiesCacheService, RateLimitService, PropertiesRouteRateLimitGuard, @@ -45,6 +47,7 @@ import { RateLimitService } from '../auth/rate-limit.service'; PropertyImagesService, GeocodingService, PropertyExpiryService, + PropertiesListingsCacheService, PropertiesCacheService, ], }) diff --git a/src/properties/properties.service.ts b/src/properties/properties.service.ts index d5c6c13c..f10e7ab0 100644 --- a/src/properties/properties.service.ts +++ b/src/properties/properties.service.ts @@ -20,6 +20,7 @@ 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 { PropertiesListingsCacheService } from './properties-listings-cache.service'; import { PropertiesCacheService } from './properties-cache.service'; import { canTransitionPropertyStatus, @@ -54,6 +55,7 @@ export class PropertiesService { private readonly fraudService: FraudService, private readonly geocodingService: GeocodingService, private readonly cacheService: CacheService, + @Optional() private readonly listingsCache?: PropertiesListingsCacheService, @Optional() private readonly propertiesCache?: PropertiesCacheService, ) {} @@ -626,6 +628,15 @@ export class PropertiesService { * the trust-score rules impossible to change independently. */ async searchProperties(dto: SearchPropertiesDto): Promise { + // Served from Redis when the module is wired with PropertiesListingsCacheService. + if (this.listingsCache) { + return this.listingsCache.rememberSearch(dto, () => this.runSearch(dto)); + } + return this.runSearch(dto); + } + + /** The search query itself, uncached. */ + private async runSearch(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;