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.

Sign up for GitHub

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

[CDAP-15407] When a wrong directive is typed in CDAP/Wrangler no error message is displayed to user. #403

Open
wants to merge 4 commits into
base: release/4.2
Choose a base branch
from
Open

[CDAP-15407] When a wrong directive is typed in CDAP/Wrangler no error message is displayed to user. #403

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c9f5655
[CDAP-15407] Fixed handling of error message when user types in a wro…
nitinmotgi May 2, 2020
6fcc43c
Resolve PR comments
nitinmotgi May 6, 2020
9903516
Fix the testing error
nitinmotgi May 6, 2020
a0c6082
Modified error message to have proper english
nitinmotgi May 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions wrangler-core/src/main/java/io/cdap/wrangler/registry/UserDirectiveRegistry.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,8 +129,8 @@ public DirectiveInfo get(String namespace, String name) throws DirectiveLoadExce
}
if (directive == null) {
throw new DirectiveLoadException(
String.format("10-5 - Unable to load the user defined directive '%s'. " +
"Please check if the artifact containing UDD is still present.", name)
String.format("Directive '%s' not found. Check if directive is spelled correctly or if directive is " +
nitinmotgi marked this conversation as resolved.
Show resolved Hide resolved
"user defined make sure artifact is uploaded", name)
);
}
DirectiveInfo directiveInfo = new DirectiveInfo(DirectiveInfo.Scope.USER, directive);
Expand Down
17 changes: 15 additions & 2 deletions wrangler-service/src/main/java/io/cdap/wrangler/service/directive/DirectivesHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
import io.cdap.wrangler.api.Row;
import io.cdap.wrangler.api.TokenGroup;
import io.cdap.wrangler.api.TransientStore;
import io.cdap.wrangler.api.parser.SyntaxError;
import io.cdap.wrangler.api.parser.Token;
import io.cdap.wrangler.api.parser.TokenType;
import io.cdap.wrangler.datamodel.DataModelGlossary;
Expand Down Expand Up @@ -431,7 +432,7 @@ public void upload(HttpServiceRequest request, HttpServiceResponder responder,
// Extract content.
byte[] content = handler.getContent();
if (content == null) {
throw new BadRequestException("Body not present, please post the file containing the "
throw new BadRequestException("Body is not present, upload file containing "
+ "records to be wrangled.");
}

Expand Down Expand Up @@ -629,7 +630,7 @@ public void execute(HttpServiceRequest request, HttpServiceResponder responder,
if ((object.getClass().getMethod("toString").getDeclaringClass() != Object.class)) {
value.put(fieldName, object.toString());
} else {
value.put(fieldName, "Non-displayable object");
value.put(fieldName, "Non displayable object");
}
} else {
value.put(fieldName, null);
Expand Down Expand Up @@ -663,6 +664,18 @@ private String addLoadablePragmaDirectives(String namespace, Request request) {
// Compile the directive extracting the loadable plugins (a.k.a
// Directives in this context).
CompileStatus status = compiler.compile(new MigrateToV2(request.getRecipe().getDirectives()).migrate());
if (!status.isSuccess()) {
StringBuilder eStr = new StringBuilder();
Iterator<SyntaxError> errors = status.getErrors();
while (errors.hasNext()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

seems like this could potentially result in a giant error message that covers the whole screen. Should we limit the number of errors in the 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.

@albertshau this is compile error. So, I don't expect it to be very long as compiler will terminate on first error, but can have two to three lines max.

Copy link
Contributor

Choose a reason for hiding this comment

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

If there are 10 syntax errors in the recipe, won't this result in 10 error messages concatenated together with commas? And if each individual message has a comma or period or multiple sentences, this can very easily become unreadable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, we terminate on first one.

Copy link
Contributor

Choose a reason for hiding this comment

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

then why bother iterating through all of these and concatenating instead of just getting the first one?

eStr.append(errors.next().getMessage());
if (errors.hasNext()) {
eStr.append(",");
}
}
throw new DirectiveParseException(eStr.toString());
}

RecipeSymbol symbols = status.getSymbols();
Iterator<TokenGroup> iterator = symbols.iterator();
List<String> userDirectives = new ArrayList<>();
Expand Down