ValueError: need more than 1 value to unpack

[anonymous]

How to Reproduce

While doing a GET operation on , Trac issued an internal error.

(please provide additional details here)

Request parameters:

{}

User agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:20.0) Gecko/20100101 Firefox/20.0

System Information

System information not available

Enabled Plugins

Plugin information not available

Python Traceback

Traceback (most recent call last):
  File "/usr/lib/python2.7/dist-packages/trac/web/main.py", line 522, in _dispatch_request
    dispatcher.dispatch(req)
  File "/usr/lib/python2.7/dist-packages/trac/web/main.py", line 208, in dispatch
    chosen_handler)
  File "/usr/lib/python2.7/dist-packages/trac/web/main.py", line 350, in _pre_process_request
    chosen_handler = filter_.pre_process_request(req, chosen_handler)
  File "/usr/lib/python2.7/dist-packages/trac/versioncontrol/api.py", line 330, in pre_process_request
    repo = self.get_repository(reponame)
  File "/usr/lib/python2.7/dist-packages/trac/versioncontrol/api.py", line 515, in get_repository
    repoinfo = self.get_all_repositories().get(reponame, {})
  File "/usr/lib/python2.7/dist-packages/trac/versioncontrol/api.py", line 583, in get_all_repositories
    for reponame, info in provider.get_repositories() or []:
  File "/usr/lib/python2.7/dist-packages/trac/versioncontrol/api.py", line 112, in get_repositories
    db = self.env.get_db_cnx()
  File "/usr/lib/python2.7/dist-packages/trac/env.py", line 329, in get_db_cnx
    return get_read_db(self)
  File "/usr/lib/python2.7/dist-packages/trac/db/api.py", line 91, in get_read_db
    or DatabaseManager(env).get_connection())
  File "/usr/lib/python2.7/dist-packages/trac/db/api.py", line 152, in get_connection
    connector, args = self.get_connector()
  File "/usr/lib/python2.7/dist-packages/trac/db/api.py", line 185, in get_connector
    scheme, args = _parse_db_str(self.connection_uri)
  File "/usr/lib/python2.7/dist-packages/trac/db/api.py", line 221, in _parse_db_str
    scheme, rest = db_str.split(':', 1)
ValueError: need more than 1 value to unpack

[anonymous]

miss: my OS is Ubuntu 13.04

[Christian Boos]

Please double check the value you gave to the [trac] database setting.

This is an InstallationIssue, but we could show a nicer error message here.

[Ryan J Ollos <ryan.j.ollos@…>]

Patch proposed in ​t11189. The special case of an empty string is handled in ​c2bd917d to prepare a nicer error message, even though it would also be caught by the try/except/raise added in ​581337ad.

[Ryan J Ollos]

I've expanded the changeset to cover all the cases.

1. Invalid format - no colon in connection string:

2. Invalid format - string terminates at the colon:

3. Empty connection string:

4. Unknown scheme: (check already existed, but error message was enhanced)

5. If the "non-scheme" part of the string is invalid, such as an invalid path to the SQLite database file, we still get an ugly traceback. However, raising a TracError for this issue probably needs to be handled outside of _parse_db_str and therefore may be outside the scope of this ticket.

[Jun Omae]

Looks good! BTW, The similar errors occur by double port numbers, e.g. postgres://localhost:42:42 and malformed parameters, e.g. postgres://localhost/schema?name. Could you please try to fix these?

$ PYTHONPATH=. ~/venv/py25/bin/python
Python 2.5.6 (r256:88840, Feb 29 2012, 04:03:24)
[GCC 4.1.2 20080704 (Red Hat 4.1.2-51)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> from trac.db.api import _parse_db_str
>>> _parse_db_str('postgres://localhost:5432:5432')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "trac/db/api.py", line 387, in _parse_db_str
    host, port = host.split(':')
ValueError: too many values to unpack
>>> _parse_db_str('postgres://localhost/schema?name')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "trac/db/api.py", line 404, in _parse_db_str
    name, value = param.split('=', 1)
ValueError: need more than 1 value to unpack

[Ryan J Ollos]

Replying to jomae:

Looks good! BTW, The similar errors occur by double port numbers, e.g. postgres://localhost:42:42 and malformed parameters, e.g. postgres://localhost/schema?name. Could you please try to fix these?

Yeah, I'll cover those cases and post the changes for feedback. Thanks for reviewing!

[Ryan J Ollos]

Btw, do you have any hints on the best way to push from Git to SVN? I've tried adding svn.edgewall.org/repos/trac as a remote to in my Git repository, but then it wants to pull down the entire history.

[Jun Omae]

Try -r option for git svn fetch, e.g. git svn fetch -rXXXX:HEAD.

1. Add the following to .git/config

   [svn-remote "svn"]
   	url = http://svn.edgewall.com/repos/trac
   	fetch = trunk:refs/remotes/svn/trunk
   	branches = branches/{0.12-stable,1.0-stable}:refs/remotes/svn/*
   

2. Run git svn fetch -r11000:HEAD, it will create remotes/svn/{trunk,1.0-stable,0.12-stable} branches.

See ​http://stackoverflow.com/a/10173516/709074.

[Christian Boos]

Well, for tracking the svn branches, you could directly fetch from the mirror (http://svn.edgewall.org/git/trac/mirror)

[Ryan J Ollos]

Replying to cboos:

Well, for tracking the svn branches, you could directly fetch from the mirror (http://svn.edgewall.org/git/trac/mirror)

At the moment I'm primarily concerned with how to push from my Git branches to SVN. Do you and Jun both use git-svn?

[Christian Boos]

Replying to rjollos:

Replying to cboos:

Well, for tracking the svn branches, you could directly fetch from the mirror (http://svn.edgewall.org/git/trac/mirror)

At the moment I'm primarily concerned with how to push from my Git branches to SVN. Do you and Jun both use git-svn?

I don't use it, when I "push to svn", I either commit what's on the branch wholesale or if I want to keep a svn changeset for each git commit, I do something like the following, assuming a tXYZ branch which has 4 commits on top of the last in svn (rebasing it first if needed):

$ git status -sb
## tXYZ
$ git checkout tXYZ~3
$ git log --format="%B" > commit.txt
$ svn ci -F commit.txt
$ git checkout tXYZ~2
...
$ git checkout tXYZ~1
...
$ git checkout tXYZ
...

(I use aliases to make the above more natural, st, co, msg).

I'm sure git svn could automate the above somehow, but I like to have this level of control, often tweaking commit.txt and running the test suite before committing to svn.

[Ryan J Ollos]

I've prepared a series of changes that I think will address the issues Jun raised in comment:6, ​t11189.2. I'll wait a few days before committing in case that anyone has a chance to review.

I was thinking that I'd squash these changes to a single changeset before committing to SVN, but I'd be interested to hear opinions on that as well.

[Peter Suter]

In #8401 the message Database Connection string must start with scheme:/ was replaced by Unknown scheme "%s"; database connection string must start with {scheme}:/'. (Although it seems like a known scheme with missing / would also trigger this message.)

Now you use the generic Invalid format "%(db_str)s" for database connection string. instead. Would it be better to again add additional hints about what exactly is invalid? E.g.:

• It must start with {scheme}: but no : was found.
• It must start with {scheme}:/ or sqlite: but no / was found and scheme was "%(scheme)s".
• It contained host "%(host)s" but "%(port)s" is not a port number.
• It contained parameters "%(qs)s" but no = was found in parameter "%(param)s".

---

About squashing related changes to a single changeset before committing to SVN: To me it seems like this has been a common practice in Trac. So it would not be surprising. But I'm not sure if it's really preferred / recommended and what the advantage is. Maybe it was just done to minimize the work with SVN before HG and Git made smaller changesets more practical?

To read everything at once one can still do things like log:trunk/trac/db@10406:10410 and diff:trunk/trac/db@10406:10410

[Ryan J Ollos]

Replying to psuter:

[…] Now you use the generic Invalid format "%(db_str)s" for database connection string. instead. Would it be better to again add additional hints about what exactly is invalid? E.g.:

• It must start with {scheme}: but no : was found.
• It must start with {scheme}:/ or sqlite: but no / was found and scheme was "%(scheme)s".
• It contained host "%(host)s" but "%(port)s" is not a port number.
• It contained parameters "%(qs)s" but no = was found in parameter "%(param)s".

Thank you for the feedback. Overall, I felt that the method was overly complex and was looking for ways to simplify the code. I spent a bit of time trying to parse the string with a regular expression, but I couldn't make it work well.

When I removed the Invalid format error message and replaced it with a more generic one, I was mainly thinking that a great amount of detail shouldn't be needed in order for the user to figure out what is wrong. We're (hopefully) trapping the exception and presenting the user with a ConfigurationError for all of the cases in which the connection string is incorrect, and the user should be able to go to the documentation to figure out what is wrong.

It would certainly be more user-friendly to show the error messages you suggest, it just adds more code and messages to be translated. If we can help a few users though, it is probably worth adding the additional messages (and #8401 is certainly a good argument for that).

About squashing related changes to a single changeset before committing to SVN: To me it seems like this has been a common practice in Trac. So it would not be surprising. But I'm not sure if it's really preferred / recommended and what the advantage is. Maybe it was just done to minimize the work with SVN before HG and Git made smaller changesets more practical?

For the moment I just want to making sure that I'm falling in line with the way things are done in Trac since I think it is most important for consistent processes to be followed. If it was entirely up to me though, I can see the value of pushing the changes at the granularity of the Git development branch (8 changesets so far), but then merging them to trunk in a single changeset to keep the merge simple and the change history of the trunk less verbose.

[Peter Suter]

I collected a few real world examples of invalid DB strings:

Error Report	DB string
​TracError: Database connection string must start with scheme:/	mysql:username:password@127.0.0.1:3306/trac
​TracError: Database connection string must start with scheme:/	mysql:db/trac.db
TracError: Database Connection string must start with scheme:/	sqllite:db/trac.db
​ValueError: invalid literal for int(): unix-socket=	mysql://root:mercury@localhost:unix-socket=/home/mysql/mysql.sock/trac
ValueError: invalid literal for int() with base 10: ''	mysql://login:ntrac123@localhost:/ntc_trac'
ValueError: invalid literal for int(): 1X2	the DB password starts with "1X2/" (Fixed in Trac: unquote password and username)
​ValueError: too many values to unpack	postgres://trac@tracdb@localhost:5432/trac?schema=local_projects
​ValueError: too many values to unpack	The '@' between the password and database address was missing
​ValueError: too many values to unpack	mysql://admin:secretpass:3306/trac
ValueError: need more than 1 value to unpack	db/trac.db
ValueError: need more than 1 value to unpack	Unknown
ValueError: need more than 1 value to unpack	Unknown
ValueError: need more than 1 value to unpack	Unknown
ValueError: need more than 1 value to unpack	Unknown
AttributeError: 'NoneType' object has no attribute 'split'	sqlite:db/trac.db(Different problem?)
AttributeError: 'NoneType' object has no attribute 'split'	sqlite:db/trac.db(Different problem?)

I think you might be right. Including the db string will already help a lot. Any additional hints would probably confuse about as often as they would help.

Would it make sense to instead include a link to the documentation? (DatabaseBackend?)

[Ryan J Ollos]

Replying to psuter:

Would it make sense to instead include a link to the documentation? (DatabaseBackend?)

I think so, and should the link specifically point to documentation on t.e.o since any local documentation will likely be unavailable?

Thanks for gathering up all of those examples. I'll take a closer look.

[Christian Boos]

Well, the link could point to TracEnvironment#DatabaseConnectionStrings, and that section be expanded a bit if needed, notably to mention how an error would manifest itself.

[Ryan J Ollos]

Replying to cboos:

I don't use it, when I "push to svn", I either commit what's on the branch wholesale or if I want to keep a svn changeset for each git commit, I do something like the following, assuming a tXYZ branch which has 4 commits on top of the last in svn (rebasing it first if needed):

Those steps worked well for me while committing to #11245. I found one additional step necessary, to copy the .svn directory from an SVN working copy of 1.0-stable into root of the Git repository. It's a nice way to apply patches from Git to SVN. Thanks!

[Ryan J Ollos]

I'll be returning to this ticket shortly, and I'm thinking that I might rework the changes to utilize the generic ConfigurationError exception with its default message, as shown in comment:4:ticket:11272. This is to avoid exposing configuration data to the end user, such as Invalid format sqlite/db/trac.db.

On the one hand it doesn't seem very harmful to expose this information to users, and it may be more convenient to just present the information to the administrator since the situation is most likely to be encountered while setting up the Trac instance. On the other hand, presenting the ConfigurationError with the default message could be consistently used across Trac, and in addition to consistency we don't have to make a decision each time a similar situation arises and decide whether the configuration data should be exposed to all users.

A third option would be a future enhancement to only show a custom error message containing configuration data (i.e. the log message) through the web interface when the user has TRAC_ADMIN (discussion in bloodhound:#589 is related). I haven't explored whether this is practical for all situations - whether a permission check will be possible in all scenarios including the one presented in bloodhound:#589.

I appreciate any comment that will help me make the right decision on this small detail.

[Peter Suter]

Replying to rjollos:

only show a custom error message containing configuration data (i.e. the log message) through the web interface when the user has TRAC_ADMIN

That sounds interesting. A nice tradeoff. In situations where it's not easily possible to check permissions, we could default to not showing the details.

But maybe this complexity is not really needed. Encouraging more people to check the logs doesn't sound so bad either.

Do we have some TracDev page where the preferred approach to (not) showing configuration details in warning and error messages is (or could be) documented?

[Ryan J Ollos]

Replying to psuter:

Replying to rjollos:

only show a custom error message containing configuration data (i.e. the log message) through the web interface when the user has TRAC_ADMIN

That sounds interesting. A nice tradeoff. In situations where it's not easily possible to check permissions, we could default to not showing the details.

But maybe this complexity is not really needed. Encouraging more people to check the logs doesn't sound so bad either.

I'm starting to think the complexity isn't needed for most cases. If there is demand for viewing the details through the web interface, maybe we could add a feature like ​th:LogViewerPlugin.

Do we have some TracDev page where the preferred approach to (not) showing configuration details in warning and error messages is (or could be) documented?

I haven't seen it documented anywhere, but I'd find documentation useful. Should we add a TracExceptions page, or is a broader category more appropriate? I'd find it useful to have the exception hierarchy displayed somewhere (​PEP-0249 is a good example), and put use details in each Exception class. ConfigurationError could describe the proper type of information to expose in the exception message.

I was also planning to review the places where exceptions are raised and see if there are places where it makes sense to replace a generic TracError with a ConfigurationError.

[Peter Suter]

I started TracDev/Exceptions. Feel free to improve on that.

Adding more details to the exception class docstrings sounds good.

[Ryan J Ollos]

I've been using ConfigurationError fairly liberally for issues that are due to improper configuration of Trac. For example, in authz_policy.py​ a ConfigurationError is raised for:

• 173: a missing [authz_policy] authz_file configuration option in trac.ini
• 183: an authz file that can't be accessed (likely the file doesn't exist or it is a permissions issue)
• 189: a missing package dependency (this will probably never be encountered after [10237] since the package is a setup.py requirement for the module, but I couldn't absolutely convince myself of that and it seemed harmless to leave it in).
• 198: exceptions raised by ConfigObj while parsing the authz file
• 201: an empty authz file

However, since ConfigurationError is defined in the config module, your narrower definition seems more valid: Exception raised when a value in the configuration file is not valid. Maybe I should be using a ParseError for 198 and 201? ParseError should probably be located in a different module such as trac.util in that case. Maybe we should have an InstallationError exception for issues such as 183 and 189?

Maybe it doesn't matter at all and I'm worrying about something irrelevant? At this point, the major differentiator I can see is having a more self-explanatory title when the exception is presented. As comment:4:ticket:11272 shows a Configuration Error, the InstallationError exception could display Installation Error.

[Peter Suter]

Note that I merely copied the existing docstring. If a broader definition would be more useful we could probably change it.

Reusing ParseError (Exception thrown for parse errors in authz files) for the 198 / 201 authz parsing issues seems fitting. But I don't see why it should be moved.

Restricting ConfigurationError to trac.config (i.e. TracIni) sounds logical. Not sure if it's a problem to reuse it for other kinds of configuration / installation issues. A new InstallationError sounds like a good alternative.

Maybe it's a bad idea to change the class hierarchy now, but would it make sense to insert it below the other two?

• TracError
  • InstallationError: There is an issue with the Trac installation.
    • ConfigurationError: There is an issue in the TracIni configuration file.
    • ParseError: There is an issue preventing parsing of an authz file.

Not sure if the name is confusing though (suggesting an error occurred during installation.)

[Ryan J Ollos]

Here are the error messages that are presented for the 3 cases in which a ConfigurationError is raised:

1. Empty database connection string:
2. Database connection string that can't be parsed:
3. Invalid path in SQLite database connection string:

Some comments:

• I moved a string to module scope so that it could be reused. I'm not sure if this is a good idea: [cd81c49c/rjollos.git]
• The tt tags will need to be changed to code tags when merging to the trunk (#11094).
• There is some discussion in this ticket about not exposing configuration information to the user. Further steps could be taken to hide the information from end-users, but it doesn't seem all that bad to expose this bit of configuration information to users because the situation is likely to be encountered only during the initial installations or after a major configuration change, and since the problem is raised quite often in tickets and on the mailing list, my thinking is that it is pain point for inexperienced users and we should make the problem and fix as obvious as possible.
• Another issue that I plan to address in a future ticket is stripping out the markup in the log messages to make them more readable. For example, maybe we can have a few rules such as replacing emphasis and tt/code tags with quotes, and putting hrefs after the text node:

  05:42:23 PM Trac[main] ERROR: can't retrieve session:
  ConfigurationError: Database connection string is empty.
  Set the <tt>database</tt> configuration option in the <a class="trac-target-new" href="http://trac.edgewall.org/wiki/TracIni#trac-section" title="TracIni documentation">[trac]</a> section of trac.ini.
  Please refer to the <a class="trac-target-new" href="http://trac.edgewall.org/wiki/TracIni#DatabaseConnectionStrings" title="Database Connection Strings documentation">documentation</a> for help.
  

  →

  05:42:23 PM Trac[main] ERROR: can't retrieve session:
  ConfigurationError: Database connection string is empty.
  Set the "database" configuration option in the [trac] (http://trac.edgewall.org/wiki/TracIni#trac-section) section of trac.ini.
  Please refer to the documentation (http://trac.edgewall.org/wiki/TracIni#DatabaseConnectionStrings) for help.
  

  This is also useful because the user is likely to encounter these exceptions while running trac-admin initenv:

  Initenv for '/home/user/Workspace/t11189/teo-rjollos.git/tracdev-pg' failed. 
  Failed to create environment.
  SQLite database not found at <tt>/home/user/Workspace/t11189/teo-rjollos.git/tracdev-pg/db/trac.db</tt>. Please refer to the <a class="trac-target-new" href="http://trac.edgewall.org/wiki/TracIni#DatabaseConnectionStrings" title="Database Connection Strings documentation">documentation</a> for help.
  Traceback (most recent call last):
    File "/home/user/Workspace/t11189/teo-rjollos.git/trac/admin/console.py", line 456, in do_initenv
      options=options)
    File "/home/user/Workspace/t11189/teo-rjollos.git/trac/core.py", line 124, in __call__
      self.__init__(*args, **kwargs)
    File "/home/user/Workspace/t11189/teo-rjollos.git/trac/env.py", line 280, in __init__
      self.create(options)
    File "/home/user/Workspace/t11189/teo-rjollos.git/trac/env.py", line 580, in create
      DatabaseManager(self).init_db()
    File "/home/user/Workspace/t11189/teo-rjollos.git/trac/db/api.py", line 249, in init_db
      connector, args = self.get_connector()
    File "/home/user/Workspace/t11189/teo-rjollos.git/trac/db/api.py", line 329, in get_connector
      path=tag.tt(args['path']), doc=_db_str_doc))
  ConfigurationError: SQLite database not found at <tt>/home/user/Workspace/t11189/teo-rjollos.git/tracdev-pg/db/trac.db</tt>. Please refer to the <a class="trac-target-new" href="http://trac.edgewall.org/wiki/TracIni#DatabaseConnectionStrings" title="Database Connection Strings documentation">documentation</a> for help.
  

  The last issue really makes me question whether it is worth having nice markup in the error message. If the error will usually be encountered during trac-admin initenv calls, then the markup will just make the message less readable.
• One major issue remains unhandled. If the username/password/hostname fragment can't be correctly parsed, then an error is no longer raised, but we'll see a traceback when trying to open the db connection:

  >>> _parse_db_str('database = postgres://trac@trac@localhost:5432/trac')
  ('database = postgres', {'path': '/trac', 'host': 'trac@localhost', 'user': 'trac', 'port': 5432})
  

  Traceback (most recent call last):
    File "/home/user/Workspace/t11189/teo-rjollos.git/trac/web/api.py", line 524, in send_error
      data, 'text/html')
    File "/home/user/Workspace/t11189/teo-rjollos.git/trac/web/chrome.py", line 983, in render_template
      message = Markup(req.session.pop('chrome.%s.%d'
    File "/home/user/Workspace/t11189/teo-rjollos.git/trac/web/api.py", line 321, in __getattr__
      value = self.callbacks[name](self)
    File "/home/user/Workspace/t11189/teo-rjollos.git/trac/web/main.py", line 268, in _get_session
      return Session(self.env, req)
    File "/home/user/Workspace/t11189/teo-rjollos.git/trac/web/session.py", line 207, in __init__
      self.get_session(sid)
    File "/home/user/Workspace/t11189/teo-rjollos.git/trac/web/session.py", line 230, in get_session
      super(Session, self).get_session(sid, authenticated)
    File "/home/user/Workspace/t11189/teo-rjollos.git/trac/web/session.py", line 77, in get_session
      with self.env.db_query as db:
    File "/home/user/Workspace/t11189/teo-rjollos.git/trac/db/api.py", line 175, in __enter__
      db = self.dbmgr.get_connection(readonly=True)
    File "/home/user/Workspace/t11189/teo-rjollos.git/trac/db/api.py", line 263, in get_connection
      db = self._cnx_pool.get_cnx(self.timeout or None)
    File "/home/user/Workspace/t11189/teo-rjollos.git/trac/db/pool.py", line 213, in get_cnx
      return _backend.get_cnx(self._connector, self._kwargs, timeout)
    File "/home/user/Workspace/t11189/teo-rjollos.git/trac/db/pool.py", line 134, in get_cnx
      raise TimeoutError(errmsg)
  TimeoutError: Unable to get database connection within 20 seconds. (OperationalError: could not translate host name "trac@localhost" to address: System error
  )
  

  Should we try to trap the TimeoutErrors?:

  trac/db/api.py

  @@ -27 +27 @@
   from trac.util.text import unicode_passwd
   from trac.util.translation import _, tag_
   
  -from .pool import ConnectionPool
  +from .pool import ConnectionPool, TimeoutError
   from .util import ConnectionWrapper
   
   
  @@ -260 +260 @@
           if not self._cnx_pool:
               connector, args = self.get_connector()
               self._cnx_pool = ConnectionPool(5, connector, **args)
  -        db = self._cnx_pool.get_cnx(self.timeout or None)
  +        try:
  +            db = self._cnx_pool.get_cnx(self.timeout or None)
  +        except TimeoutError, e:
  +            raise TracError(e)
           if readonly:
               db = ConnectionWrapper(db, readonly=True)
           return db

  I couldn't find any performance difference after the try/except was added when measuring calls to env.get_db_cnx with the timeit module.

An even simpler fix could be:

trac/db/pool.py

@@ -26 +26 @@
 from trac.util.translation import _
 
 
-class TimeoutError(Exception):
+class TimeoutError(TracError):
     """Exception raised by the connection pool when no connection has become
     available after a given timeout."""

However, it makes me wonder if it was intentional that TimeoutError not inherit from TracError. TracError existed when TimeoutError was added in [2503].

The latest proposed changes can be found in rjollos.git:t11189.3.

[Ryan J Ollos]

Replying to rjollos:

This is also useful because the user is likely to encounter these exceptions while running trac-admin initenv:

Initenv for '/home/user/Workspace/t11189/teo-rjollos.git/tracdev-pg' failed. 
Failed to create environment.
SQLite database not found at <tt>/home/user/Workspace/t11189/teo-rjollos.git/tracdev-pg/db/trac.db</tt>. Please refer to the <a class="trac-target-new" href="http://trac.edgewall.org/wiki/TracIni#DatabaseConnectionStrings" title="Database Connection Strings documentation">documentation</a> for help.
Traceback (most recent call last):
  File "/home/user/Workspace/t11189/teo-rjollos.git/trac/admin/console.py", line 456, in do_initenv
    options=options)
  File "/home/user/Workspace/t11189/teo-rjollos.git/trac/core.py", line 124, in __call__
    self.__init__(*args, **kwargs)
  File "/home/user/Workspace/t11189/teo-rjollos.git/trac/env.py", line 280, in __init__
    self.create(options)
  File "/home/user/Workspace/t11189/teo-rjollos.git/trac/env.py", line 580, in create
    DatabaseManager(self).init_db()
  File "/home/user/Workspace/t11189/teo-rjollos.git/trac/db/api.py", line 249, in init_db
    connector, args = self.get_connector()
  File "/home/user/Workspace/t11189/teo-rjollos.git/trac/db/api.py", line 329, in get_connector
    path=tag.tt(args['path']), doc=_db_str_doc))
ConfigurationError: SQLite database not found at <tt>/home/user/Workspace/t11189/teo-rjollos.git/tracdev-pg/db/trac.db</tt>. Please refer to the <a class="trac-target-new" href="http://trac.edgewall.org/wiki/TracIni#DatabaseConnectionStrings" title="Database Connection Strings documentation">documentation</a> for help.

The last issue really makes me question whether it is worth having nice markup in the error message. If the error will usually be encountered during trac-admin initenv calls, then the markup will just make the message less readable.

We could probably do a bit better overall in presentation of these excepted exception - catch the exception, print the exception error message (possibly after stripping out the markup) and avoid displaying the traceback in the console.

[Jun Omae]

Replying to rjollos:

• I moved a string to module scope so that it could be reused. I'm not sure if this is a good idea: [cd81c49c/rjollos.git]

That's not good idea. I confirmed that _("documentation") in the changes never be translated. Also, if i18n methods return Babel's LazyProxy object, the object will cache translated message. Then, we should create the string every time.

[Ryan J Ollos]

The latest proposed changes can be found in log:rjollos.git:t11189.4:

• Squashed previous set of commits and [cd81c49c/rjollos.git] was removed as suggested in comment:30.
• TimeoutError inherits from TracError so that exceptions will be trapped.
• If database connection can't be obtained and user has an authenticated session, a traceback still results. This can occurs, at least for LoginModule, while trying to retrieve the cookie from the database: trac.web.main:authenticate → trac.web.auth:authenticate → trac.web.auth:_get_name_for_cookie → trac.web.auth:_cookie_to_name​. I've proposed to fix this in [2698e6dd/rjollos.git], but I'm not sure it is the best way.

[Peter Suter]

Replying to rjollos:

• TimeoutError inherits from TracError so that exceptions will be trapped.

There are some other exception classes that don't inherit from TracError. Should (some of) these be changed as well?

[Ryan J Ollos]

Replying to psuter:

There are some other exception classes that don't inherit from TracError. Should (some of) these be changed as well?

I'm not sure, but I after I saw your comment on the wiki I added to my TODO list as an issue to investigate. I thought I'd look at the cases where they are used and make sure to test the behavior when exceptions are raised. Based on the code I've worked with, ParseError seems like an obvious candidate to inherit from TracError. Probably some or all of the others as well.

I'll make sure to update the exception hierarchy at TracDev/Exceptions if the change in [11d384c7/rjollos.git] gets pushed.

[Ryan J Ollos]

• Pushed two of the changes to 1.0-stable in [12305:12306]; merged to trunk in [12307].
• Exception hierarchy updated.

Checking for the existence of the SQLite database file in [f95af781/rjollos.git#file0] presents a problem because the connector is retrieved before the database is initialized: branches/1.0-stable/trac/db/api.py@12111:248,251#L222​. The following traceback results:

$ trac-admin new initenv
Creating a new Trac environment at /home/user/Workspace/t11189/teo-rjollos.git/new

Trac will first ask a few questions about your environment
in order to initialize and prepare the project database.

 Please enter the name of your project.
 This name will be used in page titles and descriptions.

Project Name [My Project]> new

 Please specify the connection string for the database to use.
 By default, a local SQLite database is created in the environment
 directory. It is also possible to use an already existing
 PostgreSQL database (check the Trac documentation for the exact
 connection string syntax).

Database connection string [sqlite:db/trac.db]> 

Creating and Initializing Project
Initenv for '/home/user/Workspace/t11189/teo-rjollos.git/new' failed. 
Failed to create environment.
SQLite database not found at <tt>/home/user/Workspace/t11189/teo-rjollos.git/new/db/trac.db</tt>. Please refer to the <a class="trac-target-new" href="http://trac.edgewall.org/wiki/TracEnvironment#DatabaseConnectionStrings" title="Database Connection Strings documentation">documentation</a> for help.
Traceback (most recent call last):
  File "/home/user/Workspace/t11189/teo-rjollos.git/trac/admin/console.py", line 456, in do_initenv
    options=options)
  File "/home/user/Workspace/t11189/teo-rjollos.git/trac/core.py", line 124, in __call__
    self.__init__(*args, **kwargs)
  File "/home/user/Workspace/t11189/teo-rjollos.git/trac/env.py", line 280, in __init__
    self.create(options)
  File "/home/user/Workspace/t11189/teo-rjollos.git/trac/env.py", line 580, in create
    DatabaseManager(self).init_db()
  File "/home/user/Workspace/t11189/teo-rjollos.git/trac/db/api.py", line 249, in init_db
    connector, args = self.get_connector()
  File "/home/user/Workspace/t11189/teo-rjollos.git/trac/db/api.py", line 334, in get_connector
    path=tag.tt(args['path']), doc=doc))
ConfigurationError: SQLite database not found at <tt>/home/user/Workspace/t11189/teo-rjollos.git/new/db/trac.db</tt>. Please refer to the <a class="trac-target-new" href="http://trac.edgewall.org/wiki/TracEnvironment#DatabaseConnectionStrings" title="Database Connection Strings documentation">documentation</a> for help.

So far, the only ways I can think to resolve this are:

• Remove the test for the path existence.
• Add a preinit parameter to get_connector that determines whether the exception is raised.
• Move the body of get_connector to a private method, but keep the path existence test in get_connector, allowing us to call the private method from init_db. Unfortunately _get_connector is already used for backward compatibility: branches/1.0-stable/trac/db/api.py?rev=12111&marks=329#L299​

[Jun Omae]

Sorry for my late response.

• TimeoutError inherits from TracError so that exceptions will be trapped.
• If database connection can't be obtained and user has an authenticated session, a traceback still results. This can occurs, at least for LoginModule, while trying to retrieve the cookie from the database: trac.web.main:authenticate → trac.web.auth:authenticate → trac.web.auth:_get_name_for_cookie → trac.web.auth:_cookie_to_name​. I've proposed to fix this in [2698e6dd/rjollos.git], but I'm not sure it is the best way.

In [12306], if IAuthenticator component raises a TracError, RequestDispatcher.authenticate method would skip the component and try next. I think that's not good because it unexpectedly might use other authenticators.

trac/web/main.py

@@ -138 +138 @@
                 self.log.error("Can't authenticate using %s: %s",
                                authenticator.__class__.__name__,
                                exception_to_unicode(e))
+                raise
             else:
                 if authname:
                     return authname

[Ryan J Ollos]

Replying to jomae:

In [12306], if IAuthenticator component raises a TracError, RequestDispatcher.authenticate method would skip the component and try next. I think that's not good because it unexpectedly might use other authenticators.

Re-raising the exception there results in the traceback for the case mentioned in comment:31. How about something like this?

trac/web/main.py

@@ -48 +48 @@
 from trac.util.translation import _, get_negotiated_locale, has_babel, \
                                   safefmt, tag_
 from trac.web.api import *
-from trac.web.chrome import Chrome
+from trac.web.chrome import Chrome, add_warning
 from trac.web.href import Href
 from trac.web.session import Session
 
@@ -138 +138 @@
                 self.log.error("Can't authenticate using %s: %s",
                                authenticator.__class__.__name__,
                                exception_to_unicode(e))
+                add_warning(req, _("Authentication error. "
+                                   "Please contact your administrator."))
             else:
                 if authname:
                     return authname
-        else:
-            return 'anonymous'
+        return 'anonymous'
 
     def dispatch(self, req):
         """Find a registered handler that matches the request and let

[Jun Omae]

Replying to rjollos:

Re-raising the exception there results in the traceback for the case mentioned in comment:31. How about something like this?

Okay, two things.

• We should log with traceback to solve it by administrator
• Break the loop to do not fallback authenticator

trac/web/main.py

@@ -48 +48 @@
 from trac.util.translation import _, get_negotiated_locale, has_babel, \
                                   safefmt, tag_
 from trac.web.api import *
-from trac.web.chrome import Chrome
+from trac.web.chrome import Chrome, add_warning
 from trac.web.href import Href
 from trac.web.session import Session
 
@@ -137 +137 @@
             except TracError, e:
                 self.log.error("Can't authenticate using %s: %s",
                                authenticator.__class__.__name__,
-                               exception_to_unicode(e))
-            else:
-                if authname:
-                    return authname
-        else:
-            return 'anonymous'
+                               exception_to_unicode(e, traceback=True))
+                add_warning(req, _("Authentication error. "
+                                   "Please contact your administrator."))
+                break  # don't fallback authenticator
+            if authname:
+                return authname
+        return 'anonymous'
 
     def dispatch(self, req):
         """Find a registered handler that matches the request and let

[Ryan J Ollos]

Okay, good. That is pretty much the same as the patch I was testing locally. Somehow though I posted the wrong patch in comment:37, which didn't have the break.

[Ryan J Ollos]

Changes from comment:37 committed to 1.0-stable in [12315] and merged to trunk in [12316].

[Ryan J Ollos]

Replying to rjollos:

Checking for the existence of the SQLite database file in [f95af781/rjollos.git#file0] presents a problem because the connector is retrieved before the database is initialized: branches/1.0-stable/trac/db/api.py@12111:248,251#L222​. The following traceback results: […] So far, the only ways I can think to resolve this are:

• Remove the test for the path existence.
• Add a preinit parameter to get_connector that determines whether the exception is raised.
• Move the body of get_connector to a private method, but keep the path existence test in get_connector, allowing us to call the private method from init_db. Unfortunately _get_connector is already used for backward compatibility: branches/1.0-stable/trac/db/api.py?rev=12111&marks=329#L299​

The latest proposed changes can be found in log:rjollos.git:t11189.7:

• Added unit tests for RequestDispatcher.authenticate
• Added a private _get_connector method, and the public get_connector method checks for path existence. The private _get_connector is called in init_db, which avoids the exception due to the check for existence of the SQLite database.

I am not entirely sure that the latter change is the best way to go, so I'll wait for feedback before committing.

[Jun Omae]

• Added a private _get_connector method, and the public get_connector method checks for path existence. The private _get_connector is called in init_db, which avoids the exception due to the check for existence of the SQLite database.

The changes break unit tests on PostgreSQL and MySQL. I think that might lead something wrong because DatabaseManager.get_connector() method is used from Trac plugins to generate DDL statements via to_sql(). Also, that is ineffective without check of writable permission for SQLite database. The issue often occurs if other than tracd web server.

I think it is difficult to validate perfectly database string without actual connecting database.

$ make python=25 db=postgres clean unit-test
...
TRAC_TEST_DB_URI=postgres://tracuser:password@localhost:5432/tractest?schema=tractest
...
psycopg2.ProgrammingError: schema "tractest" already exists
$ make python=25 db=mysql clean unit-test
...
TRAC_TEST_DB_URI=mysql://tracuser:password@localhost/tractest
...
_mysql_exceptions.OperationalError: (1050, "Table 'system' already exists")

[anonymous]

offtopic: milestone is moving target. Do you think you could fix remaining 6 tickets and release?

[Ryan J Ollos]

Additional changes committed to 1.0-stable in [12366:12368] and merged to trunk in [12369]. Work will continue in #11415.

[Ryan J Ollos]

Replying to Ryan J Ollos:

Replying to cboos:

I don't use it, when I "push to svn", I either commit what's on the branch wholesale or if I want to keep a svn changeset for each git commit, I do something like the following, assuming a tXYZ branch which has 4 commits on top of the last in svn (rebasing it first if needed):

Those steps worked well for me while committing to #11245. I found one additional step necessary, to copy the .svn directory from an SVN working copy of 1.0-stable into root of the Git repository. It's a nice way to apply patches from Git to SVN. Thanks!

comment:1:ticket:12358 discusses how the steps might be formally documented.