| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
@sepehr-rs - You were the author of the original, are you able to take a look? Thanks in advance! |
Sorry, something went wrong.
There was a problem hiding this comment.
Hi @stestagg, thank you for reaching out to me on this!
I think your change is good, but I believe you should also add a versionchanged:: next note under the Cursor.row_factory attribute docs, mirroring what was added for Connection.row_factory/text_factory. Besides that, +1 from me.
Sorry, something went wrong.
As I understand it, gh-149738 already altered the doc for the Cursor.row_factory attr (as well as for the Connection.row_factory), just not the code: cpython/Doc/library/sqlite3.rst Lines 1718 to 1719 in f3bf8ab It's likely I've misunderstood/missed something here! |
Sorry, something went wrong.
Oh, you're right. I must have forgotten about that. This is a real gap, thanks for checking this! :) |
Sorry, something went wrong.
|
There's a related issue that I uncovered above, if you create a Connection or Cursor without calling __init__ on it (why would you do this?) then row_factory/text_factory fields are never initialized, so then you get a segfault anyway. To me, there's a few options:
I'm not sure of the best approach tbh. |
Sorry, something went wrong.
|
I propose to split this into 2 PRs: one with the deletion, one with the assignement. Let's keep this PR focused on one thing :) |
Sorry, something went wrong.
fair point, reverted, I'll raise the other part separately |
Sorry, something went wrong.
|
Hi! Is there anything I can do to progress this? I'll do a rebase on main to check everything's ok, and try to clear the test failure (seems like they're unrelated) |
Sorry, something went wrong.
prevent segfault and bring behaviour in line with docs
re-initialize in tp_init (because of tp_clear calls) and also guard each use of row_factory and text_factory with an explicit NULL check. Keep the previous delattr guards, because who would be calling __delattr__ on these anyway. I'm imagining someone will suggest removing some of these changes, but something something seek forgiveness something.
…n, keeping the underlying delattr guards
There was a problem hiding this comment.
LGTM besides few nitpicks.
Sorry, something went wrong.
| }; | ||
|
|
||
| static PyObject * | ||
| cursor_get_row_factory(PyObject *op, void *closure) |
There was a problem hiding this comment.
Style nitpick: use Py_UNUSED(closure).
Sorry, something went wrong.
| static PyObject * | ||
| cursor_get_row_factory(PyObject *op, void *closure) | ||
| { | ||
| pysqlite_Cursor *self = (pysqlite_Cursor *)op; |
There was a problem hiding this comment.
Use _pysqlite_Cursor_CAST().
Sorry, something went wrong.
| cursor_set_row_factory(PyObject *op, PyObject *value, void *closure) | ||
| { | ||
| pysqlite_Cursor *self = (pysqlite_Cursor *)op; |
There was a problem hiding this comment.
The same.
Sorry, something went wrong.
|
Thanks @stestagg for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14, 3.15. |
Sorry, something went wrong.
|
Sorry, @stestagg and @serhiy-storchaka, I could not cleanly backport this to 3.15 due to a conflict. cherry_picker 8e96dd6f25b53f0773d3f3f2a898eaa1501ee92e 3.15 |
Sorry, something went wrong.
|
Sorry, @stestagg and @serhiy-storchaka, I could not cleanly backport this to 3.14 due to a conflict. cherry_picker 8e96dd6f25b53f0773d3f3f2a898eaa1501ee92e 3.14 |
Sorry, something went wrong.
|
Sorry, @stestagg and @serhiy-storchaka, I could not cleanly backport this to 3.13 due to a conflict. cherry_picker 8e96dd6f25b53f0773d3f3f2a898eaa1501ee92e 3.13 |
Sorry, something went wrong.
|
GH-156160 is a backport of this pull request to the 3.15 branch. |
Sorry, something went wrong.
|
GH-156161 is a backport of this pull request to the 3.14 branch. |
Sorry, something went wrong.
|
GH-156162 is a backport of this pull request to the 3.13 branch. |
Sorry, something went wrong.
…GH-152818) (GH-156162) (cherry picked from commit 8e96dd6) Co-authored-by: Steve Stagg <stestagg@gmail.com>
…GH-152818) (GH-156161) (cherry picked from commit 8e96dd6) Co-authored-by: Steve Stagg <stestagg@gmail.com>
|
Thanks @serhiy-storchaka! I was planning on looking at the comments today, but you saved me :) |
Sorry, something went wrong.
…GH-152818) (GH-156160) (cherry picked from commit 8e96dd6) Co-authored-by: Steve Stagg <stestagg@gmail.com>
| Back | FazBrowse Home | New Git URL |
gh-149738 prevents deletion of row_factory and text_factory attributes on a sqlite3 connection, this PR applies that fix to cursor objects too, to avoid a similar segfault:
I've piggy-backed off the existing docs change and blurb as that seems to cover this already