Opened 9 years ago

Closed 7 years ago

#23546 closed New feature (fixed)

Add kwargs support for cursor.callproc()

Reported by: fatal10110 Owned by: Mariusz Felisiak
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords: oracle
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I think it will be useful to add kwargs or args parameters to callproc method for those backends that support more parameters for callproc method (e.g cx_oracle)

I can not use named parameters for my procedures with django db module. =

Change History (19)

comment:1 by Shai Berger, 9 years ago

Keywords: oracle added
Needs documentation: set
Needs tests: set

comment:2 by Aymeric Augustin, 9 years ago

Needs documentation: unset
Triage Stage: UnreviewedAccepted

PEP 249 says that callproc only accept positional arguments.

That said, since we're just forwarding arguments to the underlying library, this small deviation seems acceptable, with a comment explaining it.

comment:3 by averybigant, 9 years ago

Owner: changed from nobody to averybigant
Status: newassigned

comment:4 by averybigant, 9 years ago

Has patch: set

pull request created
https://github.com/django/django/pull/3342
already tested on ubuntu 12.04 server(amd64) with oracle XE 11g

comment:5 by averybigant, 9 years ago

Needs tests: unset

comment:6 by Shai Berger, 9 years ago

Thanks for the patch. I'll take a look at it tonight (UTC+3)

comment:7 by Shai Berger, 9 years ago

Needs documentation: set
Patch needs improvement: set

Hi,

I made some notes on the PR. Additionally, documentation needed. I suggested making the feature more general, less Oracle-specific, but even if this path is not taken, the new behavior needs to be documented. Currently, callproc is not documented at all, which is a shame, but forgivable considering that the function is as defined in PEP 249; deviating from the PEP justifies adding explicit documentation, probably in docs/topics/db/sql.txt; whether or not that is done, the addition should be mentioned in the release notes (docs/releases/1.8.txt).

in reply to:  7 comment:8 by averybigant, 9 years ago

Needs documentation: unset
Patch needs improvement: unset

Replying to shaib:

Hi,

I made some notes on the PR. Additionally, documentation needed. I suggested making the feature more general, less Oracle-specific, but even if this path is not taken, the new behavior needs to be documented. Currently, callproc is not documented at all, which is a shame, but forgivable considering that the function is as defined in PEP 249; deviating from the PEP justifies adding explicit documentation, probably in docs/topics/db/sql.txt; whether or not that is done, the addition should be mentioned in the release notes (docs/releases/1.8.txt).

Thanks for your detailed comments. New commit pushed. I also replied your comments on github.

comment:9 by Michael Manfre, 9 years ago

Patch needs improvement: set

Implementation needs a DatabaseFeature (e.g. callproc_supports_kwargs) to explicitly enable support for kparams for a backend. The current patch will pass along kparams to the underlying backend as long as they are provided to callproc, which will raise a confusing exception to the user. It should probably raise django.db.utils.NotSupportedError.

Last edited 9 years ago by Michael Manfre (previous) (diff)

in reply to:  9 comment:10 by averybigant, 9 years ago

Patch needs improvement: unset

Replying to manfre:

Implementation needs a DatabaseFeature (e.g. callproc_supports_kwargs) to explicitly enable support for kparams for a backend. The current patch will pass along kparams to the underlying backend as long as they are provided to callproc, which will raise a confusing exception to the user. It should probably raise django.db.utils.NotSupportedError.

Thanks. Made the changes you suggested.

comment:11 by Shai Berger, 9 years ago

I've added some Notes on the PR, and opened a related mailing list discussion.

comment:12 by Tim Graham, 9 years ago

Easy pickings: unset
Patch needs improvement: set

comment:13 by Tim Graham, 8 years ago

Owner: averybigant removed
Status: assignednew

comment:14 by theromulanz, 8 years ago

Is this patch still being considered for release?
I have the same use-case as the ticket description(cx_Oracle keywordParameters).

comment:15 by Tim Graham, 8 years ago

If you'd like to help, please update the old pull request for the comments.

comment:16 by Mariusz Felisiak, 7 years ago

Has patch: unset
Owner: set to Mariusz Felisiak
Patch needs improvement: unset
Status: newassigned
Version: 1.7master

comment:17 by Mariusz Felisiak, 7 years ago

Has patch: set

comment:18 by Tim Graham, 7 years ago

Summary: callproc **kwargs or *args parameterAdd kwargs support for cursor.callproc()
Triage Stage: AcceptedReady for checkin

comment:19 by GitHub <noreply@…>, 7 years ago

Resolution: fixed
Status: assignedclosed

In 489421b:

Fixed #23546 -- Added kwargs support for CursorWrapper.callproc() on Oracle.

Thanks Shai Berger, Tim Graham and Aymeric Augustin for reviews and
Renbi Yu for the initial patch.

Note: See TracTickets for help on using tickets.
Back to Top