Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions packages/client/lib/commands/XTRIM.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,12 @@ describe('XTRIM', () => {
);
});

it('with LIMIT', () => {
it('with LIMIT without strategyModifier ~', () => {
assert.deepEqual(
parseArgs(XTRIM, 'key', 'MAXLEN', 1, {
LIMIT: 1
}),
['XTRIM', 'key', 'MAXLEN', '1', 'LIMIT', '1']
['XTRIM', 'key', 'MAXLEN', '1']
);
});

Expand All @@ -53,13 +53,13 @@ describe('XTRIM', () => {
);
});

it('with strategyModifier, LIMIT', () => {
it('with strategyModifier ~, LIMIT', () => {
Comment thread
cursor[bot] marked this conversation as resolved.
assert.deepEqual(
parseArgs(XTRIM, 'key', 'MAXLEN', 1, {
strategyModifier: '=',
LIMIT: 1
parseArgs(XTRIM, 'key', 'MINID', 5, {
strategyModifier: '~',
LIMIT: 2
}),
['XTRIM', 'key', 'MAXLEN', '=', '1', 'LIMIT', '1']
['XTRIM', 'key', 'MINID', '~', '5', 'LIMIT', '2']
);
});

Expand Down
28 changes: 21 additions & 7 deletions packages/client/lib/commands/XTRIM.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,20 +3,34 @@ import { NumberReply, Command, RedisArgument } from '../RESP/types';
import { StreamDeletionPolicy } from './common-stream.types';

/**
* Options for the XTRIM command
*
* @property strategyModifier - Exact ('=') or approximate ('~') trimming
* Options for exact XTRIM trimming
*
* @property strategyModifier - Exact ('=') trimming
* @property policy - Policy to apply when deleting entries (optional, defaults to KEEPREF)
*/
export interface XTrimExactOptions {
strategyModifier?: '=';
/** added in 8.2 */
policy?: StreamDeletionPolicy;
Comment on lines +11 to +14

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Exclude LIMIT from the exact union branch

Because XTrimExactOptions does not declare LIMIT?: never and shares policy with the approximate branch, TypeScript accepts an invalid value such as { LIMIT: 10, policy: STREAM_DELETION_POLICY.KEEPREF } as XTrimOptions; excess-property checking sees LIMIT elsewhere in the union while structural assignability succeeds through the exact branch. Thus the advertised compile-time guarantee fails whenever policy is also supplied, and the parser then silently discards the accepted LIMIT.

Useful? React with 👍 / 👎.

}

/**
* Options for approximate XTRIM trimming
*
* @property strategyModifier - Approximate ('~') trimming, required for LIMIT
* @property LIMIT - Maximum number of entries to trim in one call (Redis 6.2+)
* @property policy - Policy to apply when deleting entries (optional, defaults to KEEPREF)
*/
export interface XTrimOptions {
strategyModifier?: '=' | '~';
export interface XTrimApproximateOptions {
strategyModifier: '~';
/** added in 6.2 */
LIMIT?: number;
/** added in 8.2 */
policy?: StreamDeletionPolicy;
}

export type XTrimOptions = XTrimExactOptions | XTrimApproximateOptions;

/**
* Command for trimming a stream to a specified length or minimum ID
*/
Expand All @@ -32,13 +46,13 @@ export default {
parser.pushKey(key);
parser.push(strategy);

if (options?.strategyModifier) {
if (options?.strategyModifier !== undefined) {
parser.push(options.strategyModifier);
}

parser.push(threshold.toString());

if (options?.LIMIT !== undefined) {
if (options?.strategyModifier === '~' && options.LIMIT !== undefined) {
parser.push('LIMIT', options.LIMIT.toString());
Comment on lines +55 to 56

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Reject LIMIT instead of silently dropping it

When an invalid options object reaches this runtime path—such as from JavaScript, any, or the structurally accepted typed case below—the new condition silently omits LIMIT rather than preserving the previous server error. Redis then executes a valid exact XTRIM, potentially deleting entries even though the caller requested bounded work; this also breaks the unchanged XTRIM.spec.ts “with LIMIT” parser assertion. Either emit LIMIT so Redis rejects it or throw locally instead of changing the command's meaning.

Useful? React with 👍 / 👎.

}

Expand Down
20 changes: 20 additions & 0 deletions packages/client/types-tests/xtrim-limit.types-test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/**
* Compile-time regression: XTRIM's LIMIT is only accepted by the server
* together with the approximate modifier ("syntax error, LIMIT cannot be used
* without the special ~ option", t_stream.c), so the options type must not
* allow LIMIT without strategyModifier '~'.
*
* Lives outside `lib/` so it is not picked up by the production build /
* typedoc. Checked with `npm run test:types -w @redis/client`.
*/
import { XTrimOptions } from '../lib/commands/XTRIM';

export function xtrimLimitRequiresApproximateModifier(): void {
// LIMIT without '~' is rejected by the server.
// @ts-expect-error LIMIT requires strategyModifier '~'
const invalid: XTrimOptions = { LIMIT: 10 };

const valid: XTrimOptions = { strategyModifier: '~', LIMIT: 10 };

console.log(invalid, valid);
}