django icon indicating copy to clipboard operation
django copied to clipboard

Fixed #33817 -- Used python-oracledb and deprecate cx_Oracle.

Open petronny opened this issue 1 year ago • 25 comments

cx_Oracle has a major new release under a new name and homepage python-oracledb.

The python-oracledb driver is a Python programming language extension module allowing Python programs to connect to Oracle Database. Python-oracledb is the new name for Oracle's popular cx_Oracle driver.

Tested on an Oracle Autonomous Database with TLSv1 connection.

petronny avatar Jul 12 '22 06:07 petronny

Hello @petronny! Thank you for your contribution 💪

As it's your first contribution be sure to check out the patch review checklist.

If you're fixing a ticket from Trac make sure to set the "Has patch" flag and include a link to this PR in the ticket!

If you have any design or process questions then you can ask in the Django forum.

Welcome aboard ⛵️!

github-actions[bot] avatar Jul 12 '22 06:07 github-actions[bot]

@petronny We should change tests/requirements/oracle.txt.

felixxm avatar Jul 30 '22 17:07 felixxm

buildbot, test on oracle.

felixxm avatar Aug 05 '22 05:08 felixxm

@petronny Please push fixes in separate commits. It's hard to review after force-push.

felixxm avatar Aug 05 '22 07:08 felixxm

Please push fixes in separate commits.

OK.

The last change only replaces oracledb.OBJECT with oracle.DB_TYPE_OBJECT. These two are same but the first one is deprecated.

This won't fix the failed test in oragis. I've created an issue https://github.com/oracle/python-oracledb/issues/43 for it on oracledb.

petronny avatar Aug 05 '22 08:08 petronny

https://github.com/oracle/python-oracledb/issues/43#issuecomment-1206453289

According the reply of the oracledb group, oracle.DB_TYPE_OBJECT is not currently supported but may be supported in the future.

For now, we have to enable the thick mode of oracledb to pass this test, which needs the Oracle Client libraries. Are these libraries available in the oragis19 test?

Also, to enable the thick mode, a call to oracledb.init_oracle_client() is needed. But currently I don't know where to add this line just for this test.

petronny avatar Aug 05 '22 13:08 petronny

For now, we have to enable the thick mode of oracledb to pass this test, which needs the Oracle Client libraries. Are these libraries available in the oragis19 test?

I'm guessing that since cx_Oracle was tested, then the libraries are available.

Overall, I recommend skipping this specific test when python-oracledb is in Thin mode (until the future time that the feature is supported in Thin mode). You could do two test runs: the first with Thin mode and the second with Thick mode.

cjbj avatar Aug 05 '22 23:08 cjbj

I've updated the code to enable this test only for the thick mode of oracledb and cx_Oracle.

But I have no idea how to setup a test for the thick mode.

petronny avatar Aug 06 '22 04:08 petronny

I've updated the code to enable this test only for the thick mode of oracledb and cx_Oracle.

But I have no idea how to setup a test for the thick mode.

Since you were able to run cx_Oracle so i assume you have the oracle instant client installed. So to switch from thin mode to thick mode in pyoracledb you need to call oracledb.init_oracle_client(lib_dir="path_to_instant_client"). You just need to call it once. You can call it either in the application or you can call after import oracledb in the following file django/db/backends/oracle/base.py.

suraj-ora-2020 avatar Aug 10 '22 09:08 suraj-ora-2020

You can call it either in the application or you can call after import oracledb in the following file django/db/backends/oracle/base.py.

It will force the mode to be thick if called in base.py. And I believe that most users should perfer the thin mode. So I perfer to call it just in the application.

But now we need some new jenkins workflows (or pipelines, idk) to test both the thin and thick modes, which are:

  • oracle19, for thin mode and the basic fields.
  • oragis19, for thin mode and the geometry fields.
  • oracle19-thick, for thick mode and the basic fields.
  • oragis19-thick , for thick mode and the geometry fields.

For the first two we can just use the existing oracle19 and oragis19 tests. I have no idea how to add the last two for jenkins and enable the thick mode in them.

petronny avatar Aug 10 '22 10:08 petronny

@felixxm are you able to help with @petronny's question about enabling extra tests?

cjbj avatar Aug 12 '22 00:08 cjbj

@felixxm are you able to help with @petronny's question about enabling extra tests?

IMO, we don't want to maintain multiple workflows for Oracle. We can decide to always use thick mode as thin mode is limited :thinking:. I should have some time to check this carefully in the coming weeks.

felixxm avatar Aug 12 '22 05:08 felixxm

Well I wish there could be an option to still use the thin mode if the thick mode is set as the default since I don't need the geometry field.

petronny avatar Aug 12 '22 05:08 petronny

@felixxm are you able to help with @petronny's question about enabling extra tests?

IMO, we don't want to maintain multiple workflows for Oracle. We can decide to always use thick mode as thin mode is limited 🤔. I should have some time to check this carefully in the coming weeks.

@felixxm I think using default as thick would not server the benefit for which pyoracledb has been developed.In thin mode we dont require instant client library dependencies, its light weight as well faster. We are continuously working on expanding thin mode functionality. We can give option to users to choose between thin and thick mode using some parameters in the settings.py.

suraj-ora-2020 avatar Aug 17 '22 10:08 suraj-ora-2020

To get some progress on this PR, I can settle for using Thick mode until Thin mode has the Object support needed for all tests. Users (like us) will be able to keep testing Thin mode easily enough - particularly if this PR lands.

cjbj avatar Aug 22 '22 22:08 cjbj

@felixxm Hi, I have tried to enable the thick mode in tests/dbshell/test_oracle.py but I'm not sure if it will work. If it works then the PR is ready for review and the rest tests on the oracle database.

petronny avatar Aug 24 '22 11:08 petronny

@petronny I had ran the test suite against your PR and have got some observations. The stats with you PR is: 15964 tests FAILED (failures=106, errors=41, skipped=1755, expected failures=6).

The stats with the current django 4.2dev version is: 15969 tests FAILED (failures=24, errors=3, skipped=1755, expected failures=6)

As we can see some new failures are coming with the PR which were not present earlier. Most of the failures are exactly same and repetative. AssertionError: 318 != 0 : Stream should be empty: actually contains '/Users/apple/oracle/django_mod1/django/django/db/backends/oracle/base.py:57

Test names : test_base_command_multiple_label (admin_scripts.tests.CommandTypes), test_testrunner_option (test_runner.tests.CustomTestRunnerOptionsCmdlineTests), etc.

Maybe some of the new changes is causing this failures.

suraj-ora-2020 avatar Aug 26 '22 07:08 suraj-ora-2020

@suraj-ora-2020 Thanks for the tests.

django/db/backends/oracle/base.py:57 gives a warning when oracledb is in the thin mode. However the test wants no warning at all.

I've already called oracledb.init_oracle_client() in tests/dbshell/test_oracle.py:15. But maybe django/db/backends/oracle/base.py is loaded earlier than that.

Could you have a try to just remove the warning at django/db/backends/oracle/base.py:57 to see if the tests pass? If pass then I just need to figure out a new place to give the warning.

petronny avatar Aug 26 '22 08:08 petronny

@suraj-ora-2020 Thanks for the tests.

django/db/backends/oracle/base.py:57 gives a warning when oracledb is in the thin mode. However the test wants no warning at all.

I've already called oracledb.init_oracle_client() in tests/dbshell/test_oracle.py:15. But maybe django/db/backends/oracle/base.py is loaded earlier than that.

Could you have a try to just remove the warning at django/db/backends/oracle/base.py:57 to see if the tests pass? If pass then I just need to figure out a new place to give the warning.

@petronny The failures were due to the warnings. I tried removing the warnings and the tests were passing without failure.I guess the place for warnings need to be modified.

suraj-ora-2020 avatar Aug 29 '22 10:08 suraj-ora-2020

I guess the place for warnings need to be modified.

@suraj-ora-2020 I've moved that warning into DatabaseWrapper. Would you like to give it a new round of tests?

petronny avatar Aug 30 '22 09:08 petronny

@petronny I can still see those errors coming from the modified place. AssertionError: 335 != 0 : Stream should be empty: actually contains '/Users/apple/oracle/django_mod1/django/django/db/backends/oracle/base.py:251

suraj-ora-2020 avatar Aug 31 '22 07:08 suraj-ora-2020

@suraj-ora-2020 Well, I've moved it to DatabaseWrapper.init_connection_state() just before the cursor is created. Could you test if it works this time?

petronny avatar Aug 31 '22 08:08 petronny

@suraj-ora-2020 Well, I've moved it to DatabaseWrapper.init_connection_state() just before the cursor is created. Could you test if it works this time?

@petronny Yes now it is working fine.

suraj-ora-2020 avatar Aug 31 '22 15:08 suraj-ora-2020

Great! Now let's wait for the review from @felixxm.

petronny avatar Aug 31 '22 16:08 petronny

@felixxm do we have any update on review or merging of this PR.

suraj-ora-2020 avatar Sep 21 '22 10:09 suraj-ora-2020

do we have any update on review or merging of this PR.

If this is ready for re-review, the ticket should be updated to remove the "Needs ..." flags needs documentation, needs tests, patch needs improvement.

jacobtylerwalls avatar Sep 25 '22 16:09 jacobtylerwalls

@jacobtylerwalls I see. Now it's done.

petronny avatar Sep 26 '22 05:09 petronny

Hi @petronny. In django/db/backends/oracle/base.py file in line 275 can you please modify the Database.driver_mode.thin_mode to Database.is_thin_mode. https://python-oracledb.readthedocs.io/en/latest/api_manual/module.html#oracledb.is_thin_mode

suraj-ora-2020 avatar Oct 03 '22 09:10 suraj-ora-2020

@suraj-ora-2020 Updated.

petronny avatar Oct 03 '22 10:10 petronny

Really looking forward to this feature, and big kudos to Oracle for the thin driver!

jlost avatar Oct 12 '22 20:10 jlost