fix: prioritize type overrides over column overrides correctly #423
Conversation
Fix the logic for resolving custom type overrides to ensure that type overrides are applied correctly even when column overrides exist for unrelated columns. Previously, the code returned early when a column override was found, ignoring type overrides that should apply to the column's type. This caused incorrect early exit and improper type resolution. The fix changes the order of checks to first look for a column override for the specific column, and if none is found, then apply the type override for the column's type. Added a test to verify that type overrides are respected even when unrelated column overrides exist.
🦋 Changeset detectedLatest commit: 90bacd2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 17 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
Warning Rate limit exceeded@Newbie012 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 6 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis PR fixes an issue with custom type inference when used with custom column overrides. Changes include a new test case validating the fix, a refactoring of the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/generate/src/generate.ts (1)
496-506: Remove redundant dead code from the refactor.Lines 496-506 duplicate the type override logic already implemented at lines 490-494. This code is unreachable because:
- Line 492 already checks if
typeOverride !== undefinedand returns- Line 496 checks
params.context.overrides?.types === undefined, but this was already implicitly checked at line 490- Line 500 performs the same lookup as line 490
This leftover code should be removed to eliminate confusion and maintain clarity.
Apply this diff to remove the redundant code:
if (typeOverride !== undefined) { return { kind: "type", value: typeOverride.value, type: pgType.name } satisfies ResolvedTarget; } - - if (params.context.overrides?.types === undefined || pgType === undefined) { - return undefined; - } - - const override = params.context.overrides.types.get(pgType.name); - - return fmap( - override, - ({ value }): ResolvedTarget => ({ kind: "type", value, type: pgType.name }), - ); + + return undefined; })();
🧹 Nitpick comments (1)
.changeset/proud-ads-poke.md (1)
1-5: Optional: Consider minor style improvements.The static analysis tool suggests two optional style improvements:
- "fixed" could be capitalized to "Fixed" for more formal wording
- "in conjunction with" could be simplified to "with" for brevity
These are minor style preferences and the current wording is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/proud-ads-poke.md(1 hunks)packages/generate/src/generate-insert.test.ts(1 hunks)packages/generate/src/generate.ts(1 hunks)
🧰 Additional context used
🪛 LanguageTool
.changeset/proud-ads-poke.md
[style] ~4-~4: Consider using a different verb for a more formal wording.
Context: --- "@ts-safeql/generate": patch --- fixed an issue where custom type was inferred...
(FIX_RESOLVE)
[style] ~5-~5: ‘in conjunction with’ might be wordy. Consider a shorter alternative.
Context: ...re custom type was inferred incorrectly in conjunction with custom columns
(EN_WORDINESS_PREMIUM_IN_CONJUNCTION_WITH)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: check (16)
- GitHub Check: check (17)
🔇 Additional comments (1)
packages/generate/src/generate-insert.test.ts (1)
476-495: LGTM! Test case effectively validates the fix.This test case correctly validates that type overrides work properly when unrelated column overrides are configured. The test setup clearly demonstrates the bug scenario: a column override for
tbl2.colshould not prevent a type override fortimestamptzfrom being applied totbl.col.
Summary by CodeRabbit
Bug Fixes
Tests