Skip to content

fix: fixes caching for nested objects and adds tests #3873

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix: fixes caching for nested objects and adds tests #3873

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e515e55
fixes caching for nested objects and adds tests
konstantinabl Jun 23, 2025
bc1e426
Merge branch 'main' into 3872-objects-in-params-are-not-serialized-wh…
konstantinabl Jun 24, 2025
02a5f93
Merge branch 'main' into 3872-objects-in-params-are-not-serialized-wh…
konstantinabl Jun 24, 2025
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
4 changes: 3 additions & 1 deletion packages/relay/src/lib/decorators/cache.decorator.ts
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

We can improve the type of generateCacheKey by using unknown

const generateCacheKey = (methodName: string, args: IArgument[]) => {
const generateCacheKey = (methodName: string, args: unknown[]) => {

Please note that this is not strictly about the issue mentioned in the PR. However, given that in the tests (see below) we type casting to use this function, maybe we can change it here.

Also, we can change the outer loop from

  for (const [, value] of Object.entries(args)) {

to

  for (const value of args) {

right?

Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,9 @@ const generateCacheKey = (methodName: string, args: IArgument[]) => {
if (value?.constructor?.name != 'RequestDetails') {
if (value && typeof value === 'object') {
for (const [key, innerValue] of Object.entries(value)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

So, now that we are using JSON.stringify, this loop might not be needed, right?

cacheKey += `_${key}_${innerValue}`;
const serializedValue =
typeof innerValue === 'object' && innerValue !== null ? JSON.stringify(innerValue) : innerValue;
cacheKey += `_${key}_${serializedValue}`;
}
continue;
}
Expand Down
125 changes: 103 additions & 22 deletions packages/relay/tests/lib/decorators/cache.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,13 +107,13 @@ describe('cache decorator', () => {

describe('shouldSkipCachingForSingleParams', () => {
it('should return false if no skip rules are provided', () => {
const args = ['safe', 'latest'] as unknown as IArguments;
const args = ['safe', 'latest'] as unknown as IArguments[];
const result = __test__.__private.shouldSkipCachingForSingleParams(args, []);
expect(result).to.be.false;
});

it('should return false if argument exists but is not in skip values', () => {
const args = ['latest', 'earliest'] as unknown as IArguments;
const args = ['latest', 'earliest'] as unknown as IArguments[];
const params = [
{ index: '0', value: 'pending' },
{ index: '1', value: 'safe|finalized' },
Expand All @@ -123,21 +123,21 @@ describe('cache decorator', () => {
});

it('should return true if a param at index matches any value in the pipe-separated list', () => {
const args = ['earliest', 'safe'] as unknown as IArguments;
const args = ['earliest', 'safe'] as unknown as IArguments[];
const params = [{ index: '1', value: 'pending|safe' }];
const result = __test__.__private.shouldSkipCachingForSingleParams(args, params);
expect(result).to.be.true;
});

it('should return true if the argument at index is missing (do not cache optional parameters)', () => {
const args = ['latest'] as unknown as IArguments;
const args = ['latest'] as unknown as IArguments[];
const params = [{ index: '1', value: 'pending|safe' }];
const result = __test__.__private.shouldSkipCachingForSingleParams(args, params);
expect(result).to.be.true;
});

it('should return true if the argument at index is explicitly undefined', () => {
const args = ['finalized', undefined] as unknown as IArguments;
const args = ['finalized', undefined] as unknown as IArguments[];
const params = [{ index: '1', value: 'pending|safe' }];
const result = __test__.__private.shouldSkipCachingForSingleParams(args, params);
expect(result).to.be.true;
Expand All @@ -146,13 +146,13 @@ describe('cache decorator', () => {

describe('shouldSkipCachingForNamedParams', () => {
it('should return false when no rules are provided', () => {
const args = [{ fromBlock: 'safe' }] as unknown as IArguments;
const args = [{ fromBlock: 'safe' }] as unknown as IArguments[];
const result = __test__.__private.shouldSkipCachingForNamedParams(args, []);
expect(result).to.be.false;
});

it('should return false if the field value does not match skip values', () => {
const args = [{ fromBlock: 'confirmed' }] as unknown as IArguments;
const args = [{ fromBlock: 'confirmed' }] as unknown as IArguments[];
const params = [
{
index: '0',
Expand All @@ -164,7 +164,7 @@ describe('cache decorator', () => {
});

it('should return false if none of the multiple fields match', () => {
const args = [{ fromBlock: 'finalized', toBlock: 'earliest' }] as unknown as IArguments;
const args = [{ fromBlock: 'finalized', toBlock: 'earliest' }] as unknown as IArguments[];
const params = [
{
index: '0',
Expand All @@ -179,7 +179,7 @@ describe('cache decorator', () => {
});

it('should return true if a field matches one of the skip values', () => {
const args = [{ fromBlock: 'pending' }] as unknown as IArguments;
const args = [{ fromBlock: 'pending' }] as unknown as IArguments[];
const params = [
{
index: '0',
Expand All @@ -191,7 +191,7 @@ describe('cache decorator', () => {
});

it('should return true if multiple fields are specified and one matches', () => {
const args = [{ fromBlock: 'earliest', toBlock: 'latest' }] as unknown as IArguments;
const args = [{ fromBlock: 'earliest', toBlock: 'latest' }] as unknown as IArguments[];
const params = [
{
index: '0',
Expand All @@ -208,21 +208,21 @@ describe('cache decorator', () => {

describe('generateCacheKey', () => {
it('should return only the method name when args are empty', () => {
const args = [] as unknown as IArguments;
const args = [] as unknown as IArguments[];
Copy link
Contributor

Choose a reason for hiding this comment

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

If we apply the suggestion left in cache.decorator.ts comment, we can avoid this casts.


const result = __test__.__private.generateCacheKey('eth_getBalance', args);
expect(result).to.equal('eth_getBalance');
});

it('should append primitive arguments to the cache key', () => {
const args = ['0xabc', 'latest'] as unknown as IArguments;
const args = ['0xabc', 'latest'] as unknown as IArguments[];

const result = __test__.__private.generateCacheKey('eth_getBalance', args);
expect(result).to.equal('eth_getBalance_0xabc_latest');
});

it('should append object key-value pairs to the cache key', () => {
const args = [{ fromBlock: 'earliest', toBlock: 5644 }] as unknown as IArguments;
const args = [{ fromBlock: 'earliest', toBlock: 5644 }] as unknown as IArguments[];

const result = __test__.__private.generateCacheKey('eth_getLogs', args);
expect(result).to.equal('eth_getLogs_fromBlock_earliest_toBlock_5644');
Expand All @@ -233,54 +233,135 @@ describe('cache decorator', () => {
constructor: { name: 'RequestDetails' },
someField: 'shouldBeIgnored',
};
const args = [mockRequestDetails, 'earliest'] as unknown as IArguments;
const args = [mockRequestDetails, 'earliest'] as unknown as IArguments[];

const result = __test__.__private.generateCacheKey('eth_call', args);
expect(result).to.equal('eth_call_earliest');
});

it('should not skip null or undefined args', () => {
const args = [undefined, null, 'pending'] as unknown as IArguments;
const args = [undefined, null, 'pending'] as unknown as IArguments[];

const result = __test__.__private.generateCacheKey('eth_call', args);
expect(result).to.equal('eth_call_undefined_null_pending');
});

it('should process multiple arguments correctly', () => {
const args = [{ fromBlock: '0xabc' }, 5644, 'safe'] as unknown as IArguments;
const args = [{ fromBlock: '0xabc' }, 5644, 'safe'] as unknown as IArguments[];

const result = __test__.__private.generateCacheKey('eth_getLogs', args);
expect(result).to.equal('eth_getLogs_fromBlock_0xabc_5644_safe');
});

it('should work with mixed types including booleans and numbers', () => {
const args = [true, 42, { fromBlock: 'safe' }] as unknown as IArguments;
const args = [true, 42, { fromBlock: 'safe' }] as unknown as IArguments[];

const result = __test__.__private.generateCacheKey('custom_method', args);
expect(result).to.equal('custom_method_true_42_fromBlock_safe');
});

it('should serialize nested objects using JSON.stringify', () => {
const args = [
{
tracer: 'callTracer',
tracerConfig: { onlyTopCall: true },
},
] as unknown as IArguments[];

const result = __test__.__private.generateCacheKey('debug_traceTransaction', args);
expect(result).to.equal('debug_traceTransaction_tracer_callTracer_tracerConfig_{"onlyTopCall":true}');
});

it('should differentiate between different nested object values', () => {
const args1 = [
{
tracer: 'callTracer',
tracerConfig: { onlyTopCall: true },
},
] as unknown as IArguments[];

const args2 = [
{
tracer: 'callTracer',
tracerConfig: { onlyTopCall: false },
},
] as unknown as IArguments[];

const result1 = __test__.__private.generateCacheKey('debug_traceTransaction', args1);
const result2 = __test__.__private.generateCacheKey('debug_traceTransaction', args2);

expect(result1).to.equal('debug_traceTransaction_tracer_callTracer_tracerConfig_{"onlyTopCall":true}');
expect(result2).to.equal('debug_traceTransaction_tracer_callTracer_tracerConfig_{"onlyTopCall":false}');
expect(result1).to.not.equal(result2);
});

it('should handle nested objects with multiple properties', () => {
const args = [
{
config: {
timeout: 5000,
retries: 3,
options: { debug: true },
},
},
] as unknown as IArguments[];

const result = __test__.__private.generateCacheKey('test_method', args);
expect(result).to.equal('test_method_config_{"timeout":5000,"retries":3,"options":{"debug":true}}');
});

it('should handle nested arrays in objects', () => {
const args = [
{
filter: {
addresses: ['0x123', '0x456'],
topics: [null, '0xabc'],
},
},
] as unknown as IArguments[];

const result = __test__.__private.generateCacheKey('eth_getLogs', args);
expect(result).to.equal('eth_getLogs_filter_{"addresses":["0x123","0x456"],"topics":[null,"0xabc"]}');
});

it('should handle deeply nested objects', () => {
const args = [
{
level1: {
level2: {
level3: {
value: 'deep',
},
},
},
},
] as unknown as IArguments[];

const result = __test__.__private.generateCacheKey('deep_method', args);
expect(result).to.equal('deep_method_level1_{"level2":{"level3":{"value":"deep"}}}');
});
});

describe('extractRequestDetails', () => {
it('should return the RequestDetails instance if found in args', () => {
const requestDetails = new RequestDetails({ requestId: 'abc123', ipAddress: '127.0.0.1' });
const args = [5644, requestDetails, 'other'] as unknown as IArguments;
const args = [5644, requestDetails, 'other'] as unknown as IArguments[];

const result = __test__.__private.extractRequestDetails(args);
expect(result.requestId).to.equal('abc123');
expect(result.ipAddress).to.equal('127.0.0.1');
});

it('should return a new default RequestDetails if not found', () => {
const args = [5644, { fromBlock: 'pending' }, 'value'] as unknown as IArguments;
const args = [5644, { fromBlock: 'pending' }, 'value'] as unknown as IArguments[];

const result = __test__.__private.extractRequestDetails(args);
expect(result.requestId).to.equal('');
expect(result.ipAddress).to.equal('');
});

it('should return new RequestDetails when args is empty', () => {
const args = [] as unknown as IArguments;
const args = [] as unknown as IArguments[];

const result = __test__.__private.extractRequestDetails(args);
expect(result.requestId).to.equal('');
Expand All @@ -290,14 +371,14 @@ describe('cache decorator', () => {
it('should return the first RequestDetails instance if multiple are present', () => {
const rd1 = new RequestDetails({ requestId: 'first', ipAddress: '1.1.1.1' });
const rd2 = new RequestDetails({ requestId: 'second', ipAddress: '2.2.2.2' });
const args = [rd1, rd2] as unknown as IArguments;
const args = [rd1, rd2] as unknown as IArguments[];

const result = __test__.__private.extractRequestDetails(args);
expect(result).to.equal(rd1);
});

it('should handle null or undefined values in args', () => {
const args = [undefined, null, 5644] as unknown as IArguments;
const args = [undefined, null, 5644] as unknown as IArguments[];

const result = __test__.__private.extractRequestDetails(args);
expect(result.requestId).to.equal('');
Expand Down
Loading