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

describer: multiSchema-aware SQL Server #3465

Merged
merged 1 commit into from Dec 6, 2022

Conversation

pimeys
Copy link
Contributor

@pimeys pimeys commented Dec 5, 2022

This adds namespaces to SQL Server describer to be used in further work in migrations and introspection.

Due to SQL Server not allowing us to pass a vector as a statement parameter such as PostgreSQL does, we considered a few options:

  • Concatenating the schemas to the query string, this might lead to SQL injection.
  • Doing a trick with a stored procedure and JSON, which would complicate the code a lot.
  • Concatenating parameter variables based on the count of schemas, and concatenating them to the query params. Again, complicates code a lot.
  • Query per schema separately. Slows everything down, more IO, more queries.
  • Query everything, and filter the needed parts in memory. <- we took this approach.

Part of: prisma/prisma#16627 and prisma/prisma#15584

@pimeys pimeys added this to the 4.8.0 milestone Dec 5, 2022
@pimeys pimeys requested a review from a team as a code owner December 5, 2022 16:03
@pimeys pimeys force-pushed the describer/sql-server-multi-schema branch from f90407c to 793b1ef Compare December 5, 2022 16:20
Copy link
Contributor

@eviefp eviefp left a comment

Choose a reason for hiding this comment

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

looks good to me

Comment on lines +406 to +421
//cross schema
// fks
// enums

//Edge cases
//name conflicts
// what if the names are used somewhere???
// table
// enum
//invalid names
// schema
// table
// enum
// re-introspection
// table
// enum
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be here? If yes, could you elaborate such that it makes sense for everyone?

Or maybe it needs to be a GH issue or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was just copied from the other file. I do not know should it be here, but also didn't want to delete it.

@pimeys pimeys merged commit e88b5dd into main Dec 6, 2022
@pimeys pimeys deleted the describer/sql-server-multi-schema branch December 6, 2022 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants