-
-
Notifications
You must be signed in to change notification settings - Fork 178
Fix Drizzle plugin column mapping for relational queries with missing uniqueName #1559
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.
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
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Co-authored-by: hayes <[email protected]>
Co-authored-by: hayes <[email protected]>
Co-authored-by: hayes <[email protected]>
|
@copilot is uniqueName the correct way to find these definitions? We should always use only 1 way to find this information without having a fallback or diverging paths. Please explore what data is available, and clearly explain which cases can't be resolved without creating custom mappings. If this is the only option, we can keep the napping, but it would need to be the way we always resolve data rather than using a try-catch fallback |
Co-authored-by: hayes <[email protected]>
After exploring the drizzle-orm APIs, I found that
The combination of I've simplified the implementation in commit Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot can you verify that we do actually need these mappings in the first place, and that there is no way to resolve this directly without creating the helper with the full schema mappings? |
Co-authored-by: hayes <[email protected]>
Verified - we do not need the pre-built mappings. The TypeScript property name can be resolved directly from the column object itself: const columnTable = getColumnTable(column);
const tableColumns = getTableColumns(columnTable);
for (const [key, col] of Object.entries(tableColumns)) {
if (col === column) return key;
}This works because the column object reference is identical to what's stored in |
|
@copilot is there a way to entirely eliminate columnToTsName? It looks like having to loop over all columns to find the correct one on each call is very inefficient |
Co-authored-by: hayes <[email protected]>
Unfortunately However, I've optimized it in commit
The |
|
@copilot let's go back to front loading the work, rather than using a weak map, let's pre-compute the mappings, which I think is what was originally happening |
Co-authored-by: hayes <[email protected]>
Updated in commit |
|
@copilot this looks pretty similar to the original implementation, can you minimize the diff to just the fix now that the new and old versions are doing very similar things, and rebase/squash the changes |
…queries Co-authored-by: hayes <[email protected]>
Minimized the diff in commit
I can't squash/rebase the commits since I don't have git push access, but the diff is now minimal. |
| } | ||
|
|
||
| const columnNameMappings: Record<string, string> = {}; | ||
| // Use column object reference as key instead of uniqueName, since uniqueName |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot remove these comments (without creating a separate commit)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the comments in commit 08e3d03.
…queries Co-authored-by: hayes <[email protected]>
|
@copilot squash this into one commit |
I don't have the ability to squash commits - I can only create new commits via the report_progress tool, but cannot rebase, squash, or force push. You'll need to squash the commits when merging the PR (using "Squash and merge") or manually rebase the branch. |
|
@hayes |
|
@shadoworion thanks! I've applied this changes, the getColumns, and some other fixes for drizzle beta 2 on another branch, should be our shortly! |
Fixes the
Column has no uniqueNameerror that occurs when using relational queries with drizzle-orm beta versions.Summary
The original
columnToTsNamefunction relied oncolumn.uniqueNamewhich may not always be set in drizzle-orm. The fix is minimal: use the column object reference itself as the map key instead ofuniqueName.Changes
columnNameMappingsfromRecord<string, string>(keyed byuniqueName) toMap<Column, string>(keyed by column object reference)uniqueNamechecks since we no longer depend on itcolumnToTsNameto a simple Map lookupThis works because column object references are stable and unique within a schema.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.