Do not overwrite existing attributes #8728
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is a safeguard against introducing a bug. This edit prevents the config loading code from overwriting the value of an existing attribute. The config loading procedure automatically creates attributes of the GalaxyAppConfiguration object for each property in the schema. If the schema contains a property with the same name as an existing attribute (e.g., `reloadable_options`, `schema`, etc.), that attribute will be overwritten. Worse, if the call to `_set_config_base()` is moved before the call to `_create_attributes_from_raw_config()`, the values of `config_dir` and `data_dir` will be set to `None`, because those are present in the schema. This fix prevents this from happening.
nsoranzo
reviewed
Sep 30, 2019
lib/galaxy/config/__init__.py
Outdated
| if not hasattr(self, key): | ||
| setattr(self, key, value) | ||
| else: | ||
| log.debug("Can't override existing attribute %s ", key) |
There was a problem hiding this comment.
Suggested change
| log.debug("Can't override existing attribute %s ", key) | |
| log.debug("Can't override existing attribute '%s'", key) |
or should that be a full-blown exception instead?
There was a problem hiding this comment.
Yeah, probably. I want to think a bit and see if I can come up with a use case for when we want just a warning.. If I can't think of one, I'll change this shortly.
There was a problem hiding this comment.
It may be safer to raise an Exception now and change the code if/when an use case emerges.
There was a problem hiding this comment.
Yes, you are right. Changing.
There was a problem hiding this comment.
Does this look right?
raise ConfigurationError("Attempting to override existing attribute %s ", key)
There was a problem hiding this comment.
I'd write it as:
if hasattr(self, key):
raise ConfigurationError("Attempting to override existing attribute '%s'" % key)
setattr(self, key, value)
|
Error in circleci is unrelated (nondeterministic test) |
nsoranzo
approved these changes
Oct 1, 2019
jdavcs
added a commit
to jdavcs/galaxy
that referenced
this pull request
Feb 4, 2020
This commit drops the exception that guarded against accidental overwriting of existing attributes, and instead ignores the setattr call, logging a debug message (which was initialy proposed in galaxyproject#8728). Rationale: with the previous version, base configuration properties (config_dir, data_dir, etc.) had to be set *after* this method was called (otherwise an exeption was raised), but *before* any path resolving code (because these properties are used for path resolution). As a result, config loading required a rigid sequence of steps: 1: load defaults and kwargs, create all config attributes. 2: set base configuration properties [CANNOT MOVE THIS] 3: resolve paths, do the rest: load individual properties, etc. However, setting base configuration properties is a special case that is *independent of schema loading*, is handled separately (in set_config_base), and takes presendence over schema loading procedures. This modification makes config loading more flexible, and logical: 1. set base configuration properties 2. do the rest (now any refactoring can go here)
jdavcs
added a commit
to jdavcs/galaxy
that referenced
this pull request
Feb 5, 2020
This commit drops the exception that guarded against accidental overwriting of existing attributes, and instead ignores the setattr call, logging a debug message (which was initialy proposed in galaxyproject#8728). Rationale: with the previous version, base configuration properties (config_dir, data_dir, etc.) had to be set *after* this method was called (otherwise an exeption was raised), but *before* any path resolving code (because these properties are used for path resolution). As a result, config loading required a rigid sequence of steps: 1: load defaults and kwargs, create all config attributes. 2: set base configuration properties [CANNOT MOVE THIS] 3: resolve paths, do the rest: load individual properties, etc. However, setting base configuration properties is a special case that is *independent of schema loading*, is handled separately (in set_config_base), and takes presendence over schema loading procedures. This modification makes config loading more flexible, and logical: 1. set base configuration properties 2. do the rest (now any refactoring can go here)
jdavcs
added a commit
to jdavcs/galaxy
that referenced
this pull request
Feb 15, 2020
This commit drops the exception that guarded against accidental overwriting of existing attributes, and instead ignores the setattr call, logging a debug message (which was initialy proposed in galaxyproject#8728). Rationale: with the previous version, base configuration properties (config_dir, data_dir, etc.) had to be set *after* this method was called (otherwise an exeption was raised), but *before* any path resolving code (because these properties are used for path resolution). As a result, config loading required a rigid sequence of steps: 1: load defaults and kwargs, create all config attributes. 2: set base configuration properties [CANNOT MOVE THIS] 3: resolve paths, do the rest: load individual properties, etc. However, setting base configuration properties is a special case that is *independent of schema loading*, is handled separately (in set_config_base), and takes precendence over schema loading procedures. This modification makes config loading more flexible, and logical: 1. set base configuration properties 2. do the rest (now any refactoring can go here) (cherry-picked from closed galaxyproject#9307)
jdavcs
added a commit
to jdavcs/galaxy
that referenced
this pull request
Mar 2, 2020
This commit drops the exception that guarded against accidental overwriting of existing attributes, and instead ignores the setattr call, logging a debug message (which was initialy proposed in galaxyproject#8728). Rationale: with the previous version, base configuration properties (config_dir, data_dir, etc.) had to be set *after* this method was called (otherwise an exeption was raised), but *before* any path resolving code (because these properties are used for path resolution). As a result, config loading required a rigid sequence of steps: 1: load defaults and kwargs, create all config attributes. 2: set base configuration properties [CANNOT MOVE THIS] 3: resolve paths, do the rest: load individual properties, etc. However, setting base configuration properties is a special case that is *independent of schema loading*, is handled separately (in set_config_base), and takes precendence over schema loading procedures. This modification makes config loading more flexible, and logical: 1. set base configuration properties 2. do the rest (now any refactoring can go here) (cherry-picked from closed galaxyproject#9307)
jdavcs
added a commit
to jdavcs/galaxy
that referenced
this pull request
Mar 9, 2020
This commit drops the exception that guarded against accidental overwriting of existing attributes, and instead ignores the setattr call, logging a debug message (which was initialy proposed in galaxyproject#8728). Rationale: with the previous version, base configuration properties (config_dir, data_dir, etc.) had to be set *after* this method was called (otherwise an exeption was raised), but *before* any path resolving code (because these properties are used for path resolution). As a result, config loading required a rigid sequence of steps: 1: load defaults and kwargs, create all config attributes. 2: set base configuration properties [CANNOT MOVE THIS] 3: resolve paths, do the rest: load individual properties, etc. However, setting base configuration properties is a special case that is *independent of schema loading*, is handled separately (in set_config_base), and takes precendence over schema loading procedures. This modification makes config loading more flexible, and logical: 1. set base configuration properties 2. do the rest (now any refactoring can go here) (cherry-picked from closed galaxyproject#9307)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a safeguard against introducing a bug. This edit prevents the
config loading code from overwriting the value of an existing attribute.
The config loading procedure automatically creates attributes of the
GalaxyAppConfiguration object for each property in the schema. If the
schema contains a property with the same name as an existing attribute
(e.g.,
reloadable_options,schema, etc.), that attribute will beoverwritten. Worse, if the call to
_set_config_base()is moved beforethe call to
_create_attributes_from_raw_config(), the values ofconfig_diranddata_dirwill be set toNone, because those arepresent in the schema. This fix prevents this from happening.