Edgewall Software
Modify

Opened 11 years ago

Closed 4 years ago

#6551 closed defect (fixed)

IntOption and BoolOption values saved even when not differing from default

Reported by: hyuga <hyugaricdeau@…> Owned by: Ryan J Ollos
Priority: normal Milestone: 1.1.5
Component: general Version: 0.11b1
Severity: normal Keywords: config
Cc: osimons
Release Notes:

Fixed incorrect highlighting of unmodified values in the Configuration section of the About Trac page.

API Changes:
  • Configuration.get always returns a string, as documented.
  • Configuration values are normalized before writing to file.
  • Moved get_configinfo (added in 1.1.2) from Environment class to a function of the config module.

Description

The Configuration.save method fails at checking whether IntOptions and BoolOptions differ from their default values. So when calling Configuration.save, if a value has been set on an IntOption or BoolOption, that value is saved to the file even if it is the same as the default for that option, which is not the desired behavior I don't think.

Attachments (3)

section_get_unicode-r7592.patch (1020 bytes ) - added by ebray 10 years ago.
I'm not entirely sure how I feel about this patch, but it does fix the problem as described. Ensures that Section.get() always returns a unicode string, and currectly returns true/false for BoolOptions and returns an integer string for IntOptions (even if the value is 0).
section_get_unicode-r7596.patch (1.2 KB ) - added by ebray 10 years ago.
A new version of the patch. I found that if an option's default value is None, then Option.__get__ would return u'None' which is certainly no good. I'm still not sure I like this fix, but it gets the job done, and always returns the correct value now.
Screen Shot 2015-04-14 at 8.44.29 PM.png (60.1 KB ) - added by Ryan J Ollos 4 years ago.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 11 years ago by hyuga <hyugaricdeau@…>

It looks like the actual problem here is that Section.get and by extension Configuration.get don't always return a string as documented.

The problem is that if the option is not in the trac.ini file, or the global trac.ini file, its default value is retrieved from the Option registry. In the case of an IntOption, for example, the default value will be an integer. The problem is in how the value is returned. From Section.get:

        if not value:
            return u''
        elif isinstance(value, basestring):
            return to_unicode(value)
        else:
            return value

This leads to all kinds of inconsistencies. For example, if the default value for the option is 0, this returns u'' instead of u'0' as one would expect. Or if the default value is 100000 it will return 100000 as an int.

It should probably be something more like this:

        if value is None:
            return u''
        else:
            return to_unicode(value)

comment:2 Changed 11 years ago by hyuga <hyugaricdeau@…>

I also noticed that there is some inconsistency with the default values from BoolOptions. For most of them, the defaults are set to 'true' or 'false' as it would appear in the file. But color_scale's default in versioncontrol.web_ui.browser is True., so with my suggested fix this would be returned as u'True', which is still not good.

comment:3 Changed 11 years ago by hyuga <hyugaricdeau@…>

Milestone: 0.11.1

Suggesting a milestone for this since it seems to have gotten lost in the pile.

comment:4 Changed 10 years ago by osimons

Owner: changed from Jonas Borgström to osimons

I'll put it on my to-do.

Changed 10 years ago by ebray

I'm not entirely sure how I feel about this patch, but it does fix the problem as described. Ensures that Section.get() always returns a unicode string, and currectly returns true/false for BoolOptions and returns an integer string for IntOptions (even if the value is 0).

Changed 10 years ago by ebray

A new version of the patch. I found that if an option's default value is None, then Option.__get__ would return u'None' which is certainly no good. I'm still not sure I like this fix, but it gets the job done, and always returns the correct value now.

comment:5 Changed 10 years ago by Christian Boos

Milestone: 0.11-retriage0.11.4

comment:6 Changed 10 years ago by osimons

Cc: osimons added

comment:7 Changed 9 years ago by Christian Boos

Keywords: config added
Priority: lownormal
Severity: minornormal

comment:8 Changed 4 years ago by Ryan J Ollos

Milestone: next-minor-0.12.xnext-stable-1.0.x

comment:9 Changed 4 years ago by Ryan J Ollos

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

Changed 4 years ago by Ryan J Ollos

comment:10 Changed 4 years ago by Ryan J Ollos

Related issue. Several IntOptions and BoolOptions are shown as modified on the About page though they still have the default value:

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

comment:11 Changed 4 years ago by Ryan J Ollos

Section.get states that it will return a string, so it seems clear that is a defect we should fix. We could use Option.dumps to convert the default option value to a string in Section.get. I'm debating whether Section.defaults should also return a dictionary of strings. Alternatively, Option.dumps could be called in Environment.get_configinfo.

The proposed changes are captured in log:rjollos.git:t6551. I'm still working to understand the steps required to produce the problem reported in comment:description.

comment:12 Changed 4 years ago by Ryan J Ollos

I'm considering moving Environment.get_configinfo to a function of the config module since it's not generally useful as a method of the Environment class. Locating the code in the config module could also make maintenance easier.

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

comment:13 Changed 4 years ago by Jun Omae

Currently, Option.dumps() with True and False serializes 'enabled' and 'disabled'. If enabled for BoolOption which has 'true' as default is set, Trac determines the value is different from the default.

$ ~/venv/trac/1.1.4/bin/python
Python 2.6.9 (default, Oct 22 2014, 19:47:46)
[GCC 4.6.3] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> from trac.test import EnvironmentStub
>>> from trac.loader import load_components
>>> from trac.config import Option, BoolOption
>>> env = EnvironmentStub()
>>> load_components(env)
>>> set(o.default for o in Option.get_registry().itervalues() if isinstance(o, BoolOption))
set([False, 'true', 'false', True])

It would be good to add Option.normalize(), e.g. True, 1, 'true' to 'enabled'. Additional proposed changes in jomae.git@t6551.2.

comment:14 Changed 4 years ago by Ryan J Ollos

Thank you for the additional changes. I'll rebase the changes on the trunk now that #11982 is committed.

comment:15 Changed 4 years ago by Ryan J Ollos

Related to the comment in #11437 about having a trac-admin $ENV config clean command, it could be useful to highlight the rows on the About page differently when the option is not defined in any enabled Component. We'd need special handling for options that only have a ConfigSection, such as ticket-workflow, or somehow indicate that arbitrarily defined options are allowed for a given section.

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

comment:16 Changed 4 years ago by Ryan J Ollos

Minor refactoring of config module committed in [14017:14018].

comment:17 Changed 4 years ago by Ryan J Ollos

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

Committed to trunk in [14050]. I'll work in #3037 to refactor the test suite and improve test coverage.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted.
to 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.