Modify

Opened 3 years ago

Closed 4 months ago

Last modified 2 months ago

#11915 closed defect (fixed)

Environment.get_db_cnx is deprecated

Reported by: juacarrag@… Owned by: Ryan J Ollos
Priority: highest Component: AccountManagerPlugin
Severity: blocker Keywords: get_db_cnx
Cc: Jun Omae, Franz Trac Release: 1.2

Description

Due to the new support for automated transaction management, Connection instances should no longer be acquired using the familiar get_db_cnx method.

http://trac.edgewall.org/wiki/TracDev/ApiChanges/0.12#get_db_cnx

Attachments (3)

acct_mgr-trac1.1.patch (34.9 KB) - added by Lele Gaifax 3 years ago.
Adapt code to the new DB API
web_ui.py.diff (969 bytes) - added by Franz 2 years ago.
Patch for acct_mgr/web_ui.py to work with Trac 1.1.3
acct_mgr_db_api.patch (46.1 KB) - added by Erik M. Bray 9 months ago.
Attached patch upgrades trunk to use the Trac 1.0 DB API only. Not sure about much else, but I did confirm with this patch that all the unit tests pass. I couldn't get the functional tests to work due to mysterious problems with Twill.

Download all attachments as: .zip

Change History (41)

comment:1 Changed 3 years ago by Ryan J Ollos

We are aware of this, but hasienda has chosen to keep the plugin backward compatible, which is a good choice. However, get_db_cnx has been dropped in Trac 1.1.2, so the changes must be made eventually: trac:wiki:TracDev/ApiChanges/1.1.2#DatabaseAPIChanges.

comment:2 Changed 3 years ago by Steffen Hoffmann

Priority: highesthigh
Trac Release: 0.121.2

Thanks to Ryan for answering before me.

To clarify furthermore, I'm not interested in adopting 0.12 db API as well, because it would mean an intermediate step mainly for keeping Python2.4 compatibility just a little longer. Looking into sources you may recognize, that I already pushed that to extremes.

The inevitable fork will radical too: Just Trac 1.0 API and consequent drop of Python2.4 compatibility. And we must follow Trac core soon now to not loose application fitness, right.

comment:3 Changed 3 years ago by Lele Gaifax

I don't understand the plan (i.e. it is not clear *if* a fork will be made or not), but in the meantime the attached patch works for me, using AMP with current Trac trunk. By any chance it is not good enough to be integrated as is, because I cannot even test some of the ramifications...

Changed 3 years ago by Lele Gaifax

Attachment: acct_mgr-trac1.1.patch added

Adapt code to the new DB API

comment:4 in reply to:  3 ; Changed 2 years ago by Steffen Hoffmann

Replying to lgaifax:

I don't understand the plan (i.e. it is not clear *if* a fork will be made or not),

Obviously branching is mandatory to adapt Trac 1.0 db API. I just hesitate to develop in 0.11 and 1.0 branches for this plugin in parallel.

but in the meantime the attached patch works for me, using AMP with current Trac trunk. By any chance it is not good enough to be integrated as is, because I cannot even test some of the ramifications...

Thanks for pushing the issue. While moving to the new API I'll review and try to improve the code. Breaking portions of your patch is not intended, but inevitable in that process. Until forking trunk to 1.0 branch I plan to offer updated patches for Trac 1.0 API here. Hopefully this will satisfy early adopters using Trac 1.1.2 or later development versions.

comment:5 in reply to:  4 ; Changed 2 years ago by Jun Omae

Cc: Jun Omae added; anonymous removed

Replying to hasienda:

Replying to lgaifax:

I don't understand the plan (i.e. it is not clear *if* a fork will be made or not),

Obviously branching is mandatory to adapt Trac 1.0 db API. I just hesitate to develop in 0.11 and 1.0 branches for this plugin in parallel.

I think We could use with_transaction decorator in trac.db.api which still is available in Trac 1.1.2 instead of Environment.get_db_cnx (dropped in 1.1.2) and Environment.db_transaction (introduced in 1.0 and only works on Python 2.5+).

See https://github.com/jun66j5/accountmanagerplugin/compare/t11915. In the branch, get_read_db function and with_transaction decorator are added to acct_mgr.compat. Thoes functions work well on Trac 0.11 through 1.1.2.

BTW, I found another incompatibility with Trac 1.1.2. OrderedExtensionsOption raises ConfigurationError for unavailable components since 1.1.2.

======================================================================
ERROR: test_render_cfg_admin_panel (acct_mgr.tests.admin.AccountManagerAdminPanelTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/jun66j5/src/accountmanagerplugin.git/acct_mgr/tests/admin.py", line 162, in test_render_cfg_admin_panel
    'config', '')
  File "/home/jun66j5/src/accountmanagerplugin.git/acct_mgr/admin.py", line 227, in render_admin_panel
    return self._do_config(req)
  File "/home/jun66j5/src/accountmanagerplugin.git/acct_mgr/admin.py", line 258, in _do_config
    list=self.acctmgr.register_checks)
  File "/home/jun66j5/venv/trac/1.1.2dev-r13069/lib/python2.6/site-packages/trac/config.py", line 795, in __get__
    option=tag.code("[%s] %s" % (self.section, self.name))))
ConfigurationError: Cannot find implementation(s) of the <code>IAccountRegistrationInspector</code> interface named <code>DisabledCheck</code>. Please check that the Component is enabled or update the option <code>[account-manager] register_check</code> in trac.ini.

comment:6 in reply to:  5 ; Changed 2 years ago by Steffen Hoffmann

Replying to jun66j5:

Replying to hasienda:

Replying to lgaifax:

I don't understand the plan (i.e. it is not clear *if* a fork will be made or not),

Obviously branching is mandatory to adapt Trac 1.0 db API. I just hesitate to develop in 0.11 and 1.0 branches for this plugin in parallel.

I think We could use with_transaction decorator in trac.db.api which still is available in Trac 1.1.2 instead of Environment.get_db_cnx (dropped in 1.1.2) and Environment.db_transaction (introduced in 1.0 and only works on Python 2.5+). See https://github.com/jun66j5/accountmanagerplugin/compare/t11915. In the branch, get_read_db function and with_transaction decorator are added to acct_mgr.compat. Thoes functions work well on Trac 0.11 through 1.1.2.

I see. But this is what I refer to as Trac 0.12 db API. Sorry, but I don't feel that its worth touching the db code twice. In fact I'm already half through with creating the changeset for the conversion to "with env.db_..".

BTW, I found another incompatibility with Trac 1.1.2. OrderedExtensionsOption raises ConfigurationError for unavailable components since 1.1.2.

Uh. I'm unaware of related changes and will have to check that first.

comment:7 in reply to:  6 ; Changed 2 years ago by Ryan J Ollos

Replying to hasienda:

BTW, I found another incompatibility with Trac 1.1.2. OrderedExtensionsOption raises ConfigurationError for unavailable components since 1.1.2.

Uh. I'm unaware of related changes and will have to check that first.

To help with refreshing memory, the change was implemented in trac:#10285.

comment:8 in reply to:  7 Changed 2 years ago by Steffen Hoffmann

Replying to rjollos:

To help with refreshing memory, the change was implemented in trac:#10285.

Thanks. Now I remember indeed. Then it does not only affect mentioned development versions leading to Trac 1.2 but current stable Trac 1.0.2 or later too.

comment:9 in reply to:  5 Changed 2 years ago by Steffen Hoffmann

Replying to jun66j5:

See https://github.com/jun66j5/accountmanagerplugin/compare/t11915. In the branch, get_read_db function and with_transaction decorator are added to acct_mgr.compat. Thoes functions work well on Trac 0.11 through 1.1.2.

There is a bunch of additional pointers on what to look for, including unit tests. Thanks a lot, even if I don't intend to keep Python2.4 compatibility any longer for the upcoming development branch.

comment:10 Changed 2 years ago by Steffen Hoffmann

#12017 has been closed as a duplicate of this ticket.

Changed 2 years ago by Franz

Attachment: web_ui.py.diff added

Patch for acct_mgr/web_ui.py to work with Trac 1.1.3

comment:11 Changed 2 years ago by Franz

Priority: highhighest

Newest Account-Manager from Trunk (SVN 14395) is not working with Trac version 1.1.3. For fixing the issue web_ui.py.diff could be applied.

Error log shows:

ERROR: Internal Server Error:
Traceback (most recent call last):
  File "build\bdist.win32\egg\trac\web\main.py", line 551, in _dispatch_request
    dispatcher.dispatch(req)
  File "build\bdist.win32\egg\trac\web\main.py", line 186, in dispatch
    chosen_handler = self._get_valid_default_handler(req)
  File "build\bdist.win32\egg\trac\web\main.py", line 279, in _get_valid_default_handler
    name = req.session.get('default_handler')
  File "build\bdist.win32\egg\trac\web\api.py", line 384, in __getattr__
    value = self.callbacks[name](self)
  File "build\bdist.win32\egg\trac\web\main.py", line 306, in _get_session
    return Session(self.env, req)
  File "build\bdist.win32\egg\trac\web\session.py", line 202, in __init__
    if req.authname == 'anonymous':
  File "build\bdist.win32\egg\trac\web\api.py", line 384, in __getattr__
    value = self.callbacks[name](self)
  File "build\bdist.win32\egg\trac\web\main.py", line 140, in authenticate
    authname = authenticator.authenticate(req)
  File "build\bdist.win-amd64\egg\acct_mgr\util.py", line 81, in wrap
    return func(self, *args, **kwds)
  File "build\bdist.win-amd64\egg\acct_mgr\web_ui.py", line 425, in authenticate
    return auth.LoginModule.authenticate(self, req)
  File "build\bdist.win32\egg\trac\web\auth.py", line 91, in authenticate
    req.incookie['trac_auth'])
  File "build\bdist.win-amd64\egg\acct_mgr\web_ui.py", line 496, in _get_name_for_cookie
    db = self.env.get_db_cnx()
AttributeError: 'Environment' object has no attribute 'get_db_cnx'

comment:12 Changed 2 years ago by Franz

Cc: Franz added

comment:13 Changed 2 years ago by Ryan J Ollos

I'm planning to upgrade trac-hacks.org to Trac 1.1.3 or 1.1.4 (#12171), hopefully in the next month or so. This ticket is a critical step to accomplishing that.

In the past I've tried to be strict in maintaining plugins and support versions of Trac back to 0.11. Recently I've been more inclined to just cut a final release supporting Trac < 1.0, even if it's not perfect, and move on to supporting Trac >= 1.0. I simply don't have the time to do everything I want to do, so I have to make some compromises somewhere. My thought is, what's better for the Trac project, supporting a user on 0.11.x, or supporting early adopters of Trac 1.1.x? If you are in the same boat as me time-wise, I think it would be good to just cut the 0.5 release and move on to supporting Trac >= 1.0. It will allow more to be done for AccountManagerPlugin in whatever little time we have available, and may even have the nice side effect of forcing users to upgrade to 1.0+.

comment:14 Changed 19 months ago by Franz

Ticket #12506 was closed as duplicate.

Fortunately acct_mgr-trac1.1.patch of lgaifax is working fine. Thanks for that.

As lgaifax, I do not understand why AccountManager is kept backwards compatible; aren't branches made for such cases? Another possibily would be to make a version 0.6 for >= 1.1.2 and keep an older version 0.5 for older Trac versions < 1.1.

comment:15 in reply to:  14 Changed 19 months ago by Ryan J Ollos

Replying to framay:

Another possibily would be to make a version 0.6 for >= 1.1.2 and keep an older version 0.5 for older Trac versions < 1.1.

That was the original plan. I'd just like to hear from hasienda whether he will be able to continue maintaining the project, or if that is just not possible for him to do any longer. I can't maintain the project long-term, but I'm willing to create the 0.5 and 0.6 releases.

It's often the case developers find they no longer have time to maintain these projects and we shouldn't fault anyone for that. Some brief communication about the status though would be a big help to those that depend on the project so that we can find a way to forge ahead.

comment:16 Changed 18 months ago by Jun Omae

#12531 was closed as duplicate.

comment:17 Changed 16 months ago by Ryan J Ollos

#12598 closed as a duplicate.

comment:18 Changed 10 months ago by Erik M. Bray

Indeed it seems the obvious thing to do would be to create a branch for older trac versions, and move trunk to supporting the new database API.

rjollos, if it's alright with you and/or hasienda I'd be willing to help with maintenance of this project at least insofar as it getting it minimally working on current Trac (1.1.6). Otherwise I'll probably have to fork it and that's no fun :(

comment:19 Changed 10 months ago by Ryan J Ollos

The trunk code supports Trac >= 1.0, which is a change from the previous plan (comment:3:ticket:12722). I'm not bothering with any versions of Trac prior to 1.0. If someone makes a patch that applies cleanly against the trunk I can review and apply it.

comment:20 in reply to:  19 Changed 10 months ago by Erik M. Bray

Replying to rjollos:

The trunk code supports Trac >= 1.0, which is a change from the previous plan (comment:3:ticket:12722). I'm not bothering with any versions of Trac prior to 1.0. If someone makes a patch that applies cleanly against the trunk I can review and apply it.

Okay, thanks for the update. I'll submit a patch sometime next week. Cheers!

Changed 9 months ago by Erik M. Bray

Attachment: acct_mgr_db_api.patch added

Attached patch upgrades trunk to use the Trac 1.0 DB API only. Not sure about much else, but I did confirm with this patch that all the unit tests pass. I couldn't get the functional tests to work due to mysterious problems with Twill.

comment:21 Changed 9 months ago by Ryan J Ollos

Owner: changed from Steffen Hoffmann to Ryan J Ollos
Status: newaccepted

The functional tests haven't been working for me as well. The patch looks good. I'll test and apply it this week. Thanks!

comment:22 Changed 9 months ago by Ryan J Ollos

In 15610:

0.5dev: Replace suite function in test module with test_suite

See trac:#11506 for more details. Refs #11915.

comment:23 Changed 9 months ago by Ryan J Ollos

Tests are passing on OSX after changes in #12192. I need to refactor acct_mgr_db_api.patch so that it conforms to Trac idioms, but should have changes committed within a few days.

comment:24 Changed 9 months ago by Erik M. Bray

Out of curiosity, which idioms does it not conform to?

comment:25 Changed 9 months ago by Ryan J Ollos

As an example,

        with self.env.db_transaction as db:
            cursor = db.cursor()
            cursor.execute("INSERT INTO session_attribute "
                               "(sid,authenticated,name,value) "
                               "VALUES (%s,1,'password',%s)",
                               ('bar', 'bar'))

can become:

        self.env.db_transaction("INSERT INTO session_attribute "
                                "(sid,authenticated,name,value) "
                                "VALUES (%s,1,'password',%s)",
                                ('bar', 'bar'))

There are other cases where it's not necessary to obtain a cursor.

Also, in some cases we can use:

for item, in self.env.db_query(""" ...

rather than:

with self.env.db_query as db:
    for item, in db(""" ....

This is documented at trac:TracDev/DatabaseApi#Trac1.0API.

comment:26 Changed 9 months ago by Erik M. Bray

Ah, I didn't know about the iterable interface for db_query. Nice!

comment:27 Changed 8 months ago by Ryan J Ollos

If you have some time to do the refactoring and rebase the patch on the latest trunk, that would be much appreciated. Otherwise, I will try to get it committed soon, but pretty busy with finalizing Trac 1.2!

All unit test pass for me on OSX. I'd be interested to hear if they pass on other platforms.

comment:28 Changed 4 months ago by Ryan J Ollos

I've been working on these changes and plan to have them committed tomorrow. I hope followers of this ticket will be willing to install the latest and give feedback. Thanks!

comment:29 Changed 4 months ago by anonymous

Are there any updated patches that one can apply on top of trunk to get this going? My current installation of trac got updated to 1.2 and it is now crippled because it's just unusable without the account manager plugin. I am now debating if I should go through the pain of a downgrade or... I don't know what else? Suggestions?

comment:30 Changed 4 months ago by Ryan J Ollos

See comment:28.

Upgrading your site without creating a backup or running the upgrade first on a staging site is a very bad idea. But if that is where you are now, you can probably get attachment:acct_mgr_db_api.patch to apply with little or no edits. That, or you could downgrade and wait. I may not finish this today, but will have it finished by end of week.

Last edited 4 months ago by Ryan J Ollos (previous) (diff)

comment:31 Changed 4 months ago by antoine@…

I have all the backups I need, that's not the problem, churn is. The trac 1.2 update came in as a system update and I would rather not pin outdated package versions if I can avoid it, even if it means doing a little bit of extra work.

As for the patch, unless there is a new one, this does not apply against trunk, I've tried (fixing patch context by hand where needed) and said in comment:29 : Are there any updated patches that one can apply. I'll wait a few days and hope an updated patch or commit is posted. Thanks

comment:32 Changed 4 months ago by anonymous

Ping. End of this week?

comment:33 Changed 4 months ago by Ryan J Ollos

Yes, it will be done soon. It's in the critical path for a number of my projects, however there is a lot of testing to do, and other AccountManager issues to resolve to make it compatible with Trac 1.2.

comment:34 Changed 4 months ago by e.grammatico@…

Hello there,

This fix is needed.

Thanks and regards,

Eric.

comment:35 Changed 4 months ago by velislav

I've read all comments but I do not understand why trunk does not contain needed changes(in the end active version of trac is 1.2). I've build the source from trunk but it does obviously not run with trac 1.2.

I've also tried patch from comment 30 with no success. Would you clear out at what revision should be applied patch from comment 30?

comment:36 Changed 4 months ago by Ryan J Ollos

Resolution: fixed
Status: acceptedclosed

In 16056:

0.5dev: Adapt to Trac 1.0 database API

Test coverage is incomplete. Please report any issues
to #11915.

Initial patch by ebray.

Fixes #11915.

comment:37 Changed 2 months ago by shane@…

Just curious - what branch is this patch on? It doesn't appear to be in the trunk yet?

comment:38 Changed 2 months ago by Ryan J Ollos

Changes were committed to trunk in r16056.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted. Next status will be 'reopened'.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.