Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce the ConfigurationIssue system #307

Merged
merged 22 commits into from
Dec 7, 2018

Conversation

dankwart-de
Copy link
Contributor

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.

dankwart-de added 4 commits November 12, 2018 17:57
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.
@dankwart-de dankwart-de requested a review from vinjana November 13, 2018 09:55
Copy link
Contributor

@vinjana vinjana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First, nice summary of the changes. Thanks!

Not fully done yet, but here are some comments.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"] | []

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another -- aberrant -- input to check:

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

This case is actually no so unlikely to happen.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a lot here. Check it out.

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
value | type | expected | expectedWarnings | expectedErrors
'abc${var}bcd${var}' | 'string' | true | [] | []
'abc${var}bcd${var}def' | 'string' | true | [] | []
'abc$' | 'string' | false | [unattachedDollarCharacter.expand('bla')] | []
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another -- aberrant -- input to check:

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

This case is actually no so unlikely to happen.

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

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.",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also: Too wordy message.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New issue

/**
* API Level 3.4+
*/
final boolean invalid
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

o.k. clarified -- Groovy idiom.

dankwart-de added 8 commits November 20, 2018 18:55
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
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.
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."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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."

/**
* API Level 3.4+
*/
final boolean invalid
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

dankwart-de and others added 6 commits November 29, 2018 13:52
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
Copy link
Contributor Author

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

dankwart-de and others added 3 commits December 6, 2018 14:27
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
Copy link
Contributor Author

Did all fixes in #309 and will merge now.

@dankwart-de dankwart-de merged commit e74744b into master Dec 7, 2018
@dankwart-de dankwart-de deleted the AddWarningIfDollarIsUsedWithouthBraces branch December 7, 2018 08:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants