Edgewall Software
Modify

Opened 13 years ago

Last modified 2 weeks ago

#3037 new defect

Multiple bugs in trac.config

Reported by: Alec Thomas Owned by:
Priority: low Milestone: next-major-releases
Component: general Version: devel
Severity: minor Keywords: global config
Cc: Branch:
Release Notes:
API Changes:

Description

While testing some global configuration changes, I noticed they weren't working with InterTrac. After some investigation it seems there are a few problems with the current config module:

  1. Section.__contains__() does not use the global config, nor does Section.get()
  2. ConfigParser ignores case, but config.Configuration._defaults does not.
  3. ConfigParser.read() overlays previous configuration. The issue with this is that we use read() when a configuration modification is detected. Deleted options are not removed.

The attached patch fixes the first two issues. It also updates one of the config unit tests, as it can now fail if a global configuration file has entries (NB. I'm not sure whether the fix in the unit test is too hackish?)

The third issue would require some kind of dirty configuration list, for both the site and env configs. I'm not sure the issue is bad enough to warrant the ugliness.

Attachments (2)

fixed-config.diff (4.4 KB ) - added by Alec Thomas 13 years ago.
Configuration module fixes
fixed-config+cleaner.diff (5.3 KB ) - added by Alec Thomas 13 years ago.
Previous patch plus save() now only saves modified values

Download all attachments as: .zip

Change History (32)

by Alec Thomas, 13 years ago

Attachment: fixed-config.diff added

Configuration module fixes

comment:1 by Alec Thomas, 13 years ago

Thinking about it, the fix could also break other unit tests that rely on the global config not being loaded so perhaps a test-suite wide solution is preferable.

comment:2 by Alec Thomas, 13 years ago

And on a semi-related note, what's the opinion on not saving values to the env config that are either the Trac default or the site config default? This would make the env config a lot cleaner I think, although perhaps having it populated is desirable as hints to the user?

by Alec Thomas, 13 years ago

Attachment: fixed-config+cleaner.diff added

Previous patch plus save() now only saves modified values

comment:3 by Alec Thomas, 13 years ago

I think only saving options that differ from the default is a good idea. If users need examples there's always trac.ini.sample.

comment:4 by Christopher Lenz, 13 years ago

Owner: changed from Jonas Borgström to Christopher Lenz
Status: newassigned

Agreed about only saving values that differ from the default. I'll try the patch later.

comment:5 by Christopher Lenz, 13 years ago

Patch integrated in [3186]. Leaving this open for point 3).

comment:6 by Alec Thomas, 13 years ago

Owner: changed from Christopher Lenz to Alec Thomas
Status: assignednew

I'll take a look at point 3.

comment:7 by Alec Thomas, 13 years ago

Status: newassigned

comment:8 by Alec Thomas, 13 years ago

This is tricky when the global config has an option set but the in-memory option is remove()ed. All is fine until the config is saved and reloaded. The environments config will not have an entry for the option, so the global default will be used, consequently ignoring the remove().

Saving the list of options that have been removed to a special [removed_options] section in the environment's config would solve this, but is spectacularly ugly.

I propose we simply remove the Configuration.remove() method for 0.10. It is not used at all in Trac core, but is used by WebAdmin and two plugins on TracHacks (HackInstallPlugin and GeneralLinkSyntaxPlugin). From a cursory examination I suspect all of these can be changed to use sentinel values or defaults.

comment:9 by Matthew Good, 13 years ago

Why should't it fall back on the global config when an option is removed? This would be consistent with the expected behavior when an option is deleted from the file directly.

comment:10 by anonymous, 13 years ago

Resolution: fixed
Status: assignedclosed

in reply to:  9 comment:11 by Alec Thomas, 13 years ago

Priority: highlow
Resolution: fixed
Severity: majorminor
Status: closedreopened

Replying to mgood:

Why should't it fall back on the global config when an option is removed? This would be consistent with the expected behavior when an option is deleted from the file directly.

That is definitely one way to look at it, and when done manually, probably expected. But from a programmatic perspective I think remove()ing an item should be more explicit. eg. WebAdmin has self.config.remove('logging', 'log_level') which may or may not actually reset the log level, depending on the global setting. If remove() can't be relied on to actually do the job, why have it? If its behaviour is to reset to the default, perhaps it would be preferable to rename it to set_default() or similar.

(also unclosing this ticket, but lowering priority/severity as now the first two issues are closed it's not that much of a problem)

comment:12 by Christian Boos, 13 years ago

Milestone: 0.100.10.1

I also think that removing an option should only remove it in the given config file.

I've not yet looked into the details of the WebAdmin, but ideally one should be able to:

  • edit either the local or the global configuration entries (defaults to editing the local entries)
  • make a difference between setting an option to an empty value (i.e. override) and unsetting an option (i.e. revert to default)

comment:13 by Christian Boos, 13 years ago

Doesn't the fix for #3620 (i.e. r3744) also help here?

in reply to:  13 comment:14 by Matthew Good, 13 years ago

Replying to cboos:

Doesn't the fix for #3620 (i.e. r3744) also help here?

No, I think these are all separate issues.

in reply to:  12 comment:15 by simon-code@…, 12 years ago

I have just added ticket #5135 with patch that has some bearing on this issue, specifically the senctence in point 3 in description: "Deleted options are not removed."

It is more a multi-process issue in my use context (allthough likely very common), but it has bearing on the fact that remove() also needs to handle better a third case "unset" (in addition to "empty" and "default"). Current parsing has no idea on how to completely "unset" an existing setting - removing all traces of it.

comment:16 by Christian Boos, 11 years ago

Milestone: 0.10.5
Resolution: worksforme
Status: reopenedclosed

This mostly works now in 0.11 and it's not going to change in 0.10-stable.

in reply to:  16 comment:17 by osimons, 11 years ago

Milestone: 0.12
Resolution: worksforme
Status: closedreopened

Replying to cboos:

This mostly works now in 0.11 and it's not going to change in 0.10-stable.

Mostly, yes. But not quite. I've left it open due to the summary in comment:15 - we need the ability to unset working variables to make administration of larger Trac installs more flexible.

Take the example of a default [milestone-groups] implemented in a shared/global trac.ini. In my local instance I would like the ability to remove active settings and split it into assigned and unsassigned for instance. There currently is no way to do this, and 'Active ticket' will appear in view regardless. That is what is left for this ticket - if we deem that important.

One way could be to use ! as the value for the setting, and if so we keep it the setting behind the scenes as a value for writing to updated config, but when using config.get() and config.has_option() we report it as if it did not exist anywhere. So to nullify any effect of a shared config from my example, I would include local settings like:

[milestone-groups]

active = !
active.order = !
active.css_class = !

; New settings here...

The same applies to other groups such as [ticket-custom] and [ticket-workflow] that will both 'bleed' into the local config without any way to fully remove settings. They can be redefined, but not removed.

comment:18 by Christian Boos, 11 years ago

Ok, but I'd prefer:

[milestone-groups]

!active =
!active.order =
!active.css_class =

as it would be more general: it would also work for the entries where "!" is valid content. It is also easier to explain: the key is negated, the entry itself is disabled.

Of course, we would have to forbid having a leading "!" in valid configuration keys, but I think that's a guideline that is easier to enforce than not using "!" in values, as it concerns us and plugin writers, not Trac administrators.

comment:19 by osimons, 10 years ago

Owner: changed from Alec Thomas to osimons
Status: reopenednew

I'll look at this (at some stage).

comment:20 by Christian Boos, 8 years ago

Milestone: next-major-0.1X0.13

… or me, while working on the #7378 topic.

Let's start by saying we need some clearer terminology and hence a more explicit API:

  • remove() a setting should be just like removing a setting from the .ini file. It's no longer specified at the level of that .ini file, so the actual value comes from the parent(s) .ini or the ultimately from the default
  • now as mentioned above we also need to be able to simulate a removal of a setting present in a parent .ini file, and this without having to change the value, which wouldn't be enough anyway if what we want to dispose of is the presence of the key itself or by extension of an entire section. The idea I proposed in comment:18 would be adequate for this. Furthermore it easily extends to marking a whole section as non-existing (e.g. [!milestone-groups]); the API for this could be Section.negate(key=None) (if None, the whole section is negated). I think this fits well with both the intended meaning and the suggested use of ! as a key or section prefix.

comment:21 by Remy Blank, 7 years ago

Milestone: 1.01.0-triage

Preparing for 1.0.

comment:22 by Ryan J Ollos, 4 years ago

Milestone: next-stable-1.0.x1.1.6
Owner: changed from osimons to Ryan J Ollos
Status: newassigned

comment:23 by Ryan J Ollos, 4 years ago

Milestone: 1.1.61.1.7

comment:24 by Ryan J Ollos, 4 years ago

Milestone: 1.1.71.2

Milestone renamed

comment:25 by Ryan J Ollos, 4 years ago

[14275] adds test coverage for some of the issues cited in this thread. Eventually I'd like to separate the Configuration class tests fully from the filesystem integration tests. I'll address the feature in comment:20 before closing.

comment:26 by Ryan J Ollos, 3 years ago

Milestone: 1.21.3.1

comment:27 by Christian Boos, 3 years ago

Milestone: 1.3.1next-dev-1.3.x

Moving to some later 1.3.x version.

comment:28 by Ryan J Ollos, 6 weeks ago

Milestone: next-dev-1.3.xnext-dev-1.5.x

Milestone renamed

comment:29 by Ryan J Ollos, 2 weeks ago

Milestone: next-dev-1.5.xnext-major-releases

comment:30 by Ryan J Ollos, 2 weeks ago

Owner: Ryan J Ollos removed
Status: assignednew

Modify Ticket

Change Properties
Set your email in Preferences
Action
as new The ticket will remain with no owner.
The ticket will be disowned. Next status will be 'new'.
as The resolution will be set. Next status will be 'closed'.
The owner will be changed from (none) to anonymous. Next status will be 'assigned'.

Add Comment


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