CDAP-15855: Updated validation API. #7
Conversation
|
@albertshau could you, please, take a look |
pom.xml
Outdated
| @@ -110,6 +110,12 @@ | |||
| <version>${cdap.version}</version> | |||
| <scope>provided</scope> | |||
| </dependency> | |||
| <dependency> | |||
| <groupId>io.cdap.cdap</groupId> | |||
| <artifactId>cdap-etl-core</artifactId> | |||
There was a problem hiding this comment.
This dependency was left for by mistake, it should be removed.
| source.getFileSystem(new Configuration()); | ||
| } catch (IOException e) { | ||
| collector.addFailure("Cannot determine the file system of the source file.", null) | ||
| .withConfigProperty(SOURCE_FILE_PATH).withStacktrace(e.getStackTrace()); | ||
| } | ||
| if (Strings.isNullOrEmpty(destFilePath)) { |
There was a problem hiding this comment.
this check can be moved up to be validated along with source path
| .withConfigProperty(FILE_REGEX); | ||
| } | ||
| try { | ||
| Charset.forName(charset); |
There was a problem hiding this comment.
this can be validated along with source path.
| if (Strings.isNullOrEmpty(sourceFilePath)) { | ||
| collector.addFailure("Source file or folder is required.", "Set source file or folder.") | ||
| .withConfigProperty(SOURCE_FILE_PATH); | ||
| collector.getOrThrowException(); |
There was a problem hiding this comment.
why do we need to throw exception here? This would just validate one property and throw an exception. we should validate other properties as well before throwing an exception
| try { | ||
| new ToUTF8Action(config).configurePipeline(configurer); | ||
| } catch (ValidationException e) { | ||
| assertEquals(1, e.getFailures().size()); |
There was a problem hiding this comment.
can we also add unit tests where more than 1 failures are collected?
There was a problem hiding this comment.
While reviewing this, I took a look at what this plugin is doing and I'm questioning the usefulness of this plugin. It is also implemented incorrectly so we shouldn't be providing it for people to use.
I'm going to see if we can remove the plugin, so not going to review the rest.
| "from https://docs.oracle.com/javase/8/docs/technotes/guides/intl/encoding.doc.html.", e); | ||
| private void validate(FailureCollector collector) throws ValidationException { | ||
| if (Strings.isNullOrEmpty(sourceFilePath)) { | ||
| collector.addFailure("Source file or folder is required.", "Set source file or folder.") |
There was a problem hiding this comment.
If the error message and the corrective action are basically the same (like here), it's better to just have the message and leave the corrective action null.
JIRA: https://issues.cask.co/browse/CDAP-15855
In scope of this PR: