fix(redis): version detection logic #11815

ibrahimmenem · 2025-12-02T11:32:16Z

fix bug with redis version detection logic

cherry-pick changes from #11767
fixes #11635
fixes #11749
close #11767
close #11764

Description of change

instead of using heuristics based on functions signatures we are reading the version from node modules
added 2 unit tests

Pull-Request Checklist

Code is up-to-date with the master branch
This pull request links relevant issues as Fixes getFromCache method throws error #11635 Bug in detectRedisVersion #11749
There are new or updated unit tests validating the change
Documentation has been updated to reflect this change

Refs: 1749

qodo-free-for-open-source-projects · 2025-12-02T11:32:45Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ✅ 11635 - PR Code Verified Compliant requirements: Fix the Redis version detection mechanism that was causing incorrect API usage Ensure proper detection of Redis versions to use correct API (callback-based vs Promise-based) Requires further human verification: Verify that the fix resolves the "Cannot read properties of undefined (reading 'then')" error in actual NestJS applications Test that `dataSource.queryResultCache.getFromCache` works correctly with the new version detection 11749 - PR Code Verified Compliant requirements: Replace the heuristic-based version detection with reading version from package.json Correctly detect Redis 3.x and set redisMajorVersion to 3 Fix the issue where Redis 3.1.2 was incorrectly detected as version 5 Requires further human verification: Test with actual Redis 3.1.2 installation to verify query caching works Verify the fix works with the specific configuration mentioned in the ticket
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review Error Handling The `readPackageVersion` method throws a generic `TypeError` when it fails to read package.json. Consider whether this is the appropriate error type, or if a more specific error (like `TypeORMError`) would be better for consistency with the rest of the codebase. throw new TypeError( `Failed to read package.json for "${name}": ${err.message}`, ) Version Parsing The version parsing logic splits on "." and takes the first element, but doesn't handle edge cases like pre-release versions (e.g., "5.0.0-beta.1") or versions with additional metadata. While `isNaN` check is present, consider if more robust semver parsing is needed. const major = parseInt(version.split(".")[0], 10) if (isNaN(major)) { throw new TypeORMError(`Invalid Redis version format: ${version}`) }

qodo-free-for-open-source-projects · 2025-12-02T11:33:52Z

PR Code Suggestions ✨

Latest suggestions up to c2aba88

Category	Suggestion	Impact
Possible issue	Validate version field exists and is string In `readPackageVersion`, validate that the `version` field from `package.json` exists and is a string before returning it. src/platform/PlatformTools.ts [34-42] static readPackageVersion(name: string): string { try { - return require(`${name}/package.json`).version + const version = require(`${name}/package.json`).version + if (typeof version !== 'string') { + throw new TypeError(`Package "${name}" does not have a valid version field`) + } + return version } catch (err) { throw new TypeError( `Failed to read package.json for "${name}": ${err.message}`, ) } } Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that `require(...).version` can be `undefined` if the `version` field is missing in `package.json`, and proposes adding validation to handle this, making the function more robust.	Medium
Possible issue	Validate version string before parsing Validate that the `version` returned from `PlatformTools.readPackageVersion` is a non-empty string before attempting to parse it. src/cache/RedisQueryResultCache.ts [308-312] const version = PlatformTools.readPackageVersion("redis") +if (!version \|\| typeof version !== 'string') { + throw new TypeORMError(`Invalid Redis version: version is empty or not a string`) +} const major = parseInt(version.split(".")[0], 10) if (isNaN(major)) { throw new TypeORMError(`Invalid Redis version format: ${version}`) } Apply / Chat Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies that `readPackageVersion` might return a non-string or empty value, and adds a defensive check to handle this case with a clearer error message.	Low
More

Previous suggestions

Suggestions up to commit f26ac5f

Category	Suggestion	Impact
Possible issue	Handle unknown error type safely In `readPackageVersion`, safely access the error message from the `catch` block by checking if the caught error is an instance of `Error` before accessing its `message` property. This prevents potential runtime errors. src/platform/PlatformTools.ts [34-42] static readPackageVersion(name: string): string { try { return require(`${name}/package.json`).version } catch (err) { + const message = err instanceof Error ? err.message : String(err) throw new TypeError( - `Failed to read package.json for "${name}": ${err.message}`, + `Failed to read package.json for "${name}": ${message}`, ) } } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that accessing `err.message` is unsafe because `err` is of type `unknown`. The proposed fix using `instanceof Error` is a robust way to prevent potential runtime errors, improving code quality and resilience.	Low

Suggestions up to commit cbc41cf

Category	Suggestion	Impact
Possible issue	Correct Redis version detection logic Correct the Redis version check from `major <= 4` to `major < 4` to properly handle the Promise-based API introduced in `redis` v4. src/cache/RedisQueryResultCache.ts [313-319] -if (major <= 4) { - // Redis 3/4 uses callback-based API +if (major < 4) { + // Redis <4 uses callback-based API this.redisMajorVersion = 3 } else { - // Redis 5+ uses Promise-based API + // Redis 4+ uses Promise-based API this.redisMajorVersion = 5 } Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a critical bug in the PR's logic, as `redis` v4 uses a Promise-based API, and the current code would cause runtime failures.	High
Possible issue	Correct an inaccurate unit test Update the unit test for `redis` v4.x to assert that `redisMajorVersion` is 5 (Promise-based), not 3, to align with the package's actual API. test/unit/cache/redis-query-result-cache.ts [55-66] -it("should detect Redis v4.x and set redisMajorVersion to 3 (callback-based)", () => { +it("should detect Redis v4.x and set redisMajorVersion to 5 (Promise-based)", () => { readPackageVersionStub.returns("4.6.13") const cache = new RedisQueryResultCache( mockDataSource as any, "redis", ) ;(cache as any).detectRedisVersion() - expect((cache as any).redisMajorVersion).to.equal(3) + expect((cache as any).redisMajorVersion).to.equal(5) }) Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies that a new unit test is based on the flawed logic from the PR, and fixing it is crucial for ensuring the correctness of the feature.	High
High-level	Consider non-Node.js environment compatibility The new version detection uses `require`, which is Node.js-specific. To maintain compatibility with non-Node.js environments, consider adding a platform check and falling back to the old heuristic or a safe default. Examples: src/platform/PlatformTools.ts [34-42] static readPackageVersion(name: string): string { try { return require(`${name}/package.json`).version } catch (err) { throw new TypeError( `Failed to read package.json for "${name}": ${err.message}`, ) } } src/cache/RedisQueryResultCache.ts [306-319] private detectRedisVersion(): void { if (this.clientType !== "redis") return const version = PlatformTools.readPackageVersion("redis") const major = parseInt(version.split(".")[0], 10) if (isNaN(major)) { throw new TypeORMError(`Invalid Redis version format: ${version}`) } if (major <= 4) { // Redis 3/4 uses callback-based API this.redisMajorVersion = 3 ... (clipped 4 lines) Solution Walkthrough: Before: // src/cache/RedisQueryResultCache.ts private detectRedisVersion(): void { if (this.clientType !== "redis") return; const version = PlatformTools.readPackageVersion("redis"); const major = parseInt(version.split(".")[0], 10); if (isNaN(major)) { throw new TypeORMError(`Invalid Redis version format: ${version}`); } if (major <= 4) { this.redisMajorVersion = 3; } else { this.redisMajorVersion = 5; } } After: // src/cache/RedisQueryResultCache.ts private detectRedisVersion(): void { if (this.clientType !== "redis") return; // Node.js-specific, reliable check if (PlatformTools.type === "node") { try { const version = PlatformTools.readPackageVersion("redis"); const major = parseInt(version.split(".")[0], 10); this.redisMajorVersion = major <= 4 ? 3 : 5; return; } catch (e) { // Fallback to heuristic if package.json is unreadable } } // Fallback for non-Node.js or if the above fails this.redisMajorVersion = 3; // Safe default } Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies that the new `require`-based logic introduces a Node.js-specific dependency, which is a potential regression for a multi-platform library like TypeORM and could cause runtime errors in other environments.	Medium
Security	Add validation to prevent path injection Add input validation to the `readPackageVersion` function to prevent potential path injection vulnerabilities from path traversal characters in the package `name`. src/platform/PlatformTools.ts [34-42] static readPackageVersion(name: string): string { + if (!name \|\| name.includes("..") \|\| name.includes("/") \|\| name.includes("\\")) { + throw new TypeError(`Invalid package name: "${name}"`) + } try { return require(`${name}/package.json`).version } catch (err) { throw new TypeError( `Failed to read package.json for "${name}": ${err.message}`, ) } } Suggestion importance[1-10]: 6 __ Why: The suggestion proposes a valid security hardening for a new public utility function, preventing potential path injection, although the current usage is not vulnerable.	Low

pkg-pr-new · 2025-12-02T11:35:23Z

typeorm-sql-js-example

npm i https://pkg.pr.new/typeorm/typeorm@11815

commit: c2aba88

coveralls · 2025-12-02T12:03:28Z

coverage: 80.771% (-0.002%) from 80.773%
when pulling c2aba88 on ibrahimmenem:fix-redis-version-detection-logic
into 38715bb on typeorm:master.

gioboa

It looks good to me 👏
is the ; at the beginning intentional ;(cache as any).detectRedisVersion() ?

ibrahimmenem · 2025-12-02T16:19:07Z

is the ; at the beginning intentional ;(cache as any).detectRedisVersion() ?

to avoid typescript interpreting it as a call of the previous line RedisQueryResultCache

mguida22

This fix looks good, thanks for your work on it @ibrahimmenem.

I still think it's very confusing and error prone to set redisMajorVersion to either 3 or 5, and not the exact major version number. In a future PR we should set this to the proper version update code that uses it to treat 3/4 as the same.

gioboa · 2025-12-02T19:41:19Z

to avoid typescript interpreting it as a call of the previous line RedisQueryResultCache

Thanks for the clarification 🙏

Co-authored-by: AdolfodelSel <adolfo.selllano@gmail.com> Co-authored-by: Giorgio Boa <35845425+gioboa@users.noreply.github.com> Co-authored-by: Mike Guida <mike@mguida.com>

AdolfodelSel and others added 3 commits December 2, 2025 11:35

fix: Bug in detectRedisVersion

feb9039

Refs: 1749

chore(redis): update comments in redis detection logic

74bd747

chore(redis): add unit tests for redis version checks

cbc41cf

qodo-free-for-open-source-projects bot added the Review effort 2/5 label Dec 2, 2025

ibrahimmenem mentioned this pull request Dec 2, 2025

fix(redis): version detection #11767

Closed

4 tasks

gioboa approved these changes Dec 2, 2025

View reviewed changes

Merge branch 'master' into fix-redis-version-detection-logic

f26ac5f

gioboa requested review from G0maa and alumni December 2, 2025 14:40

mguida22 changed the title ~~Fix redis version detection logic~~ fix(redis): version detection logic Dec 2, 2025

mguida22 approved these changes Dec 2, 2025

View reviewed changes

Merge branch 'master' into fix-redis-version-detection-logic

c2aba88

mguida22 merged commit 6f486e5 into typeorm:master Dec 2, 2025
64 checks passed

mguida22 mentioned this pull request Dec 15, 2025

PlatformTools.readPackageVersion throws an error in NextJs #11846

Open

18 tasks

alumni mentioned this pull request Dec 15, 2025

fix(redis): read version from package #11853

Draft

4 tasks

Minishlink mentioned this pull request Jan 12, 2026

fix(postgres): handle timestamptz correctly #11916

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix(redis): version detection logic #11815

fix(redis): version detection logic #11815

ibrahimmenem commented Dec 2, 2025

Uh oh!

qodo-free-for-open-source-projects bot commented Dec 2, 2025

Uh oh!

qodo-free-for-open-source-projects bot commented Dec 2, 2025 •

edited

Loading

Examples:

Solution Walkthrough:

Before:

After:

Uh oh!

pkg-pr-new bot commented Dec 2, 2025 •

edited

Loading

Uh oh!

coveralls commented Dec 2, 2025 •

edited

Loading

Uh oh!

gioboa left a comment

Uh oh!

ibrahimmenem commented Dec 2, 2025

Uh oh!

mguida22 left a comment

Uh oh!

Uh oh!

gioboa commented Dec 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

fix(redis): version detection logic #11815

fix(redis): version detection logic #11815

Conversation

ibrahimmenem commented Dec 2, 2025

Description of change

Pull-Request Checklist

Uh oh!

qodo-free-for-open-source-projects bot commented Dec 2, 2025

PR Reviewer Guide 🔍

Uh oh!

qodo-free-for-open-source-projects bot commented Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Previous suggestions

Examples:

Solution Walkthrough:

Before:

After:

Uh oh!

pkg-pr-new bot commented Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gioboa left a comment

Choose a reason for hiding this comment

Uh oh!

ibrahimmenem commented Dec 2, 2025

Uh oh!

mguida22 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gioboa commented Dec 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

qodo-free-for-open-source-projects bot commented Dec 2, 2025 •

edited

Loading

pkg-pr-new bot commented Dec 2, 2025 •

edited

Loading

coveralls commented Dec 2, 2025 •

edited

Loading