Modify

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#8834 closed defect (fixed)

TypeError: sequence item 0: expected string, int found

Reported by: stroucki@… Owned by: hasienda
Priority: normal Component: AccountManagerPlugin
Severity: normal Keywords: type error exception
Cc: Trac Release: 0.11

Description

224 new_password = self._random_password()
225 mgr = AccountManager(self.env)
226 try:
227 mgr._notify('password_reset', username, email, new_password)
228 except Exception, e:
229 return {'error': ','.join(e.args)}
230 mgr.set_password(username, new_password)
231 if mgr.force_passwd_change:
232 db = self.env.get_db_cnx()
233 cursor = db.cursor()

In line 229 here, if the error returning is something like (111, 'Connection refused), the statement will fail because 111 is not a string. Changing to join(map(str, e.args)) will convert all members to string and remove the error.

Attachments (0)

Change History (8)

comment:1 Changed 3 years ago by hasienda

  • Keywords needinfo added; conversion removed

Sorry, didn't saw this coming in. Maybe email choke on the bad formatting? A little bit of formatting works wonders (just add {{{ and on the next new line #!python, both on top and another set of }}} on a line on it's own below the piece of code - similar for error messages). Result for the code above (impossible to understand otherwise):

224     new_password = self._random_password()
225     mgr = AccountManager(self.env)
226     try:
227         mgr._notify('password_reset', username, email, new_password)
228     except Exception, e:
229         return {'error': ','.join(e.args)}
230     mgr.set_password(username, new_password)
231     if mgr.force_passwd_change:
232         db = self.env.get_db_cnx()
233         cursor = db.cursor()

Another point: I still had to guess or look-up proper indentation...

I don't argue with the intention. You're trying to improve the code, that's fine. But why do you let me/us jump through unnecessary loopholes then? Easy for you to tell us, it's acct_mgr.web_ui what we're looking at here, right? I doubt, this is the only occurrence of unmapped error messages in this plugin, right? Did you check?

Considering your line numbers you're certainly not looking at the trunk branch of AccountManagerPlugin. So but what's your version? Did you ever experience a HTTP 111 error, and if YES, under what circumstances?

Your reasoning is valid and possibly valuable for the ongoing development. Thanks for taking care, taking your time to report and in advance for more feedback to elaborate towards a valid (global?) fix. Please follow-up.

comment:2 Changed 3 years ago by stroucki@…

Thanks for finding the file and formatting - it was late and I forgot. I can't see what version it was anymore, but if the code in trunk is like this the error will still happen in the same circumstance. If you let me know where I can find the version, I'll let you know.

I don't know what you mean by unmapped error message. It was an exception raised within the exception handler. I received 111, Connection refused (not an http error) when the smtp server specified wasn't actually listening for connections. The exception handler which got executed as the connection failed then excepted itself, with the above message.

Whether similar bugs exist elsewhere, maybe. I ran into this specific one and I fixed it. I wanted to let you know so you could fix it and so others could see a solution.

comment:3 Changed 3 years ago by hasienda

  • Keywords needinfo removed

Sure, thanks for the quick reply. I've never seen a non-graceful handling of notification message sending failures, so I didn't expect that now.

I'll try to reproduce according to your hints.

Never mind about the plugin version. This is most probably valid for 0.11 and trunk branch. Since I have a full copy of the repository here, I can check this in a minute. Just begging you to provide as much inside as possible, even if it looks irrelevant to yourself.

comment:4 Changed 3 years ago by hasienda

  • Status changed from new to assigned

A development note: There is an i18n related issue with str we expect Unicode chars here (tested), so we'll replace it with to_unicode from trac.util.text.

comment:5 Changed 3 years ago by hasienda

Found no other occurrence other than in the 0.11 branch to fix. BTW, line numbers were matching there.

Changes are prepared and ready for commit now. Any objections against mentioning your email in the public commit message for the repository, or what other name would you prefer for credit?

comment:6 follow-up: Changed 3 years ago by stroucki@…

No objections, thank you.

comment:7 Changed 3 years ago by hasienda

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [10259]) AccountManagerPlugin: Prevent exception in exception handler, closes #8834.

comment:8 in reply to: ↑ 6 Changed 3 years ago by hasienda

Replying to stroucki@andrew.cmu.edu:

No objections, thank you.

Thanks, I've been just too fast, sorry. I'm still grateful for your help.

Add Comment

Modify Ticket

Action
as closed .
as The resolution will be set. Next status will be 'closed'.
to The owner will be changed from hasienda. Next status will be 'closed'.
The resolution will be deleted. Next status will be 'reopened'.
Author


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

 
Note: See TracTickets for help on using tickets.