Skip to content

export the database/sql driver implementation type #79

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

Merged

Conversation

NyaaaWhatsUpDoc
Copy link
Contributor

This allows for further user customization of the database/sql sqlite driver, for example in GoToSocial where we would require translating any returned errors. This does so while not breaking the current API.

@ncruces
Copy link
Owner

ncruces commented Apr 19, 2024

I'm not against this, but it'll take some convincing.
Can you show me how you'd use this, what do you need it for?

The driver was initially a singleton, now it's a global variable. You're assuming it is set on an init?
But what if two client packages need different customizations?

@ncruces
Copy link
Owner

ncruces commented Apr 19, 2024

I think I understand why you might want to export the current singleton: so it can be wrapped.

I can also understand the frustration with me taking over the "sqlite3" driver name. I was always conflicted about it, but decided it was the least bad option (and still added an escape hatch).

I don't understand why the driver would need to stop being a singleton and become a global variable.

@NyaaaWhatsUpDoc
Copy link
Contributor Author

Okay so thoughts for designing it were as follows:

  • exporting the driver type, so as you say, it can be wrapped. in GoToSocial we do this so we can simply translate all the errors at the lowest level and not have to worry about them by the time they've ended up in our ORM (https://github.com/uptrace/bun)

  • making the registered database/sql singleton exported so you can customize it in your own manner and still access it under the registered name "sqlite3, without having to set any linker flags to unset the registration variable name. i was thinking that people not so comfortable with passing linker flags would likely end up opting to register a new instance of the driver under a different name in the sql package, meaning you'd have an unnecessary singleton hanging around. not hugely important but i figured it might be a bit easier this way

i'm not hugely tied to that second point so if you'd prefer i can remove the exported global variable!

@ncruces
Copy link
Owner

ncruces commented Apr 20, 2024

Yes, I would really rather not export the global variable.
But if you have a better idea for driver registration (and me not taking over "sqlite3") I'm all ears.

I can explain why I did it like this (and why I disagree with modernc using "sqlite" instead).
Having multiple SQLite drivers in the same process is not just wasteful but outright dangerous, for reasons I explain here.

So, in a sense, I want to panic if people try to use mattn and my driver in the same process (ideally I'd panic for modernc too).

The counter argument is that all 3 drivers (mattn, modernc, ncruces) have different DSNs, so they should have different registration names.

Ideally, none of the drivers would "auto-register," but that's the Go convention, so that's out.

The only solution I see, is what I did with package embed: a package driver to register, and a package driver/impl (?) that exports the implementation for customization. Would that be better? I can prototype it for you.

Sorry, this is the kind of stuff that I'd really like to get right at once, but that only happens when someone shows up with a weird requirement.

@NyaaaWhatsUpDoc
Copy link
Contributor Author

Yes, I would really rather not export the global variable. But if you have a better idea for driver registration (and me not taking over "sqlite3") I'm all ears.

I can explain why I did it like this (and why I disagree with modernc using "sqlite" instead). Having multiple SQLite drivers in the same process is not just wasteful but outright dangerous, for reasons I explain here.

So, in a sense, I want to panic if people try to use mattn and my driver in the same process (ideally I'd panic for modernc too).

The counter argument is that all 3 drivers (mattn, modernc, ncruces) have different DSNs, so they should have different registration names.

Ideally, none of the drivers would "auto-register," but that's the Go convention, so that's out.

The only solution I see, is what I did with package embed: a package driver to register, and a package driver/impl (?) that exports the implementation for customization. Would that be better? I can prototype it for you.

Sorry, this is the kind of stuff that I'd really like to get right at once, but that only happens when someone shows up with a weird requirement.

no worries that makes sense! will make the change 👍

@NyaaaWhatsUpDoc
Copy link
Contributor Author

@ncruces have removed the global variable, and renamed the driver implementation to instead be SQLite{}

@ncruces ncruces self-assigned this Apr 22, 2024
Copy link
Owner

@ncruces ncruces left a comment

Choose a reason for hiding this comment

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

Thanks! I'll fix some doc nits in a followup; hope you don't mind.

@ncruces ncruces merged commit 189fbc9 into ncruces:main Apr 22, 2024
7 checks passed
@ncruces
Copy link
Owner

ncruces commented Apr 22, 2024

Just a heads up: because I have encryption in the pipeline, it will take me some time to tag a release of this.

I'm planing to make some small backwards incompatible changes to the VFS API in #82, and I really want to get them right. I also want to make sure encryption is works the way it should, as that's a sensitive matter.

If you're not creating a VFS (and I guess no one is, yet), you can use main for a while, then update.
That won't cause any breakages.

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.

2 participants