mirror of
https://github.com/civitai/civitai.git
synced 2026-09-20 22:08:18 +08:00
fix(images): stop /api/v1/images coercing an all-digit username to a number (#4839)
Server half of civitai/cli#513 / #4768. Pairs with the submodule fix civitai/event-engine-common#13 (5da5cc6467), which this pins. The coercion was never in Meilisearch, which is what #4768's body still says. The index document's user.username never reaches the response: getImagesFromFeedSearch -> ImagesFeed.populatedQuery builds `user` from the Postgres-backed userData Redis cache and spreads it AFTER the doc, overriding it. A Redis hash stores only strings, so createCache serialised each field on write and guessed the type back on read -- isNaN(Number(v)) ? v : Number(v). Number('0222') is 222. Discriminating observation, cache-busted with cf-cache-status MISS on every row: ?username=0222&limit=11 returned "0222" (Redis miss, raw Postgres row) and limit=12..16 returned 222 (Redis hit, decoded). A Meilisearch document cannot change between two requests seconds apart. Ships both halves: the field-type declarations in the in-repo fork of the cache (apps/event-engine/src/common/caches/, which writes the SAME Redis keys), the submodule pin, and a belt-labelled String() cast at the emit site. 447 of the lines are tests. Consumer audit posted on #13: the only LIVE defect is the username one. The 'false'-as-truthy-string and array-as-raw-text cases are real in the decoder but latent -- nothing reads modelData.nsfw from the cache (all three .nsfw reads come from a direct pg.query) and nothing calls Array.isArray on the cached arrays. NOT verified: nothing was exercised against a real Redis or a deploy, and the consumer audit covers static field reads only -- services/cache.ts uses a namespace import, so a dynamic access would not have appeared. /api/v1/blocks/images shares runImageSearch but returns 401 Block token required, so it is covered by code identity, not measurement.
This commit is contained in:
Zachary Lowden
@@ -1,5 +1,6 @@
|
||||
import { IRedisClient } from '../types/package-stubs';
|
||||
import { chunk, sleep } from '../utils/basic';
|
||||
import { decodeCacheFields, type CacheFieldTypes } from './value-codec';
|
||||
|
||||
const CACHE_TTL = 24 * 60 * 60; // 24 hours
|
||||
const MISS_CACHE_TTL = 5 * 60; // 5 minutes
|
||||
@@ -17,6 +18,17 @@ export type CacheContext = {
|
||||
export type CacheConfig<T extends object> = {
|
||||
redisKey: string; // Prefix for cached items (e.g., 'user:data')
|
||||
idKey: keyof T; // Property used as key in result (e.g., 'userId')
|
||||
/**
|
||||
* How each stored field is read back out of the Redis hash.
|
||||
*
|
||||
* REQUIRED, and required to be COMPLETE — the type is a non-`Partial` record
|
||||
* over `keyof T`, so adding a column to `fetch`'s `SELECT` does not compile
|
||||
* until it is declared. Without a declaration a field falls back to guessing
|
||||
* its type from the text, which is how an all-digit username was read back as
|
||||
* a number and a leading-zero one was read back as a DIFFERENT name
|
||||
* (civitai#4768). See `caches/value-codec.ts`.
|
||||
*/
|
||||
fieldTypes: CacheFieldTypes<T>;
|
||||
fetch: (ctx: CacheContext, ids: number[]) => Promise<T[]>; // Fetch function returns array
|
||||
ttl?: number; // Default cache TTL in seconds
|
||||
debounceTime?: number; // Time for writes to propagate to read replicas (default 10s)
|
||||
@@ -86,26 +98,11 @@ export function createCache<T extends object>(config: CacheConfig<T>) {
|
||||
continue;
|
||||
}
|
||||
|
||||
// Parse the cached data
|
||||
// Parse the cached data. `value-codec` is the single place that knows
|
||||
// how a field is to be read back; inferring the type from the text here
|
||||
// is what destroyed all-digit usernames (civitai#4768).
|
||||
const cachedAt = cacheResult.cachedAt ? new Date(cacheResult.cachedAt) : new Date(0);
|
||||
const item: any = {};
|
||||
|
||||
for (const [key, value] of Object.entries(cacheResult)) {
|
||||
if (key === 'cachedAt') continue;
|
||||
|
||||
// Try to parse as JSON first (for arrays/objects)
|
||||
if (value.startsWith('[') || value.startsWith('{')) {
|
||||
try {
|
||||
item[key] = JSON.parse(value);
|
||||
continue;
|
||||
} catch {
|
||||
// Not valid JSON, fall through to string/number handling
|
||||
}
|
||||
}
|
||||
|
||||
// Try to parse as number if it looks like one
|
||||
item[key] = isNaN(Number(value)) ? value : Number(value);
|
||||
}
|
||||
const item: any = decodeCacheFields(cacheResult, config.fieldTypes);
|
||||
|
||||
// Check if stale and needs revalidation
|
||||
if (staleWhileRevalidate && cachedAt < ttlExpiry) {
|
||||
@@ -253,14 +250,12 @@ export function createCache<T extends object>(config: CacheConfig<T>) {
|
||||
// Skip not found entries
|
||||
if (cacheResult.notFound === '1') continue;
|
||||
|
||||
// Parse the cached data
|
||||
const item: any = {};
|
||||
for (const [key, value] of Object.entries(cacheResult)) {
|
||||
if (key === 'cachedAt') continue;
|
||||
item[key] = isNaN(Number(value)) ? value : Number(value);
|
||||
}
|
||||
|
||||
results[id] = item as T;
|
||||
// Parse the cached data through the SAME codec as the uncontended
|
||||
// read above. This branch used to carry its own copy of the decoder,
|
||||
// and that copy had no array/object handling at all — so a cached
|
||||
// `tags`/`cosmetics` array came back as the raw text '[3,9]' and
|
||||
// every `Array.isArray` check downstream silently saw `false`.
|
||||
results[id] = decodeCacheFields(cacheResult, config.fieldTypes) as T;
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -23,6 +23,7 @@ const ALWAYS_INCLUDE_TAGS = ['anime', 'cartoon', 'comics', 'manga', 'man', 'woma
|
||||
export const imageTagIds = createCache<ImageTagIds>({
|
||||
redisKey: 'image:tagIds',
|
||||
idKey: 'imageId',
|
||||
fieldTypes: { imageId: 'number', tags: 'json' },
|
||||
async fetch(ctx: CacheContext, ids: number[]): Promise<ImageTagIds[]> {
|
||||
// Fetch tags on image
|
||||
const imageTags = await ctx.pg.query<{
|
||||
@@ -110,6 +111,8 @@ export type TagData = {
|
||||
export const tagData = createCache<TagData>({
|
||||
redisKey: 'tag:data',
|
||||
idKey: 'id',
|
||||
// A tag NAME is a string even when it is all digits ('1', '2girls' etc.).
|
||||
fieldTypes: { id: 'number', name: 'string', type: 'number', nsfwLevel: 'number' },
|
||||
async fetch(ctx: CacheContext, ids: number[]): Promise<TagData[]> {
|
||||
return await ctx.pg.query<TagData>(
|
||||
`SELECT
|
||||
@@ -143,6 +146,13 @@ export type CosmeticData = {
|
||||
export const cosmeticData = createCache<CosmeticData>({
|
||||
redisKey: 'cosmetic:data',
|
||||
idKey: 'id',
|
||||
fieldTypes: {
|
||||
id: 'number',
|
||||
name: 'string',
|
||||
type: 'string',
|
||||
data: 'json',
|
||||
source: 'string',
|
||||
},
|
||||
async fetch(ctx: CacheContext, ids: number[]): Promise<CosmeticData[]> {
|
||||
return await ctx.pg.query<CosmeticData>(
|
||||
`SELECT
|
||||
@@ -177,6 +187,7 @@ export type UserCosmeticData = {
|
||||
export const userCosmetics = createCache<UserCosmeticData>({
|
||||
redisKey: 'user:cosmetics',
|
||||
idKey: 'userId',
|
||||
fieldTypes: { userId: 'number', cosmetics: 'json' },
|
||||
async fetch(ctx: CacheContext, ids: number[]): Promise<UserCosmeticData[]> {
|
||||
const cosmetics = await ctx.pg.query<{
|
||||
userId: number;
|
||||
@@ -233,6 +244,17 @@ export type ProfilePictureData = {
|
||||
export const profilePictures = createCache<ProfilePictureData>({
|
||||
redisKey: 'user:profilePicture',
|
||||
idKey: 'userId',
|
||||
fieldTypes: {
|
||||
userId: 'number',
|
||||
id: 'number',
|
||||
url: 'string',
|
||||
nsfwLevel: 'number',
|
||||
hash: 'string',
|
||||
type: 'string',
|
||||
width: 'number',
|
||||
height: 'number',
|
||||
metadata: 'json?',
|
||||
},
|
||||
async fetch(ctx: CacheContext, ids: number[]): Promise<ProfilePictureData[]> {
|
||||
return await ctx.pg.query<ProfilePictureData>(
|
||||
`SELECT
|
||||
|
||||
@@ -16,6 +16,13 @@ export type ModelCacheData = {
|
||||
export const modelData = createCache<ModelCacheData>({
|
||||
redisKey: 'model:data',
|
||||
idKey: 'modelId',
|
||||
fieldTypes: {
|
||||
modelId: 'number',
|
||||
name: 'string',
|
||||
type: 'string',
|
||||
nsfw: 'boolean',
|
||||
userId: 'number',
|
||||
},
|
||||
async fetch({ pg }: CacheContext, ids: number[]) {
|
||||
const models = await pg.query<ModelCacheData>(
|
||||
`SELECT
|
||||
|
||||
@@ -15,6 +15,20 @@ export type UserCacheData = {
|
||||
export const userData = createCache<UserCacheData>({
|
||||
redisKey: 'user:data',
|
||||
idKey: 'userId',
|
||||
// 🔴 `username` is a STRING and must never be inferred. A Civitai username may
|
||||
// be entirely digits (`usernameSchema` is `/^[A-Za-z0-9_]*$/`), and inferring
|
||||
// the type from the stored text turned `'0222'` into `222` — a name that
|
||||
// matches no account — and `'2428023993'` into an unquoted JSON number that
|
||||
// no typed API client could decode. civitai#4768 / civitai/cli#513.
|
||||
//
|
||||
// `username` is NOT declared nullable, and that is deliberate: the column is
|
||||
// `NOT NULL`, and `'null'` is a name this regex allows someone to hold.
|
||||
fieldTypes: {
|
||||
userId: 'number',
|
||||
username: 'string',
|
||||
image: 'string?',
|
||||
deletedAt: 'date?',
|
||||
},
|
||||
async fetch({ pg }: CacheContext, ids: number[]) {
|
||||
const users = await pg.query<UserCacheData>(
|
||||
`SELECT
|
||||
|
||||
@@ -0,0 +1,158 @@
|
||||
/**
|
||||
* The ONE place that decides how a cached entity field is read back out of a
|
||||
* Redis hash.
|
||||
*
|
||||
* A Redis hash stores strings and nothing else, so every field of every cache
|
||||
* built by `createCache` is serialised on write and de-serialised on read. The
|
||||
* read used to GUESS the original type back from the text:
|
||||
*
|
||||
* item[key] = isNaN(Number(value)) ? value : Number(value);
|
||||
*
|
||||
* That guess is lossy, and it is lossy for real, legal data:
|
||||
*
|
||||
* - `'0222'` -> `222` a username's leading zeros are DESTROYED, and
|
||||
* the name that comes out matches no account
|
||||
* - `'2428023993'` -> `2428023993` an all-digit username becomes a JSON
|
||||
* number, so `/api/v1/images` emitted
|
||||
* `"username":2428023993` unquoted and every
|
||||
* typed client failed to decode the 200
|
||||
* - `''` -> `0` because `Number('') === 0`
|
||||
* - `'false'` -> `'false'` a truthy string, for a stored `false`
|
||||
* - `'null'` -> `'null'` likewise truthy, and `deletedAt` is read as
|
||||
* `!!value` by several consumers
|
||||
*
|
||||
* See civitai#4768 / civitai/cli#513. A Civitai username may be entirely digits
|
||||
* — `usernameSchema` is `/^[A-Za-z0-9_]*$/` — so this is legal data, not bad
|
||||
* data, and the endpoint's legacy Prisma branch (which does not go through this
|
||||
* cache) returned the same names correctly quoted the whole time.
|
||||
*
|
||||
* 🔴 THE FIX DELIBERATELY DOES NOT CHANGE THE STORED FORMAT. The bytes in Redis
|
||||
* are already correct — `'0222'` is stored as `0222`; only the read destroyed
|
||||
* it. Re-encoding (per-field JSON, say) would be tidier, but it cannot be rolled
|
||||
* out without a window in which processes running the OLD code read entries
|
||||
* written by the NEW code and render every string with literal quotes, and it
|
||||
* would only start helping once each entry had been rewritten (up to its TTL).
|
||||
* Declaring the field types instead is correct for entries ALREADY in Redis, on
|
||||
* the first read, with no migration, no cold cache and no version skew.
|
||||
*
|
||||
* What a declaration cannot repair is a field whose stored text is genuinely
|
||||
* ambiguous for its own declared type — `'null'` for a non-nullable string is
|
||||
* indistinguishable from a user literally named `null`. Those are called out at
|
||||
* the declaration sites.
|
||||
*/
|
||||
|
||||
/**
|
||||
* How one cached field is to be read back.
|
||||
*
|
||||
* A trailing `?` marks the field nullable, which is what makes the stored text
|
||||
* `'null'` decode to `null` instead of to the truthy four-character string. It
|
||||
* is opt-in per field precisely because `'null'` is a legal username.
|
||||
*/
|
||||
export type CacheFieldType =
|
||||
| 'string'
|
||||
| 'string?'
|
||||
| 'number'
|
||||
| 'number?'
|
||||
| 'boolean'
|
||||
| 'boolean?'
|
||||
| 'date'
|
||||
| 'date?'
|
||||
| 'json'
|
||||
| 'json?';
|
||||
|
||||
/**
|
||||
* The declared type of every field a cache stores.
|
||||
*
|
||||
* Non-`Partial` on purpose: this is a compiler-enforced ledger, so adding a
|
||||
* column to a cache's `SELECT` (and to its `T`) fails to typecheck until the
|
||||
* field is declared here. Without that, a new field silently falls back to the
|
||||
* type guess this module exists to remove.
|
||||
*/
|
||||
export type CacheFieldTypes<T> = Record<Extract<keyof T, string>, CacheFieldType>;
|
||||
|
||||
/**
|
||||
* Hash fields that carry cache bookkeeping rather than entity data, and so must
|
||||
* never be decoded into the returned item.
|
||||
*/
|
||||
const RESERVED_FIELDS: ReadonlySet<string> = new Set(['cachedAt', 'notFound', 'debounce']);
|
||||
|
||||
/**
|
||||
* De-serialise a Redis hash into an entity, using the cache's declared field
|
||||
* types. Both read paths in `createCache` — the ordinary one and the
|
||||
* lock-contention retry — go through here, so they cannot drift apart.
|
||||
*/
|
||||
export function decodeCacheFields<T>(
|
||||
hash: Record<string, string>,
|
||||
fieldTypes: CacheFieldTypes<T>
|
||||
): Record<string, unknown> {
|
||||
const item: Record<string, unknown> = {};
|
||||
|
||||
for (const [key, value] of Object.entries(hash)) {
|
||||
if (RESERVED_FIELDS.has(key)) continue;
|
||||
|
||||
const declared = (fieldTypes as Record<string, CacheFieldType | undefined>)[key];
|
||||
item[key] = declared ? decodeDeclared(value, declared) : decodeUndeclared(value);
|
||||
}
|
||||
|
||||
return item;
|
||||
}
|
||||
|
||||
function decodeDeclared(value: string, declared: CacheFieldType): unknown {
|
||||
const nullable = declared.endsWith('?');
|
||||
const base = nullable ? declared.slice(0, -1) : declared;
|
||||
|
||||
// `String(null)`/`String(undefined)` are what the writer stored for an absent
|
||||
// value; both mean "no value" for a field declared nullable.
|
||||
if (nullable && (value === 'null' || value === 'undefined')) return null;
|
||||
|
||||
switch (base) {
|
||||
case 'string':
|
||||
return value;
|
||||
case 'number': {
|
||||
const asNumber = Number(value);
|
||||
return Number.isNaN(asNumber) ? value : asNumber;
|
||||
}
|
||||
case 'boolean':
|
||||
if (value === 'true') return true;
|
||||
if (value === 'false') return false;
|
||||
return value;
|
||||
case 'date': {
|
||||
const asDate = new Date(value);
|
||||
return Number.isNaN(asDate.getTime()) ? value : asDate;
|
||||
}
|
||||
case 'json':
|
||||
default:
|
||||
return parseJson(value);
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Fallback for a hash field with no declaration — a stale field left behind by a
|
||||
* removed column, or a bookkeeping key added by a future writer.
|
||||
*
|
||||
* This is the old guess with ONE repair, which is pure gain and cannot be wrong:
|
||||
* a number is recovered only when the text is the CANONICAL rendering of that
|
||||
* number, i.e. `String(Number(text)) === text`. `String(222)` can never produce
|
||||
* the text `'0222'`, so `'0222'` was never a number and must not be read back as
|
||||
* one. The same reasoning rescues `''` (`String(0)` is `'0'`, not `''`) and
|
||||
* `'1e5'`.
|
||||
*/
|
||||
function decodeUndeclared(value: string): unknown {
|
||||
if (value.startsWith('[') || value.startsWith('{')) {
|
||||
const parsed = parseJson(value);
|
||||
if (parsed !== value) return parsed;
|
||||
}
|
||||
|
||||
const asNumber = Number(value);
|
||||
if (!Number.isNaN(asNumber) && String(asNumber) === value) return asNumber;
|
||||
return value;
|
||||
}
|
||||
|
||||
/** JSON.parse that returns the raw text rather than throwing a whole read away. */
|
||||
function parseJson(value: string): unknown {
|
||||
try {
|
||||
return JSON.parse(value);
|
||||
} catch {
|
||||
return value;
|
||||
}
|
||||
}
|
||||
+1
-1
Submodule event-engine-common updated: e796afee3c...5da5cc6467
@@ -0,0 +1,317 @@
|
||||
import { describe, expect, it, vi } from 'vitest';
|
||||
import { createCache } from '../../../../event-engine-common/caches/base';
|
||||
import { userData } from '../../../../event-engine-common/caches/userData.cache';
|
||||
import type {
|
||||
IClickhouseClient,
|
||||
IRedisClient,
|
||||
} from '../../../../event-engine-common/types/package-stubs';
|
||||
|
||||
/**
|
||||
* REGRESSION GUARD for civitai#4768 (reported downstream as civitai/cli#513).
|
||||
*
|
||||
* `/api/v1/images?username=<all-digit-name>` emitted `username` as a bare JSON
|
||||
* NUMBER, and for a name with a leading zero it emitted a DIFFERENT name:
|
||||
* `0222` came back as `222`, which round-trips to no account at all.
|
||||
*
|
||||
* The coercion is NOT in Meilisearch and NOT at the endpoint. It is in the feed's
|
||||
* Redis entity cache. A Redis hash stores only strings, so `createCache`
|
||||
* serialises every field on write and de-serialises it on read — and the read
|
||||
* used to GUESS the type back:
|
||||
*
|
||||
* item[key] = isNaN(Number(value)) ? value : Number(value);
|
||||
*
|
||||
* `username` is a string that can be all digits (`usernameSchema` is
|
||||
* `/^[A-Za-z0-9_]*$/`), so this is legal data, not bad data. The REST endpoint's
|
||||
* legacy Prisma branch never goes through this cache, which is exactly why
|
||||
* `?imageId=` was quoted while `?username=` was not.
|
||||
*
|
||||
* These tests drive the REAL `createCache` against a fake Redis and assert the
|
||||
* property that matters: whatever the fetcher produced is what a cache HIT
|
||||
* returns. Fixture values are pairwise distinct so no assertion can pass by
|
||||
* collapsing onto a neighbour's value or onto a constant it names itself.
|
||||
*/
|
||||
|
||||
type Row = {
|
||||
userId: number;
|
||||
username: string;
|
||||
altName: string;
|
||||
bio: string;
|
||||
nsfw: boolean;
|
||||
deletedAt: string | null;
|
||||
image: string;
|
||||
tags: number[];
|
||||
meta: { weight: number };
|
||||
};
|
||||
|
||||
/** A user whose real name is all digits WITH a leading zero — the worst case. */
|
||||
const ROW: Row = {
|
||||
userId: 4768,
|
||||
username: '0222',
|
||||
altName: '2428023993',
|
||||
bio: '',
|
||||
nsfw: false,
|
||||
deletedAt: null,
|
||||
image: 'avatar-7f3a',
|
||||
tags: [3, 9],
|
||||
meta: { weight: 51 },
|
||||
};
|
||||
|
||||
const FIELD_TYPES = {
|
||||
userId: 'number',
|
||||
username: 'string',
|
||||
altName: 'string',
|
||||
bio: 'string',
|
||||
nsfw: 'boolean',
|
||||
deletedAt: 'date?',
|
||||
image: 'string',
|
||||
tags: 'json',
|
||||
meta: 'json',
|
||||
} as const;
|
||||
|
||||
type FakeRedis = IRedisClient & {
|
||||
__hashes: Map<string, Record<string, string>>;
|
||||
__strings: Map<string, string>;
|
||||
__hGetAllCalls: number;
|
||||
};
|
||||
|
||||
function makeFakeRedis(opts: { hideFirstRead?: boolean } = {}): FakeRedis {
|
||||
const hashes = new Map<string, Record<string, string>>();
|
||||
const strings = new Map<string, string>();
|
||||
const client = {
|
||||
__hashes: hashes,
|
||||
__strings: strings,
|
||||
__hGetAllCalls: 0,
|
||||
async hGetAll(key: string) {
|
||||
client.__hGetAllCalls += 1;
|
||||
if (opts.hideFirstRead && client.__hGetAllCalls === 1) return {};
|
||||
return { ...(hashes.get(key) ?? {}) };
|
||||
},
|
||||
async hSet(key: string, fields: Record<string, string>) {
|
||||
const current = hashes.get(key) ?? {};
|
||||
Object.assign(current, fields);
|
||||
hashes.set(key, current);
|
||||
return 1;
|
||||
},
|
||||
async expire() {
|
||||
return 1;
|
||||
},
|
||||
async set(key: string, value: string, options?: { NX?: boolean; EX?: number }) {
|
||||
if (options?.NX && strings.has(key)) return null;
|
||||
strings.set(key, value);
|
||||
return 'OK';
|
||||
},
|
||||
async del(keys: string | string[]) {
|
||||
const list = Array.isArray(keys) ? keys : [keys];
|
||||
for (const key of list) {
|
||||
strings.delete(key);
|
||||
hashes.delete(key);
|
||||
}
|
||||
return list.length;
|
||||
},
|
||||
} as unknown as FakeRedis;
|
||||
return client;
|
||||
}
|
||||
|
||||
const noopPg = { query: async () => [] } as unknown as {
|
||||
query: <T>(q: string, p?: any[]) => Promise<T[]>;
|
||||
};
|
||||
const noopCh = noopPg as unknown as IClickhouseClient & typeof noopPg;
|
||||
|
||||
function makeCache(rows: Row[], redisKey: string) {
|
||||
const fetcher = vi.fn(async () => rows);
|
||||
const cache = createCache<Row>({
|
||||
redisKey,
|
||||
idKey: 'userId',
|
||||
fieldTypes: FIELD_TYPES,
|
||||
fetch: fetcher,
|
||||
});
|
||||
return { cache, fetcher };
|
||||
}
|
||||
|
||||
function ctxFor(redis: FakeRedis) {
|
||||
return { redis, pg: noopPg, ch: noopCh } as any;
|
||||
}
|
||||
|
||||
describe('createCache value codec — civitai#4768 numeric-username coercion', () => {
|
||||
it('POSITIVE CONTROL: the second read is served from Redis, so the decode path really runs', async () => {
|
||||
const redis = makeFakeRedis();
|
||||
const { cache, fetcher } = makeCache([ROW], 'codec:control');
|
||||
|
||||
const miss = await cache.fetch(ctxFor(redis), [ROW.userId]);
|
||||
expect(fetcher).toHaveBeenCalledTimes(1);
|
||||
// The miss returns the fetcher's own object untouched — it proves nothing
|
||||
// about the codec, which is why every assertion below reads the SECOND call.
|
||||
expect(miss[ROW.userId].username).toBe('0222');
|
||||
|
||||
// Something was actually written to Redis. Without this, a cache that stored
|
||||
// nothing would make every "hit" below a silent re-fetch and the whole file
|
||||
// would pass while testing no decode at all.
|
||||
expect(redis.__hashes.get(`codec:control:${ROW.userId}`)).toBeDefined();
|
||||
|
||||
const hit = await cache.fetch(ctxFor(redis), [ROW.userId]);
|
||||
expect(fetcher).toHaveBeenCalledTimes(1); // no second fetch => served from cache
|
||||
expect(hit[ROW.userId]).toBeDefined();
|
||||
});
|
||||
|
||||
it('keeps an all-digit username with a leading zero as the SAME string', async () => {
|
||||
const redis = makeFakeRedis();
|
||||
const { cache } = makeCache([ROW], 'codec:leading-zero');
|
||||
|
||||
await cache.fetch(ctxFor(redis), [ROW.userId]);
|
||||
const hit = await cache.fetch(ctxFor(redis), [ROW.userId]);
|
||||
|
||||
expect(hit[ROW.userId].username).toBe('0222');
|
||||
expect(typeof hit[ROW.userId].username).toBe('string');
|
||||
});
|
||||
|
||||
it('keeps an all-digit username WITHOUT a leading zero as a string, not a number', async () => {
|
||||
const redis = makeFakeRedis();
|
||||
const { cache } = makeCache([ROW], 'codec:all-digit');
|
||||
|
||||
await cache.fetch(ctxFor(redis), [ROW.userId]);
|
||||
const hit = await cache.fetch(ctxFor(redis), [ROW.userId]);
|
||||
|
||||
expect(hit[ROW.userId].altName).toBe('2428023993');
|
||||
expect(typeof hit[ROW.userId].altName).toBe('string');
|
||||
});
|
||||
|
||||
it('round-trips every field type the caches actually store', async () => {
|
||||
const redis = makeFakeRedis();
|
||||
const { cache } = makeCache([ROW], 'codec:round-trip');
|
||||
|
||||
await cache.fetch(ctxFor(redis), [ROW.userId]);
|
||||
const hit = await cache.fetch(ctxFor(redis), [ROW.userId]);
|
||||
|
||||
// Numbers stay numbers (INVARIANT GUARD — this held before the fix too; it
|
||||
// is here so an over-correction that stringified everything is caught).
|
||||
expect(hit[ROW.userId].userId).toBe(4768);
|
||||
// `false` used to come back as the STRING 'false', which is truthy.
|
||||
expect(hit[ROW.userId].nsfw).toBe(false);
|
||||
// `null` used to come back as the STRING 'null', which is truthy — and
|
||||
// `deletedAt` is branched on with `!!user.deletedAt` by several consumers.
|
||||
expect(hit[ROW.userId].deletedAt).toBeNull();
|
||||
// An empty string used to come back as the NUMBER 0, because Number('') is 0.
|
||||
expect(hit[ROW.userId].bio).toBe('');
|
||||
// Non-numeric strings were never affected (INVARIANT GUARD).
|
||||
expect(hit[ROW.userId].image).toBe('avatar-7f3a');
|
||||
// Arrays/objects round-trip (INVARIANT GUARD for the uncontended read path).
|
||||
expect(hit[ROW.userId].tags).toEqual([3, 9]);
|
||||
expect(hit[ROW.userId].meta).toEqual({ weight: 51 });
|
||||
});
|
||||
|
||||
it('decodes arrays on the CONTENDED read path, not just the uncontended one', async () => {
|
||||
// The lock-contention retry loop had its OWN copy of the decoder, and that
|
||||
// copy lacked the array/object branch entirely — so a cached `tags` array
|
||||
// came back as the raw string '[3,9]' and every `Array.isArray` check on it
|
||||
// silently saw `false`. Consolidating the two decoders is what fixes this.
|
||||
const redis = makeFakeRedis({ hideFirstRead: true });
|
||||
const key = 'codec:contended';
|
||||
const { cache, fetcher } = makeCache([ROW], key);
|
||||
|
||||
// Warm the hash the way a real writer would (through the cache itself), then
|
||||
// hand the next reader a HELD lock so it takes the retry path.
|
||||
const warm = makeFakeRedis();
|
||||
await cache.fetch(ctxFor(warm), [ROW.userId]);
|
||||
const stored = warm.__hashes.get(`${key}:${ROW.userId}`);
|
||||
expect(stored).toBeDefined();
|
||||
redis.__hashes.set(`${key}:${ROW.userId}`, { ...(stored as Record<string, string>) });
|
||||
redis.__strings.set(`lock:${key}:${ROW.userId}`, '1'); // someone else holds it
|
||||
|
||||
fetcher.mockClear();
|
||||
const hit = await cache.fetch(ctxFor(redis), [ROW.userId]);
|
||||
|
||||
expect(fetcher).not.toHaveBeenCalled(); // proves the retry path served it
|
||||
expect(hit[ROW.userId].tags).toEqual([3, 9]);
|
||||
expect(hit[ROW.userId].username).toBe('0222');
|
||||
expect(hit[ROW.userId].meta).toEqual({ weight: 51 });
|
||||
});
|
||||
|
||||
it('repairs entries ALREADY in Redis, with no re-write and no migration', async () => {
|
||||
// The stored bytes were never wrong — `0222` is in the hash as `0222`; only
|
||||
// the read destroyed it. This is why the fix declares field types instead of
|
||||
// re-encoding: an entry written by a process running the OLD code decodes
|
||||
// correctly on the FIRST read, so there is no TTL to wait out, no cold cache
|
||||
// and no window in which two deployed versions disagree about the format.
|
||||
const redis = makeFakeRedis();
|
||||
const key = 'codec:pre-existing';
|
||||
const { cache, fetcher } = makeCache([ROW], key);
|
||||
|
||||
redis.__hashes.set(`${key}:${ROW.userId}`, {
|
||||
cachedAt: new Date().toISOString(),
|
||||
userId: '4768',
|
||||
username: '0222',
|
||||
altName: '2428023993',
|
||||
bio: '',
|
||||
nsfw: 'false',
|
||||
deletedAt: 'null',
|
||||
image: 'avatar-7f3a',
|
||||
tags: '[3,9]',
|
||||
meta: '{"weight":51}',
|
||||
});
|
||||
|
||||
const hit = await cache.fetch(ctxFor(redis), [ROW.userId]);
|
||||
expect(fetcher).not.toHaveBeenCalled(); // decoded straight out of the old hash
|
||||
|
||||
expect(hit[ROW.userId].username).toBe('0222');
|
||||
expect(hit[ROW.userId].altName).toBe('2428023993');
|
||||
expect(hit[ROW.userId].userId).toBe(4768);
|
||||
expect(hit[ROW.userId].nsfw).toBe(false);
|
||||
expect(hit[ROW.userId].deletedAt).toBeNull();
|
||||
expect(hit[ROW.userId].bio).toBe('');
|
||||
expect(hit[ROW.userId].tags).toEqual([3, 9]);
|
||||
});
|
||||
|
||||
it('a field with no declaration no longer destroys leading zeros', async () => {
|
||||
// Fallback path: a stale hash field left behind by a removed column has no
|
||||
// declaration. It still gets the type guess, but only when the text is the
|
||||
// CANONICAL rendering of the number — `String(222)` can never produce the
|
||||
// text '0222', so '0222' was never a number.
|
||||
const redis = makeFakeRedis();
|
||||
const key = 'codec:undeclared';
|
||||
const { cache, fetcher } = makeCache([ROW], key);
|
||||
|
||||
redis.__hashes.set(`${key}:${ROW.userId}`, {
|
||||
cachedAt: new Date().toISOString(),
|
||||
userId: '4768',
|
||||
username: '0222',
|
||||
altName: '2428023993',
|
||||
bio: '',
|
||||
nsfw: 'false',
|
||||
deletedAt: 'null',
|
||||
image: 'avatar-7f3a',
|
||||
tags: '[3,9]',
|
||||
meta: '{"weight":51}',
|
||||
legacyPaddedCode: '0077', // no declaration
|
||||
legacyCount: '42', // no declaration
|
||||
});
|
||||
|
||||
const hit = (await cache.fetch(ctxFor(redis), [ROW.userId]))[ROW.userId] as unknown as Record<
|
||||
string,
|
||||
unknown
|
||||
>;
|
||||
expect(fetcher).not.toHaveBeenCalled();
|
||||
|
||||
expect(hit.legacyPaddedCode).toBe('0077');
|
||||
// A canonical number is still recovered as one (INVARIANT GUARD).
|
||||
expect(hit.legacyCount).toBe(42);
|
||||
});
|
||||
});
|
||||
|
||||
describe('the userData cache declares username as a string', () => {
|
||||
// A BEHAVIOURAL check on the real cache object, not a spelling check: the
|
||||
// shipped `userData` config is what the images feed uses to build
|
||||
// `user.username`, and it is the one declaration whose absence caused #4768.
|
||||
it('never infers a type for username', async () => {
|
||||
const redis = makeFakeRedis();
|
||||
const rows = [{ userId: 4768, username: '0222', image: null, deletedAt: null }];
|
||||
const pg = { query: vi.fn(async () => rows) } as any;
|
||||
const ctx = { redis, pg, ch: noopCh } as any;
|
||||
|
||||
await userData.fetch(ctx, [4768]);
|
||||
const hit = await userData.fetch(ctx, [4768]);
|
||||
|
||||
expect(pg.query).toHaveBeenCalledTimes(1); // second read came from Redis
|
||||
expect(hit[4768].username).toBe('0222');
|
||||
expect(typeof hit[4768].username).toBe('string');
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,130 @@
|
||||
import { beforeEach, describe, expect, it, vi } from 'vitest';
|
||||
import type { NextApiRequest } from 'next';
|
||||
|
||||
/**
|
||||
* `/api/v1/images` publishes `username` as a STRING. civitai#4768 /
|
||||
* civitai/cli#513: it emitted a bare JSON number for an all-digit username, and
|
||||
* an external Go client failed to decode the entire 200 with
|
||||
* `cannot unmarshal number into Go struct field .items.username of type string`.
|
||||
*
|
||||
* The CAUSE is upstream — the feed's Redis entity cache inferred the field's
|
||||
* type from its stored text — and is guarded by
|
||||
* `feed-cache-value-codec.test.ts`. This file guards the OTHER half: the
|
||||
* published wire contract, at the one place that builds it.
|
||||
*
|
||||
* 🔴 It is a belt, not the fix, and the third case below says so as an
|
||||
* assertion rather than as a comment: a number that has already lost its leading
|
||||
* zeros casts to a string that is still the WRONG NAME. A guard that only
|
||||
* checked `typeof === 'string'` would call that a pass.
|
||||
*/
|
||||
|
||||
vi.mock('~/server/services/feature-flags.service', () => ({
|
||||
getFeatureFlags: vi.fn(() => ({ canViewNsfw: true, datapacketRead: false })),
|
||||
buildFliptContext: vi.fn(() => ({})),
|
||||
}));
|
||||
vi.mock('~/server/flipt/client', () => ({
|
||||
FLIPT_FEATURE_FLAGS: {},
|
||||
getFliptVariant: vi.fn(async () => 'off'),
|
||||
}));
|
||||
vi.mock('~/server/redis/caches', () => ({
|
||||
imageMetaCache: { fetch: vi.fn(async () => ({})) },
|
||||
}));
|
||||
vi.mock('~/client-utils/edge-url', () => ({ getEdgeUrl: vi.fn(() => 'https://edge/x') }));
|
||||
|
||||
const feedSearch = vi.fn(async () => ({ items: [] as unknown[], nextCursor: undefined }));
|
||||
const legacySearch = vi.fn(async () => ({ items: [] as unknown[], nextCursor: undefined }));
|
||||
vi.mock('~/server/services/image.service', () => ({
|
||||
getAllImages: (...args: unknown[]) => legacySearch(...(args as [])),
|
||||
getAllImagesIndex: vi.fn(async () => ({ items: [], nextCursor: undefined })),
|
||||
getImagesFromFeedSearch: (...args: unknown[]) => feedSearch(...(args as [])),
|
||||
}));
|
||||
|
||||
import { runImageSearch } from '../image-search.service';
|
||||
|
||||
const req = {
|
||||
headers: { 'user-agent': 'probe/1.0' },
|
||||
socket: { remoteAddress: '203.0.113.7' },
|
||||
} as unknown as NextApiRequest;
|
||||
|
||||
function itemWithUsername(username: unknown) {
|
||||
return {
|
||||
id: 1446527,
|
||||
url: 'abc',
|
||||
hash: 'h',
|
||||
width: 512,
|
||||
height: 768,
|
||||
nsfwLevel: 1,
|
||||
type: 'image',
|
||||
createdAt: new Date(0),
|
||||
postId: 369748,
|
||||
stats: {},
|
||||
user: { id: 4768, username },
|
||||
baseModel: 'SD 1.5',
|
||||
modelVersionIds: [],
|
||||
tags: [],
|
||||
};
|
||||
}
|
||||
|
||||
/** Drive the REAL service over the feed (non-legacy) branch. */
|
||||
async function usernameFromFeed(raw: unknown) {
|
||||
feedSearch.mockResolvedValue({ items: [itemWithUsername(raw)], nextCursor: undefined });
|
||||
const { items } = await runImageSearch(
|
||||
{ limit: 1, withMeta: false, withTags: false, data: {} } as never,
|
||||
{ browsingLevel: 1, user: undefined, req } as never
|
||||
);
|
||||
return items[0].username;
|
||||
}
|
||||
|
||||
/** Drive the REAL service over the legacy (`?imageId=`) branch. */
|
||||
async function usernameFromLegacy(raw: unknown) {
|
||||
legacySearch.mockResolvedValue({ items: [itemWithUsername(raw)], nextCursor: undefined });
|
||||
const { items } = await runImageSearch(
|
||||
{ limit: 1, withMeta: false, withTags: false, data: { imageId: 1446527 } } as never,
|
||||
{ browsingLevel: 1, user: undefined, req } as never
|
||||
);
|
||||
return items[0].username;
|
||||
}
|
||||
|
||||
describe('runImageSearch: the published type of `username`', () => {
|
||||
beforeEach(() => {
|
||||
feedSearch.mockClear();
|
||||
legacySearch.mockClear();
|
||||
});
|
||||
|
||||
it('POSITIVE CONTROL: the harness reaches both branches and can tell them apart', async () => {
|
||||
await usernameFromFeed('alice');
|
||||
expect(feedSearch).toHaveBeenCalledTimes(1);
|
||||
expect(legacySearch).not.toHaveBeenCalled();
|
||||
|
||||
await usernameFromLegacy('bob');
|
||||
expect(legacySearch).toHaveBeenCalledTimes(1);
|
||||
expect(feedSearch).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it('emits a string when the feed hands it a number', async () => {
|
||||
const username = await usernameFromFeed(2428023993);
|
||||
expect(username).toBe('2428023993');
|
||||
expect(typeof username).toBe('string');
|
||||
});
|
||||
|
||||
it('CANNOT restore a name the cache already destroyed — this belt is not the fix', async () => {
|
||||
// `0222` reached the wire as `222` because the cache decoded the stored text
|
||||
// as a number. Casting at this boundary produces a well-typed 200 carrying a
|
||||
// name that matches no account, which is why the cache fix is the real one.
|
||||
const username = await usernameFromFeed(222);
|
||||
expect(username).toBe('222');
|
||||
expect(username).not.toBe('0222');
|
||||
});
|
||||
|
||||
it('leaves an ordinary string untouched', async () => {
|
||||
expect(await usernameFromFeed('0222')).toBe('0222');
|
||||
expect(await usernameFromLegacy('2428023993')).toBe('2428023993');
|
||||
});
|
||||
|
||||
it('does not turn a null username into the string "null"', async () => {
|
||||
// The legacy Prisma branch can carry a null username for a deleted account.
|
||||
// A bare `String(...)` here would publish `'null'` as somebody's name.
|
||||
expect(await usernameFromLegacy(null)).toBeNull();
|
||||
expect(await usernameFromFeed(undefined)).toBeUndefined();
|
||||
});
|
||||
});
|
||||
@@ -252,7 +252,24 @@ export async function runImageSearch(
|
||||
const useFlat = flatMeta !== undefined ? flatMeta : !useLegacyMethod;
|
||||
return useFlat ? imageMeta : { id: image.id, meta: imageMeta };
|
||||
})(),
|
||||
username: image.user.username,
|
||||
// `/api/v1/images` publishes `username` as a STRING, and a typed client
|
||||
// fails to decode the whole 200 when it is not one — which is how #4768
|
||||
// was reported (civitai/cli#513: "cannot unmarshal number into Go struct
|
||||
// field .items.username of type string").
|
||||
//
|
||||
// The real repair is upstream, in the feed's entity cache, which used to
|
||||
// infer a field's type from its stored text and so read the all-digit
|
||||
// username `0222` back as the number `222`
|
||||
// (`event-engine-common/caches/value-codec.ts`). This is a belt on the
|
||||
// published contract, nothing more: it can keep a number from reaching the
|
||||
// wire, but it CANNOT recover a name the cache already lost — `222` casts
|
||||
// to `'222'`, which is still not `'0222'`. Do not read it as the fix.
|
||||
//
|
||||
// Only a `number` is coerced. `String()` on its own would be a regression:
|
||||
// the legacy DB branch can carry a null username for a deleted account,
|
||||
// and `String(null)` is the four-character string `'null'`.
|
||||
username:
|
||||
typeof image.user.username === 'number' ? String(image.user.username) : image.user.username,
|
||||
baseModel: image.baseModel,
|
||||
modelVersionIds: image.modelVersionIds,
|
||||
tags: withTags ? image.tags?.map((t) => ({ id: t.id, name: t.name })) ?? [] : undefined,
|
||||
|
||||
Reference in New Issue
Block a user