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

Don't expose sqlite3 Cache and Statement #74448

Closed
palaviv mannequin opened this issue May 3, 2017 · 14 comments
Closed

Don't expose sqlite3 Cache and Statement #74448

palaviv mannequin opened this issue May 3, 2017 · 14 comments
Labels
3.8 only security fixes extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error

Comments

@palaviv
Copy link
Mannequin

palaviv mannequin commented May 3, 2017

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
    Copy link
    Mannequin Author

    palaviv mannequin commented May 3, 2017

    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.

    @palaviv palaviv mannequin added 3.7 (EOL) end of life extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error labels May 3, 2017
    @bitdancer
    Copy link
    Member

    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
    Copy link
    Member

    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
    Copy link
    Member

    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
    Copy link
    Member

    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
    Copy link
    Member

    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
    Copy link
    Member

    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
    Copy link
    Mannequin Author

    palaviv mannequin commented May 6, 2017

    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
    Copy link
    Member

    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
    Copy link
    Mannequin Author

    palaviv mannequin commented May 8, 2017

    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
    Copy link
    Member

    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
    Copy link
    Mannequin Author

    palaviv mannequin commented May 13, 2017

    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
    Copy link
    Mannequin

    izbyshev mannequin commented Sep 15, 2018

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

    @berkerpeksag
    Copy link
    Member

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

    @berkerpeksag berkerpeksag added 3.8 only security fixes and removed 3.7 (EOL) end of life labels May 9, 2019
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.8 only security fixes extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants