Don't expose sqlite3 Cache and Statement

[palaviv]

BPO	30262
Nosy	@bitdancer, @berkerpeksag, @palaviv, @izbyshev
PRs	bpo-30262: Don't expose sqlite Cache and Statement  #1440 bpo-30262: Deprecate sqlite Cache and Statement #1573

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2019-05-09.18:10:30.263>
created_at = <Date 2017-05-03.19:52:10.390>
labels = ['extension-modules', 'type-bug', '3.8']
title = "Don't expose sqlite3 Cache and Statement"
updated_at = <Date 2019-05-09.18:10:30.257>
user = 'https://github.com/palaviv'

bugs.python.org fields:

activity = <Date 2019-05-09.18:10:30.257>
actor = 'berker.peksag'
assignee = 'none'
closed = True
closed_date = <Date 2019-05-09.18:10:30.263>
closer = 'berker.peksag'
components = ['Extension Modules']
creation = <Date 2017-05-03.19:52:10.390>
creator = 'palaviv'
dependencies = []
files = []
hgrepos = []
issue_num = 30262
keywords = []
message_count = 14.0
messages = ['292936', '293104', '293105', '293109', '293128', '293134', '293144', '293166', '293170', '293242', '293257', '293619', '325446', '341989']
nosy_count = 5.0
nosy_names = ['ghaering', 'r.david.murray', 'berker.peksag', 'palaviv', 'izbyshev']
pr_nums = ['1440', '1573']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue30262'
versions = ['Python 3.8']

[palaviv]

Both the Cache and Statement objects are internally used and should not be exposed to the user from the sqlite3 module. They are not mentioned in the documentation as well.

[bitdancer]

If these objects have been exposed in the past, we won't simply delete them. At a minimum there would need to be a deprecation period, but is there a real motivation for deleting them?

[bitdancer]

Sorry, by "real motivation" I meant something beyond just cleaning up the API...that's a real motivation, it may just not be enough.

[berkerpeksag]

Even if users somehow managed to create Cache and Statement objects themselves, they are basically implementation details of the module and there is no way to use them to mess with the internal state of the module via using the current API (e.g. Cursor.statement is not exposed)

[bitdancer]

That's the same motivation, not a new one :) Someone somewhere may be using them for something, they've been around for a long time. I hope not, though.

[berkerpeksag]

Someone somewhere may be using them for something, they've been around for a long time.

Well, you can use the same argument for every issue on the tracker. People can even rely on real bugs that are still open for 10 years, but that doesn't mean they shouldn't be fixed.

In this case, your argument looks a bit hypothetical to me. I can't think of valid usages of these objects (I understand people can abuse Python's dynamic nature, but still... :)) You've obviously spent much more time working on sqlite3 than me so I wonder how can they can be useful to third party users :) (e.g. can someone use them for some reason to test their enhanced version of pydoc's extension module support?)

[bitdancer]

No, I'm arguing purely from a generic backward compatibility perspective. There does not seem to be me be sufficient benefit to removing them to justify doing it.

[palaviv]

I have 3 argument for the motivation for this change:

1. Cleaner API - Both objects are internal implementation details. I think that we can look on exposing them as a bug.
2. Easier development - When you remove these objects from the API you can change them without any concern. I for one think that I can make the sqlite3 module faster by changing them from Python objects to simple C structs.
3. Documentation - Currently both objects are part of the API. Thus they should be documented but how would you document the Statement class? Do we really want to have a generic Cache class in the sqlite3 module?

I have less experience with cpython then you. Do you think that maybe the Cache class should be moved to more appropriate place (maybe collections) and be used by others?

At a minimum there would need to be a deprecation period,

Is there a place that document the deprecation process? I will update the patch.

[bitdancer]

They are not part of the API, that is why they are not documented. The convention of "always" using _ prefixed names for non-API stuff is (relatively) recent. It used to be we just didn't document the non-API stuff.

Your second argument is a good motivation. Let's see what others think.

I thought the deprecation process was documented in a PEP, but I can't find it. Basically, we introduce a deprecation warning (and a document the deprecation, but that isn't needed here) in the next release, and then the release after that we actually do the removal. Or in many cases we don't do the removal at all, we just leave the deprecation warning in place until 2.7 is out of maintenance (but I don't think we need to worry about that in this case).

[palaviv]

I am not sure how to raise the deprecation waning in this case. We use both the Cache and Statement objects as part of the sqlite3 module internal flow. How can I only warn the user when he creates this classes directly and not when the sqlite3 module uses them?

[bitdancer]

I don't do much with the C API, but since your goal is to remove them from the PyMODINIT_FUNC, I would think you could replace those entries with calls to wrapper functions that issue the deprecation and then call the real function.

[palaviv]

I opened a separate PR for the deprecation of the Cache and Statement.

I would think you could replace those entries with calls to wrapper functions that issue the deprecation and then call the real function.

I made wrapper deprecation classes that issue the deprecation. I am not sure if there is a easier way...

[izbyshev]

Some additional motivation for removing Cache: it may be used to crash the interpreter (bpo-31734).

[berkerpeksag]

New changeset e657624 by Berker Peksag (Aviv Palivoda) in branch 'master':
bpo-30262: Don't expose private objects in sqlite3 (GH-1440)
e657624