Skip to content
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

feat: add schema to TableNodes within RawNodes @ WithSchemaPlugin. #826

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

igalklebanov
Copy link
Member

@igalklebanov igalklebanov commented Dec 31, 2023

closes #761.

When using sql.table, eb.table, sql.ref and eb.ref, we create TableNodes.

When executing sql template tag queries or parts of queries, any plugins on the db instance are run - including WithSchemaPlugin when available.

Currently, the plugin doesn't handle RawNodes, so we only get partial schema addition.

This PR addresses that. Now using .ref and .table is even more powerful. 👓

Copy link

vercel bot commented Dec 31, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
kysely ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 21, 2024 3:31pm

@igalklebanov igalklebanov added enhancement New feature or request built-in plugin Related to a built-in plugin postgres Related to PostgreSQL mssql Related to MS SQL Server (MSSQL) labels Jan 1, 2024
@igalklebanov igalklebanov marked this pull request as ready for review January 1, 2024 00:23
@igalklebanov igalklebanov changed the title feat: add schema to TableNodes within RawNodes @ WIthSchemaPlugin. feat: add schema to TableNodes within RawNodes @ WithSchemaPlugin. Jan 1, 2024
@@ -517,6 +520,72 @@ for (const dialect of DIALECTS.filter(
})
})

describe('raw sql', () => {
Copy link
Member

Choose a reason for hiding this comment

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

I think a little copy-pasta is better than these DRY tests that are really really hard to read. This is fine, but in the future, if there's only a couple of cases, just copy paste them.

Copy link
Member Author

Choose a reason for hiding this comment

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

Absolutely agree, it's a mess.


subsuites.forEach(([name, builder]) => {
describe(`${name}.table`, () => {
it('should add schema', () => {
Copy link
Member

@koskimas koskimas May 25, 2024

Choose a reason for hiding this comment

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

Add a test case like this:

sql`
  select * from person as p
  inner join pets on pets.owner_id = ${sql.ref('p.id')}
`

I think the current code would incorrectly add schema to mammals.p.id 🤔

It might be impossible to handle this correctly since we don't know how ref and table are used in raw SQL.

Copy link
Member Author

Choose a reason for hiding this comment

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

Damn, got some rethinking to do. Great catch! 💪

Maybe if we limit the scope to just eb.ref & eb.table - since it forces usage of things from query context - and find a way to skip aliases (don't remember if I handled it) - this could work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
built-in plugin Related to a built-in plugin enhancement New feature or request mssql Related to MS SQL Server (MSSQL) postgres Related to PostgreSQL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Raw sql execute does not use schema during migration
2 participants