Introduce the ConfigurationIssue system #307

dankwart-de · 2018-11-13T09:55:04Z

Add the IConfigurationIssueInterface, and the ConfigurationIssue class
and the ConfigurationIssueLevel and ConfigurationIssueTemplate
enumerations.

Configuration issues are stored for configuration objects and for
configuration value objects.
They are either of type info, warning or error.
Issues in configuration values are passed to the configuration on
request, they are not stored there! This has changed to previous
versions of Roddy.

Implement the IConfigurationIssueInterface interface in Configuration,
ConfigurationValue.

The interface adds the methods: hasWarnings/Errors, addWarning/Error,
getWarnings/Errors.
getWarning/Error returns a copy of the underlying collections in its
current implementations.

Move ConfigurationTest to ConfigurationSpec, add a test for error and
warning passthrough to the configuration object.

Configuration class:

Rename Configuration.hasErrors to hasLoadErrors.
Groovyfy some code in the Configuration class.

Analysis class:

Rename Analysis.java to Analysis.groovy and Groovyfy it.
Rebuild and extend the print errors and warning methods in Analysis
class to match the new possibilites and make the code more clear.

Changes to ConfigurationValue

Add constructor methods for int, float, double, bool to
ConfigurationValue.
Enable auto-validation of ConfigurationValue objects upon their
construction.
Groovyfy some the class.

The Enumeration class has the method getDefaultCValueTypeEnumeration()
now. The method will return a default list of available configuration
value types and their default validators.
Also did a code cleanup to Groovyfy it.

Move ConfigurationValueValidator.java to
ConfigurationValueValidator.groovy and cleanup the class.
Implement the IConfigurationIssueInterface and add proper fields and
methods.

Rename DefaultValidator.java to DefaultValidator.groovy and Groovyvy
Extend the DefaultValidator class with checkDollarSignUsage(). The
method now stores its errors and warnings in their fields in the parent
class.

Add test for proper dollar character usage to the ConfigurationFactory
test.

Add ConfigurationIssueSpec test class.

Add the DefaultValidatorSpec test class.

Remove the ConfigurationTest class.

Add the IConfigurationIssueInterface, and the ConfigurationIssue class and the ConfigurationIssueLevel and ConfigurationIssueTemplate enumerations. - Configuration issues are stored for configuration objects and for configuration value objects. - They are either of type info, warning or error. - Issues in configuration values are passed to the configuration on request, they are not stored there! This has changed to previous versions of Roddy. Implement the IConfigurationIssueInterface interface in Configuration, ConfigurationValue. - The interface adds the methods: hasWarnings/Errors, addWarning/Error, getWarnings/Errors. - getWarning/Error returns a copy of the underlying collections in its current implementations. Move ConfigurationTest to ConfigurationSpec, add a test for error and warning passthrough to the configuration object. Configuration class: - Rename Configuration.hasErrors to hasLoadErrors. - Groovyfy some code in the Configuration class. Analysis class: - Rename Analysis.java to Analysis.groovy and Groovyfy it. - Rebuild and extend the print errors and warning methods in Analysis class to match the new possibilites and make the code more clear. Changes to ConfigurationValue - Add constructor methods for int, float, double, bool to ConfigurationValue. - Enable auto-validation of ConfigurationValue objects upon their construction. - Groovyfy some the class. The Enumeration class has the method getDefaultCValueTypeEnumeration() now. The method will return a default list of available configuration value types and their default validators. Also did a code cleanup to Groovyfy it. Move ConfigurationValueValidator.java to ConfigurationValueValidator.groovy and cleanup the class. Implement the IConfigurationIssueInterface and add proper fields and methods. Rename DefaultValidator.java to DefaultValidator.groovy and Groovyvy Extend the DefaultValidator class with checkDollarSignUsage(). The method now stores its errors and warnings in their fields in the parent class. Add test for proper dollar character usage to the ConfigurationFactory test. Add ConfigurationIssueSpec test class. Add the DefaultValidatorSpec test class. Remove the ConfigurationTest class.

Added a new constructor for elevated values to the ConfigurationValue class. This also speeds up the code, as the validation result will just be taken from the parent value. Otherwise, an unnecessary revalidation would occur and they are quite expensive. Changed the toString of ConfigurationIssue to print out the message instead of some made up text. Added equals and hashCode to EnumerationValue to make them comparable. Add printErrorsAndWarnings() to Analysis to allow a single call to print out warnings, info messages and errors. Add detailed warnings and error output for condensed messages to the detailed logfile. Added more tests.

vinjana

First, nice summary of the changes. Thanks!

Not fully done yet, but here are some comments.

vinjana · 2018-11-14T12:27:43Z

RoddyCore/test/src/de/dkfz/roddy/config/validation/DefaultValidatorSpec.groovy

+        value                   | type     | expected | expectedWarnings                          | expectedErrors
+        'abc${var}bcd${var}'    | 'string' | true     | []                                        | []
+        'abc${var}bcd${var}def' | 'string' | true     | []                                        | []
+        'abc$'                  | 'string' | false    | [unattachedDollarCharacter.expand('bla')] | []


What about escaped dollar signs here?

'\\$' | 'string' | true | [] | [] 'abc\\$' | 'string' | true | [] | [] 'abc\\${' | 'string' | true | [] | [] 'a\\${b}c' | 'string' | true | [] | []

What about "empty" dollar signs, i.e. dollars followed by non-variable characters?

'$' | 'string' | false | ["lonely variable initializer '$'"] | [] 'abc$\\{' | 'string' | false | ["lonely variable initializer '$'"] | []

What about unmatched braces?

'ab${c' | 'string' | false | ["non-terminated variable expression"] | []

Another -- aberrant -- input to check:

'${}' | 'string' | true | ["empty variable expression"] | []

This case is actually no so unlikely to happen.

a='{' b='}' c='\\$${a}${b}'

I did a lot here. Check it out.

RoddyCore/test/src/de/dkfz/roddy/config/ConfigurationFactoryTest.groovy

RoddyCore/test/src/de/dkfz/roddy/config/ConfigurationIssueSpec.groovy

RoddyCore/test/src/de/dkfz/roddy/config/ConfigurationFactoryTest.groovy

RoddyCore/src/de/dkfz/roddy/config/Enumeration.groovy

RoddyCore/src/de/dkfz/roddy/config/validation/DefaultValidator.groovy

RoddyCore/src/de/dkfz/roddy/core/Analysis.groovy

RoddyCore/test/src/de/dkfz/roddy/config/validation/DefaultValidatorSpec.groovy

ConfigurationValue: - Fix the constructor for "double" values - Add the replaceObsoleteVariableIdentifiers and call it from the constructor. The method replaces several older malformed values ($VAL) to their proper form (${VAL}). - Remove the methods: * replaceString * toFile(), toFile(Map<String, String> replacements) * replaceConfigurationBasedValues * toString(List<String>) - Consolidate and cleanup the methods toFile(Analysis, DataSet) and toFile(Analysis). The methods now use the normal variable replacement feature of Roddy instead of the removed replace method - Make the field variableDetection static final - Make toEvaluatedValue final and extend its argument list Add a on-null-check to DefaultValidator.checkDollarSignUsage Analysis: - Clean up and groovyfy the class - Change the getConfiguration method so that it will return a configuration which knows values like USERNAME and USERGROUP DataSet now contains a getConfiguration method, which knows the different variants of DATASET. For internal use only! Add tests for variable replacement to ConfigurationValueSpec Add ExecutionService and FileSystemAccessProvider init code to some test classes

vinjana · 2018-11-20T12:21:11Z

RoddyCore/test/src/de/dkfz/roddy/config/validation/DefaultValidatorSpec.groovy

+        value                   | type     | expected | expectedWarnings                          | expectedErrors
+        'abc${var}bcd${var}'    | 'string' | true     | []                                        | []
+        'abc${var}bcd${var}def' | 'string' | true     | []                                        | []
+        'abc$'                  | 'string' | false    | [unattachedDollarCharacter.expand('bla')] | []


Another -- aberrant -- input to check:

'${}' | 'string' | true | ["empty variable expression"] | []

This case is actually no so unlikely to happen.

a='{' b='}' c='\\$${a}${b}'

vinjana · 2018-11-20T12:53:29Z

RoddyCore/src/de/dkfz/roddy/config/ConfigurationIssue.groovy

+    static enum ConfigurationIssueTemplate {
+        unattachedDollarCharacter(
+                ConfigurationIssueLevel.CVALUE_WARNING,
+                "Several variables in your configuration contain one or more dollar signs. As this might impose problems in your cluster jobs, check the entries in your job configuration files. See the extended logs for more information.",


Yes, these values should not get warnings. The second option you propose I do not understand, but certainly users should not get worried unnecessarily.

vinjana · 2018-11-20T12:54:33Z

RoddyCore/src/de/dkfz/roddy/config/ConfigurationIssue.groovy

+        unattachedDollarCharacter(
+                ConfigurationIssueLevel.CVALUE_WARNING,
+                "Several variables in your configuration contain one or more dollar signs. As this might impose problems in your cluster jobs, check the entries in your job configuration files. See the extended logs for more information.",
+                "The variable named '#REPLACE_0#' contains one or more dollar signs, which do not belong to a Roddy variable definition (\${variable identifier}). This might impose problems, so make sure, that your results job configuration is created in the way you want."


Also: Too wordy message.

vinjana · 2018-11-20T12:55:06Z

RoddyCore/src/de/dkfz/roddy/config/ConfigurationValue.groovy

+    /**
+     * API Level 3.4+
+     */
+    final boolean invalid


vinjana · 2018-11-20T12:56:00Z

RoddyCore/src/de/dkfz/roddy/config/ConfigurationValue.groovy

+    /**
+     * API Level 3.4+
+     */
+    ConfigurationValue(Configuration config = null, String id, boolean value) {


o.k. clarified -- Groovy idiom.

RoddyCore/test/src/de/dkfz/roddy/config/validation/DefaultValidatorSpec.groovy

ConfigurationIssue: - Changed some messages in regards to the Pull Request - Added the inproperVariableExpression issue - Change name of unattachedDollarCharacter to detachedDollarCharacter ConfigurationValue: - Moved the getDefaultCValueTypeEnumeration from Enumeration to here - Make "invalid" field deprecated and remove it internally from isInvalid DefaultValidator: - Changed name of checkDataTypes to isConfigurationValueTypeCorrect - Add another check for variable imports - Change name of checkDollarSignUsage to areDollarCharactersProperlyUsed - Add tests for the new method

ConfigurationValue: - Fix the constructor for "double" values - Add the replaceObsoleteVariableIdentifiers and call it from the constructor. The method replaces several older malformed values ($VAL) to their proper form (${VAL}). - Remove the methods: * replaceString * toFile(), toFile(Map<String, String> replacements) * replaceConfigurationBasedValues * toString(List<String>) - Consolidate and cleanup the methods toFile(Analysis, DataSet) and toFile(Analysis). The methods now use the normal variable replacement feature of Roddy instead of the removed replace method - Make the field variableDetection static final - Make toEvaluatedValue final and extend its argument list Add a on-null-check to DefaultValidator.checkDollarSignUsage Analysis: - Clean up and groovyfy the class - Change the getConfiguration method so that it will return a configuration which knows values like USERNAME and USERGROUP DataSet now contains a getConfiguration method, which knows the different variants of DATASET. For internal use only! Add tests for variable replacement to ConfigurationValueSpec Add ExecutionService and FileSystemAccessProvider init code to some test classes

…com/theroddywms/Roddy into ChangeConfigurationValueReplacement

Change usage of strings like "string", "integer", etc. for configuration value types all over the code to use new constants in the ConfigurationConstants class. Changed minor things pointed at in the Pull request. ConfigurationValue: - Added a larger comment to toFile(). - Changed .equals() to ==. - Removed usage of checkAndCorrectPath and the method itself. - Moved getDefaultCValueTypeEnumeration from Enumeration to ConfigurationValue.

This class extends the Groovy Specification class and is used to do some basic initialization like the FileSystemProvier and the ExecutionService.

Updated the copyright notice in all files to our current version. Added the RoddyTestSpec class, which is meant to be a base class for Spock tests. The class provides some basic initialization settings.

The context object would have been available for every sub class. Removed it to avoid the possibility of paralellism issues. Just create your own context objects when you need them.

vinjana · 2018-11-28T09:45:15Z

RoddyCore/src/de/dkfz/roddy/config/ConfigurationIssue.groovy

                ConfigurationIssueLevel.CVALUE_WARNING,
                "Several variables in your configuration contain one or more dollar signs. As this might impose problems in your cluster jobs, check the entries in your job configuration files. See the extended logs for more information.",
-                "The variable named '#REPLACE_0#' contains one or more dollar signs, which do not belong to a Roddy variable definition (\${variable identifier}). This might impose problems, so make sure, that your results job configuration is created in the way you want."
+                "Variable '#REPLACE_0#' contains plain dollar sign(s) without braces. Roddy does not interpret them as variables and cannot guarantee correct ordering of assignments for such variables in the job parameter file."


Maybe a hint how to avoid the warning? Maybe simply add "and unescaped" before dollar? Also "plain" in vague and does not add much meaning. Maybe better "unescaped dollar sign(s) without braces" would be sufficient.

"Variable '#REPLACE_0#' contains unescaped dollar sign(s) without braces. Roddy does not interpret them as variables and cannot guarantee correct ordering of assignments for such variables in the job parameter file."

RoddyCore/src/de/dkfz/roddy/config/ConfigurationIssue.groovy

vinjana · 2018-11-28T09:50:22Z

RoddyCore/src/de/dkfz/roddy/config/ConfigurationValue.groovy

+    /**
+     * API Level 3.4+
+     */
+    final boolean invalid


I don't understand. Why do you keep the "invalid" field anyway?

RoddyCore/src/de/dkfz/roddy/config/ConfigurationValue.groovy

RoddyCore/src/de/dkfz/roddy/config/validation/DefaultValidator.groovy

RoddyCore/test/src/de/dkfz/roddy/config/validation/DefaultValidatorSpec.groovy

Added a comment to Analysis.getConfiguration(). The method will fail for many tests, when the tests do not also initialize the ExecutionService and FileSystemAccessProvider. The comment describes the method which needs to be added to the test class. Added the aforementioned method to ContextResourceTest Changed ContextResource so, that the contextResource object is now @shared and a @ClassRule but NOT static! Also the static initializer in the anonymous creation is removed.

Add roddy test spec class and change all copyright notices

Add the SystemProperties class to add one place for accessing system properties and to perform code deduplication. Replace access code to System.getProperty() to use the new class. Update messages for two ConfigurationIssue templates. Extend the xml configuration part in the docs to describe the order of evaluation for ConfigurationValue.toFile(). Remove the invalid field and the setInvalid method from ConfigurationValue. Remove try/catch from getUsername() and getUsergroup() in the Analysis class. Both method returned "UNKNOWN". Change name of getOutputAnalysisBaseCV() to getOutputAnalysisBaseDirectoryCV() in RuntimeService class. Add the test case for $abc to 'test check proper dollar sign usage in variables' in DefaultValidatorSpec.

The DefaultValidator now recognizes starting '$' characters in strings. => '$abc' will be marked as invalid. Fix tests.

dankwart-de · 2018-12-06T12:55:24Z

Please note, that I (unfortunately) did most of the fixes in #309. Please just check that and if you agree I will merge both.

In DefaultValidator: - Rename areDollarCharactersProperlyUsed to areDollarCharactersAttached. - Typo fixes. Change messages in ConfigurationIssue class. Added a test for '${}' to "test check for proper variable usage definition"() in DefaultValidatorSpec test class. Fixed tests in ConfigurationIssueSpec test class.

…acement Fix for Configuration value replacement

dankwart-de · 2018-12-07T08:06:44Z

Did all fixes in #309 and will merge now.

dankwart-de added 4 commits November 12, 2018 17:57

Add extension for print warnings/errors in Analysis

6a6b626

Fix tests

ace788d

dankwart-de requested a review from vinjana November 13, 2018 09:55

vinjana requested changes Nov 14, 2018

View reviewed changes

vinjana requested changes Nov 15, 2018

View reviewed changes

vinjana reviewed Nov 20, 2018

View reviewed changes

dankwart-de added 8 commits November 20, 2018 18:55

Merge branch 'ChangeConfigurationValueReplacement' of https://github.…

660b0f2

…com/theroddywms/Roddy into ChangeConfigurationValueReplacement

Add the RoddyTestSpec class

c9b7650

This class extends the Groovy Specification class and is used to do some basic initialization like the FileSystemProvier and the ExecutionService.

Add the RoddyTestSpec class, update Copyright

f136f9e

Updated the copyright notice in all files to our current version. Added the RoddyTestSpec class, which is meant to be a base class for Spock tests. The class provides some basic initialization settings.

Changed eilslabs references to TheRoddyWMS reference

bfcfbf9

Removed genericContext from RoddyTestSpec

1f453f8

The context object would have been available for every sub class. Removed it to avoid the possibility of paralellism issues. Just create your own context objects when you need them.

vinjana reviewed Nov 28, 2018

View reviewed changes

vinjana mentioned this pull request Nov 28, 2018

Implement an existing templating mechanism for ConfigurationIssueLevel and rework the messages. #310

Open

vinjana reviewed Nov 28, 2018

View reviewed changes

RoddyCore/test/src/de/dkfz/roddy/config/validation/DefaultValidatorSpec.groovy Outdated Show resolved Hide resolved

dankwart-de and others added 6 commits November 29, 2018 13:52

Merge branch 'master' into AddRoddyTestSpecClass

859c401

Changed some ExitReasons messages

c270668

Merge pull request #314 from TheRoddyWMS/AddRoddyTestSpecClass

7e0111b

Add roddy test spec class and change all copyright notices

Fix DefaultValidator, update tests

6ab250f

The DefaultValidator now recognizes starting '$' characters in strings. => '$abc' will be marked as invalid. Fix tests.

dankwart-de and others added 3 commits December 6, 2018 14:27

Rename exit method to exitWithMessage.

7a552f2

Merge pull request #309 from TheRoddyWMS/ChangeConfigurationValueRepl…

4d3ed25

…acement Fix for Configuration value replacement

dankwart-de merged commit e74744b into master Dec 7, 2018

dankwart-de deleted the AddWarningIfDollarIsUsedWithouthBraces branch December 7, 2018 08:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce the ConfigurationIssue system #307

Introduce the ConfigurationIssue system #307

dankwart-de commented Nov 13, 2018

vinjana left a comment

vinjana Nov 14, 2018

vinjana Nov 20, 2018

dankwart-de Nov 20, 2018

vinjana Nov 20, 2018

vinjana Nov 20, 2018

vinjana Nov 20, 2018

dankwart-de Nov 20, 2018

vinjana Nov 20, 2018

vinjana Nov 20, 2018

vinjana Nov 28, 2018

vinjana Nov 28, 2018

dankwart-de commented Dec 6, 2018

dankwart-de commented Dec 7, 2018

Introduce the ConfigurationIssue system #307

Introduce the ConfigurationIssue system #307

Conversation

dankwart-de commented Nov 13, 2018

vinjana left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dankwart-de commented Dec 6, 2018

dankwart-de commented Dec 7, 2018