Edgewall Software
Modify

Opened 6 years ago

Closed 5 years ago

Last modified 3 years ago

#11573 closed enhancement (fixed)

log_level and log_type should be ChoiceOptions

Reported by: Ryan J Ollos Owned by: Ryan J Ollos
Priority: normal Milestone: 1.1.3
Component: general Version:
Severity: normal Keywords:
Cc: Branch:
Release Notes:

The [trac] default_date_format and default_dateinfo_format, and the [logging] log_type and log_level options are now ChoiceOptions. A ConfigurationError exception will be raised if an invalid entry is specified for one of the options in trac.ini.

API Changes:

Description

If we change log_level and log_type to ChoiceOptions, a TracError will be raised when an invalid value is used.

[logging]
log_level = DEBUG
log_type = stderror
Error

TracError: [logging] log_type: expected one of ("file", "none", "stderr", "syslog", "winlog"), got u'stderror'

This could be useful, but will be challenged by the presence of aliases:

  • ALL is an alias for DEBUG, and winlog and syslog have aliases.
  • WARN is documented as an alias for WARNING, and it appears that the default is WARNING for any unrecognized value.

Possible resolutions are:

  • Eliminate some aliases and replace them in trac.ini in a upgrade step.
  • Enhance ChoiceOption to support aliases.

I probably won't take any action on this for a while, but prepared some proposed changes in log:rjollos.git:t11573 before noticing these issues, and wanted to capture them in a ticket.

Attachments (0)

Change History (13)

comment:1 by Jun Omae, 6 years ago

I think using ChoiceOption is a good idea. I like to enhance ChoiceOption rather than add a upgrade step for the minor issue.

Also, in [2e6e9c1c/rjollos.git], documents of the log_level and log_type options wouldn't be extracted for tracini.pot.

BTW, I think a ConfigurationError will be raised for such a misconfiguration, not a TracError.

comment:2 by Ryan J Ollos, 6 years ago

Milestone: undecided1.1.3

in reply to:  1 comment:3 by Ryan J Ollos, 6 years ago

Replying to jomae:

BTW, I think a ConfigurationError will be raised for such a misconfiguration, not a TracError.

ConfigurationError is raised in the call to trac.env.open_environment and the exception is trapped in trac.web.main.dispatch_request. The exception message is passed as env_error to trac.web.main._dispatch_request and an HTTPInternalError is raised: tags/trac-1.0.1/trac/web/main.py@:494#L485. That exception is trapped (tags/trac-1.0.1/trac/web/main.py@:502#L485) and sent to the user in trac.web.main._send_user_error. The result is that the user sees:

Error

TracError: [logging] log_type: expected one of ("file", "none", "stderr", "syslog", "winlog"), got u'stderror'

So yes, a ConfigurationError is raised. I should have been more precise and said "the user sees a TracError", rather than "a TracError will be raised".

comment:4 by Ryan J Ollos, 5 years ago

[trac] default_date_format and [trac] default_dateinfo__format can also be ChoiceOptions: log:rjollos.git:t11573.1.

This results in a significant change in behavior in that a ConfigurationError is raised when an invalid option is specified in trac.ini (similar to the change in #10285).

It was suggested in comment:2:ticket:10285 to add a new class rather than having OrderedExtensionOption raise an exception. Reconsidering this behavior, it might make sense to have a strict parameter for the ChoiceOption, ExtensionOption and OrderedExtensionOption. The strict option would ultimately determine whether the error is only logged, or also presented to the UI. The mechanism could be to have two exception classes that get handled differently.

comment:5 by Ryan J Ollos, 5 years ago

The changes in log:rjollos.git:t11573.1 could be pushed to the trunk rather than 1.0-stable so that we have time to get feedback on whether it's too extreme of a reaction to show a ConfigurationError on the web interface in response to an invalid configuration value.

comment:6 by Ryan J Ollos, 5 years ago

Release Notes: modified (diff)

Changes from comment:4 committed to trunk in [13298].

comment:7 by Ryan J Ollos, 5 years ago

Owner: set to Ryan J Ollos
Status: newassigned

comment:8 by Ryan J Ollos, 5 years ago

LOG_LEVELS extracted to trac.log in [13357]. We might want to make this a dictionary or other type that allows aliases can be specified.

comment:9 by Ryan J Ollos, 5 years ago

Also, making log_type a ChoiceOption would restrict the ability to add new loggers unless we also provide a plugable interface for new logger types.

in reply to:  9 comment:10 by Ryan J Ollos, 5 years ago

Replying to rjollos:

Also, making log_type a ChoiceOption would restrict the ability to add new loggers unless we also provide a plugable interface for new logger types.

I'll propose a new interface in #11874.

For now, the proposed changes that convert the two Options to ChoiceOptions can be found in: log:rjollos.git:t11573.2. I'll add some tests before committing.

comment:11 by Ryan J Ollos, 5 years ago

Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

Committed to trunk in [13502].

Last edited 5 years ago by Ryan J Ollos (previous) (diff)

comment:12 by Ryan J Ollos, 3 years ago

In r15116, fixed string interpolation error in r13502. Merged to trunk in r15117.

comment:13 by Ryan J Ollos, 3 years ago

Regression with these changes reported in #12779.

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'.
to as closed The owner will be changed from Ryan J Ollos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.