Implemented data validation on the backend #2
Conversation
| @@ -0,0 +1,68 @@ | |||
| /* | |||
| * Copyright © 2017-2019 Cask Data, Inc. | |||
| @@ -37,7 +38,10 @@ | |||
| // Values held by the row. | |||
| private List<Object> values = new ArrayList<>(); | |||
|
|
|||
| private final UUID id; | |||
There was a problem hiding this comment.
it doesn't look like the id is used anywhere. It would be better to add it later if required instead of right now.
| protected final List<Object> causes; | ||
|
|
||
| public ErrorRecordBase( | ||
| String message, int code, boolean showInWrangler, List<Object> causes) { |
There was a problem hiding this comment.
the CDAP related projects (including wrangler) use a checkstyle that only requires lines to be within 120 characters and not 80, so these could all be on the previous line.
| .stream() | ||
| .filter(ErrorRecord::isShownInWrangler) | ||
| // Use slim error record to reduce message size. | ||
| .map(SlimErrorRecord::new) |
There was a problem hiding this comment.
is the eventual response eventually serialized as a SlimErrorRecord? I would think it gets serialized as an ErrorRecordBase since that's what's returned by ErrorRecordsException.getErrorRecords()?
There was a problem hiding this comment.
It gets serialized as SlimErrorRecord, since that's the actual type.
| // Code associated with the message. | ||
| protected final int code; | ||
| protected final boolean showInWrangler; | ||
| protected final List<Object> causes; |
There was a problem hiding this comment.
I think it would be better for causes to have a defined structure rather than allow any object. Object, for example, would allow somebody to use a mix of primitives, objects, collections, etc.
This is probably a larger discussion, and the causes don't seem to be used yet so it may be better to remove this and add it back later when there's a more concrete idea of how it will be used.
There was a problem hiding this comment.
This depends on the frontend actually. Originally I wanted to put Exceptions/Throwables here, but it's probably not useful to hold a stacktrace and all that. Perhaps we should make a common base class that contains some data pointer and a message and then ValidationIssue is an extension of that with a schema pointer?
There was a problem hiding this comment.
It will probably end up something like that, with an actual Cause class that either allows users to extend it with arbitrary data, or contains some map of properties that users populate, or something like that. Since there are still open questions about what exactly is needed here and how it will be used, this should be added in the future. Anything we add in wrangler-api cannot be removed/changed without deprecating it for the entirety of a major release.
| @@ -0,0 +1,76 @@ | |||
| /* | |||
| * Copyright © 2019 Google Inc. | |||
There was a problem hiding this comment.
copyright here is different from other files
| private final List<ErrorRecordBase> errorRecords; | ||
|
|
||
| public ErrorRecordsException(List<ErrorRecordBase> errorRecords) { | ||
| super(String.format("%d error records were produced", errorRecords.size())); |
There was a problem hiding this comment.
With the assumption that nothing in the UI is changing, it makes sense to also include the message of the first error in the exception message (or the first X errors, or the first N characters?), otherwise there will be no way to know what to correct. I would just avoid showing them all, as it could easily become a huge message that covers the entire screen.
I think if we were to change the UI, we would also want to do the full solution in the backend where all errors are collected and returned, rather than this intermediate solution.
…re/improve-directive-error Remove usage of toString() from directive error messages
…x/CDAP-15541-null-error-message [CDAP-15541] fix error message when there is schema mismatch
…end-to-error Fix error message assert in unit tests
rpolyano
force-pushed
the
feature/data-models-2
branch
2 times, most recently
from
6dbacf6 to
073fad2
Compare
rpolyano
force-pushed
the
feature/data-models-2
branch
from
073fad2 to
bc4464f
Compare
This approach creates an error collector analog at wrangle-time, which can also be reused by other directives that want to draw user attention to error rows.