You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2022/11/22 18:29:26 UTC
[GitHub] [nifi] tpalfy opened a new pull request, #6703: NIFI-10765 - Added better error reporting in JASN1Reader.
tpalfy opened a new pull request, #6703:
URL: https://github.com/apache/nifi/pull/6703
<!-- Licensed to the Apache Software Foundation (ASF) under one or more -->
<!-- contributor license agreements. See the NOTICE file distributed with -->
<!-- this work for additional information regarding copyright ownership. -->
<!-- The ASF licenses this file to You under the Apache License, Version 2.0 -->
<!-- (the "License"); you may not use this file except in compliance with -->
<!-- the License. You may obtain a copy of the License at -->
<!-- http://www.apache.org/licenses/LICENSE-2.0 -->
<!-- Unless required by applicable law or agreed to in writing, software -->
<!-- distributed under the License is distributed on an "AS IS" BASIS, -->
<!-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -->
<!-- See the License for the specific language governing permissions and -->
<!-- limitations under the License. -->
# Summary
[NIFI-10765](https://issues.apache.org/jira/browse/NIFI-10765)
# Tracking
Please complete the following tracking steps prior to pull request creation.
### Issue Tracking
- [ ] [Apache NiFi Jira](https://issues.apache.org/jira/browse/NIFI) issue created
### Pull Request Tracking
- [ ] Pull Request title starts with Apache NiFi Jira issue number, such as `NIFI-00000`
- [ ] Pull Request commit message starts with Apache NiFi Jira issue number, as such `NIFI-00000`
### Pull Request Formatting
- [ ] Pull Request based on current revision of the `main` branch
- [ ] Pull Request refers to a feature branch with one commit containing changes
# Verification
Please indicate the verification steps performed prior to pull request creation.
### Build
- [ ] Build completed using `mvn clean install -P contrib-check`
- [ ] JDK 8
- [ ] JDK 11
- [ ] JDK 17
### Licensing
- [ ] New dependencies are compatible with the [Apache License 2.0](https://apache.org/licenses/LICENSE-2.0) according to the [License Policy](https://www.apache.org/legal/resolved.html)
- [ ] New dependencies are documented in applicable `LICENSE` and `NOTICE` files
### Documentation
- [ ] Documentation formatting appears as expected in rendered files
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a diff in pull request #6703: NIFI-10765 - Added better error reporting in JASN1Reader.
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on code in PR #6703:
URL: https://github.com/apache/nifi/pull/6703#discussion_r1029705455
##########
nifi-nar-bundles/nifi-asn1-bundle/nifi-asn1-services/pom.xml:
##########
@@ -62,6 +62,12 @@
<artifactId>caffeine</artifactId>
<version>2.8.1</version>
</dependency>
+ <dependency>
+ <groupId>antlr</groupId>
+ <artifactId>antlr</artifactId>
+ <version>2.7.7</version>
Review Comment:
On further review, I noted that that jasn1-compiler uses this same version, so it makes sense to keep this version.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a diff in pull request #6703: NIFI-10765 - Added better error reporting in JASN1Reader.
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on code in PR #6703:
URL: https://github.com/apache/nifi/pull/6703#discussion_r1029809865
##########
nifi-nar-bundles/nifi-asn1-bundle/nifi-asn1-services/src/main/java/org/apache/nifi/jasn1/JASN1Reader.java:
##########
@@ -219,8 +236,55 @@ private void compileAsnToClass(String... asnFilePaths) {
asnCompilerArguments.add("-o");
asnCompilerArguments.add(asnOutDir.toString());
+ HashMap<String, AsnModule> modulesByName = new HashMap<>();
+
+ Exception parseException = null;
+ for (String asn1File : asnFilePaths) {
+ logger.info("Parsing " + asn1File);
+ try {
+ AsnModel model = getJavaModelFromAsn1File(asn1File);
+ modulesByName.putAll(model.modulesByName);
+ } catch (FileNotFoundException e) {
+ logger.error("Couldn't find " + asn1File, e);
Review Comment:
Log messages should use placeholders instead of string concatenation. Also recommend avoiding contractions and spelling out the message instead.
```suggestion
logger.error("ASN.1 file not found [{}]", asn1File, e);
```
##########
nifi-nar-bundles/nifi-asn1-bundle/nifi-asn1-services/src/main/java/org/apache/nifi/jasn1/JASN1Reader.java:
##########
@@ -219,8 +236,55 @@ private void compileAsnToClass(String... asnFilePaths) {
asnCompilerArguments.add("-o");
asnCompilerArguments.add(asnOutDir.toString());
+ HashMap<String, AsnModule> modulesByName = new HashMap<>();
+
+ Exception parseException = null;
+ for (String asn1File : asnFilePaths) {
+ logger.info("Parsing " + asn1File);
+ try {
+ AsnModel model = getJavaModelFromAsn1File(asn1File);
+ modulesByName.putAll(model.modulesByName);
+ } catch (FileNotFoundException e) {
+ logger.error("Couldn't find " + asn1File, e);
+ parseException = e;
+ } catch (TokenStreamException | RecognitionException e) {
+ logger.error("Error while parsing " + asn1File, e);
+ parseException = e;
+ } catch (Exception e) {
+ logger.error("Couldn't parse " + asn1File, e);
Review Comment:
```suggestion
logger.error("ASN.1 parsing failed [{}]", asn1File, e);
```
##########
nifi-nar-bundles/nifi-asn1-bundle/nifi-asn1-services/src/main/java/org/apache/nifi/jasn1/JASN1Reader.java:
##########
@@ -219,8 +236,55 @@ private void compileAsnToClass(String... asnFilePaths) {
asnCompilerArguments.add("-o");
asnCompilerArguments.add(asnOutDir.toString());
+ HashMap<String, AsnModule> modulesByName = new HashMap<>();
+
+ Exception parseException = null;
+ for (String asn1File : asnFilePaths) {
+ logger.info("Parsing " + asn1File);
+ try {
+ AsnModel model = getJavaModelFromAsn1File(asn1File);
+ modulesByName.putAll(model.modulesByName);
+ } catch (FileNotFoundException e) {
+ logger.error("Couldn't find " + asn1File, e);
+ parseException = e;
+ } catch (TokenStreamException | RecognitionException e) {
+ logger.error("Error while parsing " + asn1File, e);
+ parseException = e;
+ } catch (Exception e) {
+ logger.error("Couldn't parse " + asn1File, e);
+ parseException = e;
+ }
+ }
+
+ if (parseException != null) {
+ throw new ProcessException("Couldn't parse asn files.", parseException);
+ }
+
try {
- com.beanit.asn1bean.compiler.Compiler.main(asnCompilerArguments.toArray(new String[0]));
+ Constructor<BerClassWriter> berClassWriterConstructor = BerClassWriter.class.getDeclaredConstructor(
+ HashMap.class,
+ String.class,
+ String.class,
+ Boolean.TYPE,
+ Boolean.TYPE,
+ Boolean.TYPE
+ );
+
+ berClassWriterConstructor.setAccessible(true);
+
+ logger.info("Writing asn classes to " + asnOutDir.toString());
Review Comment:
```suggestion
logger.info("Writing ASN.1 classes to directory [{}]", asnOutDir);
```
##########
nifi-nar-bundles/nifi-asn1-bundle/nifi-asn1-services/src/main/java/org/apache/nifi/jasn1/JASN1Reader.java:
##########
@@ -219,8 +236,55 @@ private void compileAsnToClass(String... asnFilePaths) {
asnCompilerArguments.add("-o");
asnCompilerArguments.add(asnOutDir.toString());
+ HashMap<String, AsnModule> modulesByName = new HashMap<>();
+
+ Exception parseException = null;
+ for (String asn1File : asnFilePaths) {
+ logger.info("Parsing " + asn1File);
+ try {
+ AsnModel model = getJavaModelFromAsn1File(asn1File);
+ modulesByName.putAll(model.modulesByName);
+ } catch (FileNotFoundException e) {
+ logger.error("Couldn't find " + asn1File, e);
+ parseException = e;
+ } catch (TokenStreamException | RecognitionException e) {
+ logger.error("Error while parsing " + asn1File, e);
+ parseException = e;
+ } catch (Exception e) {
+ logger.error("Couldn't parse " + asn1File, e);
+ parseException = e;
+ }
+ }
+
+ if (parseException != null) {
+ throw new ProcessException("Couldn't parse asn files.", parseException);
Review Comment:
```suggestion
throw new ProcessException("ASN.1 parsing failed", parseException);
```
##########
nifi-nar-bundles/nifi-asn1-bundle/nifi-asn1-services/src/test/java/org/apache/nifi/jasn1/JASN1ReaderTest.java:
##########
@@ -78,4 +89,82 @@ public void testCanLoadClassCompiledFromAsn() throws Exception {
assertEquals("org.apache.nifi.jasn1.test.RootType", actualRootModelName);
assertNotNull(actual);
}
+
+ @Test
+ public void testAsnFileDoesntExist() throws Exception {
+ // GIVEN
+ ConfigurationContext context = mock(ConfigurationContext.class, RETURNS_DEEP_STUBS);
+ when(context.getProperty(ASN_FILES).isSet()).thenReturn(true);
+ when(context.getProperty(ASN_FILES).evaluateAttributeExpressions().getValue()).thenReturn(
+ "src/test/resources/test.asn,src/test/resources/doesnt_exist.asn"
+ );
+
+ // WHEN
+ ProcessException processException = assertThrows(
+ ProcessException.class,
+ () -> testSubject.onEnabled(context)
+ );
+ Throwable cause = processException.getCause();
+
+ assertEquals(FileNotFoundException.class, cause.getClass());
+ assertThat(cause.getMessage(), containsString("src/test/resources/doesnt_exist.asn"));
+ }
+
+ @Test
+ public void testCantParseAsn() throws Exception {
+ // GIVEN
+ String asnFiles = "src/test/resources/cant_parse.asn";
+
+ List<String> expectedErrorMessages = Arrays.asList(
+ "line 11:5: unexpected token: field3",
+ "line 17:33: unexpected token: ["
+ );
+
+ // WHEN
+ // THEN
+ testError(asnFiles, expectedErrorMessages);
+ }
+
+ @Test
+ public void testCantCompileAsn() throws Exception {
+ // GIVEN
+ String asnFiles = "src/test/resources/cant_compile.asn";
+
+ List<String> expectedErrorMessages = Arrays.asList(
+ "class SAMENAMEWithDifferentCase is public, should be declared in a file named SAMENAMEWithDifferentCase.java",
+ "cannot find symbol\n" +
+ " symbol: class SameNameWithDifferentCase\n" +
+ " location: class org.apache.nifi.jasn1.test.SAMENAMEWithDifferentCase",
+ "incompatible types: com.beanit.asn1bean.ber.types.BerInteger cannot be converted to com.beanit.asn1bean.ber.BerLength",
+ "incompatible types: boolean cannot be converted to java.io.OutputStream",
+ "Some messages have been simplified; recompile with -Xdiags:verbose to get full output"
+ );
Review Comment:
Expecting precise error messages makes tests more brittle, and appears to introduce problems with different language locales. Recommend asserting particular keywords, such as a class name, instead of full messages.
##########
nifi-nar-bundles/nifi-asn1-bundle/nifi-asn1-services/src/main/java/org/apache/nifi/jasn1/JASN1Reader.java:
##########
@@ -219,8 +236,55 @@ private void compileAsnToClass(String... asnFilePaths) {
asnCompilerArguments.add("-o");
asnCompilerArguments.add(asnOutDir.toString());
+ HashMap<String, AsnModule> modulesByName = new HashMap<>();
+
+ Exception parseException = null;
+ for (String asn1File : asnFilePaths) {
+ logger.info("Parsing " + asn1File);
+ try {
+ AsnModel model = getJavaModelFromAsn1File(asn1File);
+ modulesByName.putAll(model.modulesByName);
+ } catch (FileNotFoundException e) {
+ logger.error("Couldn't find " + asn1File, e);
+ parseException = e;
+ } catch (TokenStreamException | RecognitionException e) {
+ logger.error("Error while parsing " + asn1File, e);
Review Comment:
```suggestion
logger.error("ASN.1 stream parsing failed [{}]", asn1File, e);
```
##########
nifi-nar-bundles/nifi-asn1-bundle/nifi-asn1-services/src/main/java/org/apache/nifi/jasn1/JASN1Reader.java:
##########
@@ -219,8 +236,55 @@ private void compileAsnToClass(String... asnFilePaths) {
asnCompilerArguments.add("-o");
asnCompilerArguments.add(asnOutDir.toString());
+ HashMap<String, AsnModule> modulesByName = new HashMap<>();
+
+ Exception parseException = null;
+ for (String asn1File : asnFilePaths) {
+ logger.info("Parsing " + asn1File);
+ try {
+ AsnModel model = getJavaModelFromAsn1File(asn1File);
+ modulesByName.putAll(model.modulesByName);
+ } catch (FileNotFoundException e) {
+ logger.error("Couldn't find " + asn1File, e);
+ parseException = e;
+ } catch (TokenStreamException | RecognitionException e) {
+ logger.error("Error while parsing " + asn1File, e);
+ parseException = e;
+ } catch (Exception e) {
+ logger.error("Couldn't parse " + asn1File, e);
+ parseException = e;
+ }
+ }
+
+ if (parseException != null) {
+ throw new ProcessException("Couldn't parse asn files.", parseException);
+ }
+
try {
- com.beanit.asn1bean.compiler.Compiler.main(asnCompilerArguments.toArray(new String[0]));
+ Constructor<BerClassWriter> berClassWriterConstructor = BerClassWriter.class.getDeclaredConstructor(
+ HashMap.class,
+ String.class,
+ String.class,
+ Boolean.TYPE,
+ Boolean.TYPE,
+ Boolean.TYPE
+ );
+
+ berClassWriterConstructor.setAccessible(true);
+
+ logger.info("Writing asn classes to " + asnOutDir.toString());
+ BerClassWriter classWriter = berClassWriterConstructor.newInstance(
+ modulesByName,
+ asnOutDir.toString(),
+ "",
+ true,
+ false,
+ false
+ );
+
+ classWriter.translate();
+ } catch (NoSuchMethodException | InstantiationException | IllegalAccessException | InvocationTargetException e) {
+ throw new ProcessException("Couldn't create asn compiler.", e);
Review Comment:
```suggestion
throw new ProcessException("ASN.1 Compiler creation failed, e);
```
##########
nifi-nar-bundles/nifi-asn1-bundle/nifi-asn1-services/src/main/java/org/apache/nifi/jasn1/JASN1Reader.java:
##########
@@ -219,8 +236,55 @@ private void compileAsnToClass(String... asnFilePaths) {
asnCompilerArguments.add("-o");
asnCompilerArguments.add(asnOutDir.toString());
+ HashMap<String, AsnModule> modulesByName = new HashMap<>();
+
+ Exception parseException = null;
+ for (String asn1File : asnFilePaths) {
+ logger.info("Parsing " + asn1File);
+ try {
+ AsnModel model = getJavaModelFromAsn1File(asn1File);
+ modulesByName.putAll(model.modulesByName);
+ } catch (FileNotFoundException e) {
+ logger.error("Couldn't find " + asn1File, e);
+ parseException = e;
+ } catch (TokenStreamException | RecognitionException e) {
+ logger.error("Error while parsing " + asn1File, e);
+ parseException = e;
+ } catch (Exception e) {
+ logger.error("Couldn't parse " + asn1File, e);
+ parseException = e;
+ }
+ }
+
+ if (parseException != null) {
+ throw new ProcessException("Couldn't parse asn files.", parseException);
+ }
+
try {
- com.beanit.asn1bean.compiler.Compiler.main(asnCompilerArguments.toArray(new String[0]));
+ Constructor<BerClassWriter> berClassWriterConstructor = BerClassWriter.class.getDeclaredConstructor(
+ HashMap.class,
+ String.class,
+ String.class,
+ Boolean.TYPE,
+ Boolean.TYPE,
+ Boolean.TYPE
+ );
+
+ berClassWriterConstructor.setAccessible(true);
Review Comment:
This approach may create problems on Java 17, is it necessary to change the accessible of this constructor?
##########
nifi-nar-bundles/nifi-asn1-bundle/nifi-asn1-services/src/main/java/org/apache/nifi/jasn1/JASN1Reader.java:
##########
@@ -246,10 +310,17 @@ private void compileAsnToClass(String... asnFilePaths) {
Iterable<? extends JavaFileObject> units;
units = fileManager.getJavaFileObjectsFromFiles(javaFiles);
- JavaCompiler.CompilationTask task = javaCompiler.getTask(null, fileManager, null, optionList, null, units);
+ DiagnosticCollector<JavaFileObject> diagnosticListener = new DiagnosticCollector<>();
+ JavaCompiler.CompilationTask task = javaCompiler.getTask(null, fileManager, diagnosticListener, optionList, null, units);
+
Boolean success = task.call();
if (!success) {
- throw new ProcessException("Couldn't compile java file.");
+ Set<String> errorMessages = new LinkedHashSet();
+ diagnosticListener.getDiagnostics().stream().map(d -> d.getMessage(Locale.getDefault())).forEach(errorMessages::add);
+
+ errorMessages.forEach(logger::error);
+
+ throw new ProcessException("Couldn't compile java files.");
Review Comment:
```suggestion
throw new ProcessException("Java compilation failed");
```
##########
nifi-nar-bundles/nifi-asn1-bundle/nifi-asn1-services/src/main/java/org/apache/nifi/jasn1/JASN1Reader.java:
##########
@@ -266,7 +337,7 @@ void deleteAsnOutDir() {
.map(Path::toFile)
.forEach(File::delete);
} catch (IOException e) {
- throw new ProcessException("Couldn't delete '" + asnOutDir + "'");
+ throw new ProcessException("Couldn't delete " + asnOutDir);
Review Comment:
```suggestion
throw new ProcessException("Delete directory failed " + asnOutDir);
```
##########
nifi-nar-bundles/nifi-asn1-bundle/nifi-asn1-services/src/main/java/org/apache/nifi/jasn1/JASN1Reader.java:
##########
@@ -294,6 +365,37 @@ public RecordReader createRecordReader(
return new JASN1RecordReader(rootClassName, recordField, schemaProvider, customClassLoader, iteratorProviderClassName, in, logger);
}
+ AsnModel getJavaModelFromAsn1File(String inputFileName)
+ throws FileNotFoundException, TokenStreamException, RecognitionException {
+
+ InputStream stream = new FileInputStream(inputFileName);
+ ASNLexer lexer = new ASNLexer(stream);
+
+ AtomicBoolean parseError = new AtomicBoolean(false);
+ ASNParser parser = new ASNParser(lexer) {
+ @Override
+ public void reportError(String s) {
+ logger.error(s);
+ parseError.set(true);
+ }
+
+ @Override
+ public void reportError(RecognitionException e) {
+ logger.error(e.toString());
+ parseError.set(true);
+ }
+ };
+
+ if (parseError.get()) {
+ throw new ProcessException("Error while parsing asn files.");
Review Comment:
```suggestion
throw new ProcessException("ASN.1 parsing failed");
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] tpalfy commented on a diff in pull request #6703: NIFI-10765 - Added better error reporting in JASN1Reader.
Posted by GitBox <gi...@apache.org>.
tpalfy commented on code in PR #6703:
URL: https://github.com/apache/nifi/pull/6703#discussion_r1031433551
##########
nifi-nar-bundles/nifi-asn1-bundle/nifi-asn1-services/src/main/java/org/apache/nifi/jasn1/JASN1Reader.java:
##########
@@ -219,8 +236,55 @@ private void compileAsnToClass(String... asnFilePaths) {
asnCompilerArguments.add("-o");
asnCompilerArguments.add(asnOutDir.toString());
+ HashMap<String, AsnModule> modulesByName = new HashMap<>();
+
+ Exception parseException = null;
+ for (String asn1File : asnFilePaths) {
+ logger.info("Parsing " + asn1File);
+ try {
+ AsnModel model = getJavaModelFromAsn1File(asn1File);
+ modulesByName.putAll(model.modulesByName);
+ } catch (FileNotFoundException e) {
+ logger.error("Couldn't find " + asn1File, e);
+ parseException = e;
+ } catch (TokenStreamException | RecognitionException e) {
+ logger.error("Error while parsing " + asn1File, e);
+ parseException = e;
+ } catch (Exception e) {
+ logger.error("Couldn't parse " + asn1File, e);
+ parseException = e;
+ }
+ }
+
+ if (parseException != null) {
+ throw new ProcessException("Couldn't parse asn files.", parseException);
+ }
+
try {
- com.beanit.asn1bean.compiler.Compiler.main(asnCompilerArguments.toArray(new String[0]));
+ Constructor<BerClassWriter> berClassWriterConstructor = BerClassWriter.class.getDeclaredConstructor(
+ HashMap.class,
+ String.class,
+ String.class,
+ Boolean.TYPE,
+ Boolean.TYPE,
+ Boolean.TYPE
+ );
+
+ berClassWriterConstructor.setAccessible(true);
Review Comment:
The constructor is package private.
The only alternative I see is to create a "fake" `com.beanit.asn1bean.compiler` package within the NiFi module and put a factory there. Would that be better? (Although if we want to prepare for Java 16+ I guess there's no other choice.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] tpalfy commented on a diff in pull request #6703: NIFI-10765 - Added better error reporting in JASN1Reader.
Posted by GitBox <gi...@apache.org>.
tpalfy commented on code in PR #6703:
URL: https://github.com/apache/nifi/pull/6703#discussion_r1031470829
##########
nifi-nar-bundles/nifi-asn1-bundle/nifi-asn1-services/src/test/java/org/apache/nifi/jasn1/JASN1ReaderTest.java:
##########
@@ -78,4 +89,82 @@ public void testCanLoadClassCompiledFromAsn() throws Exception {
assertEquals("org.apache.nifi.jasn1.test.RootType", actualRootModelName);
assertNotNull(actual);
}
+
+ @Test
+ public void testAsnFileDoesntExist() throws Exception {
+ // GIVEN
+ ConfigurationContext context = mock(ConfigurationContext.class, RETURNS_DEEP_STUBS);
+ when(context.getProperty(ASN_FILES).isSet()).thenReturn(true);
+ when(context.getProperty(ASN_FILES).evaluateAttributeExpressions().getValue()).thenReturn(
+ "src/test/resources/test.asn,src/test/resources/doesnt_exist.asn"
+ );
+
+ // WHEN
+ ProcessException processException = assertThrows(
+ ProcessException.class,
+ () -> testSubject.onEnabled(context)
+ );
+ Throwable cause = processException.getCause();
+
+ assertEquals(FileNotFoundException.class, cause.getClass());
+ assertThat(cause.getMessage(), containsString("src/test/resources/doesnt_exist.asn"));
+ }
+
+ @Test
+ public void testCantParseAsn() throws Exception {
+ // GIVEN
+ String asnFiles = "src/test/resources/cant_parse.asn";
+
+ List<String> expectedErrorMessages = Arrays.asList(
+ "line 11:5: unexpected token: field3",
+ "line 17:33: unexpected token: ["
+ );
+
+ // WHEN
+ // THEN
+ testError(asnFiles, expectedErrorMessages);
+ }
+
+ @Test
+ public void testCantCompileAsn() throws Exception {
+ // GIVEN
+ String asnFiles = "src/test/resources/cant_compile.asn";
+
+ List<String> expectedErrorMessages = Arrays.asList(
+ "class SAMENAMEWithDifferentCase is public, should be declared in a file named SAMENAMEWithDifferentCase.java",
+ "cannot find symbol\n" +
+ " symbol: class SameNameWithDifferentCase\n" +
+ " location: class org.apache.nifi.jasn1.test.SAMENAMEWithDifferentCase",
+ "incompatible types: com.beanit.asn1bean.ber.types.BerInteger cannot be converted to com.beanit.asn1bean.ber.BerLength",
+ "incompatible types: boolean cannot be converted to java.io.OutputStream",
+ "Some messages have been simplified; recompile with -Xdiags:verbose to get full output"
+ );
Review Comment:
The error messages by the underlying libraries are very technical and don't convey the root cause very well if at all. So there are no good keywords the presence/lack of which would correspond to a given error.
For the same reason I documented the errors exactly as they appear in the additionalDetails.html and provided an explanation for them.
I expect the list of known issues to grow but not the existing ones to change as the underlying apis don't change often. And when they do, we need to make sure we understand the change.
However I'll update the tests to mention the additionalDetails. This way any change will be captured by the tests and hopefully the user won't get stuck with outdated info either.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory closed pull request #6703: NIFI-10765 - Added better error reporting in JASN1Reader.
Posted by GitBox <gi...@apache.org>.
exceptionfactory closed pull request #6703: NIFI-10765 - Added better error reporting in JASN1Reader.
URL: https://github.com/apache/nifi/pull/6703
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] tpalfy commented on a diff in pull request #6703: NIFI-10765 - Added better error reporting in JASN1Reader.
Posted by GitBox <gi...@apache.org>.
tpalfy commented on code in PR #6703:
URL: https://github.com/apache/nifi/pull/6703#discussion_r1031470829
##########
nifi-nar-bundles/nifi-asn1-bundle/nifi-asn1-services/src/test/java/org/apache/nifi/jasn1/JASN1ReaderTest.java:
##########
@@ -78,4 +89,82 @@ public void testCanLoadClassCompiledFromAsn() throws Exception {
assertEquals("org.apache.nifi.jasn1.test.RootType", actualRootModelName);
assertNotNull(actual);
}
+
+ @Test
+ public void testAsnFileDoesntExist() throws Exception {
+ // GIVEN
+ ConfigurationContext context = mock(ConfigurationContext.class, RETURNS_DEEP_STUBS);
+ when(context.getProperty(ASN_FILES).isSet()).thenReturn(true);
+ when(context.getProperty(ASN_FILES).evaluateAttributeExpressions().getValue()).thenReturn(
+ "src/test/resources/test.asn,src/test/resources/doesnt_exist.asn"
+ );
+
+ // WHEN
+ ProcessException processException = assertThrows(
+ ProcessException.class,
+ () -> testSubject.onEnabled(context)
+ );
+ Throwable cause = processException.getCause();
+
+ assertEquals(FileNotFoundException.class, cause.getClass());
+ assertThat(cause.getMessage(), containsString("src/test/resources/doesnt_exist.asn"));
+ }
+
+ @Test
+ public void testCantParseAsn() throws Exception {
+ // GIVEN
+ String asnFiles = "src/test/resources/cant_parse.asn";
+
+ List<String> expectedErrorMessages = Arrays.asList(
+ "line 11:5: unexpected token: field3",
+ "line 17:33: unexpected token: ["
+ );
+
+ // WHEN
+ // THEN
+ testError(asnFiles, expectedErrorMessages);
+ }
+
+ @Test
+ public void testCantCompileAsn() throws Exception {
+ // GIVEN
+ String asnFiles = "src/test/resources/cant_compile.asn";
+
+ List<String> expectedErrorMessages = Arrays.asList(
+ "class SAMENAMEWithDifferentCase is public, should be declared in a file named SAMENAMEWithDifferentCase.java",
+ "cannot find symbol\n" +
+ " symbol: class SameNameWithDifferentCase\n" +
+ " location: class org.apache.nifi.jasn1.test.SAMENAMEWithDifferentCase",
+ "incompatible types: com.beanit.asn1bean.ber.types.BerInteger cannot be converted to com.beanit.asn1bean.ber.BerLength",
+ "incompatible types: boolean cannot be converted to java.io.OutputStream",
+ "Some messages have been simplified; recompile with -Xdiags:verbose to get full output"
+ );
Review Comment:
I used this approach in `testAsnFileDoesntExist` for example.
However in these cases the error messages by the underlying libraries are very technical and don't convey the root cause very well if at all. So there are no good keywords the presence/lack of which would correspond to a given error.
For the same reason I documented the errors exactly as they appear in the additionalDetails.html and provided an explanation for them.
I expect the list of known issues to grow but not the existing ones to change as the underlying apis don't change often. And when they do, we need to make sure we understand the change.
However I'll update the tests to mention the additionalDetails. This way any change will be captured by the tests and hopefully the user won't get stuck with outdated info either.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a diff in pull request #6703: NIFI-10765 - Added better error reporting in JASN1Reader.
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on code in PR #6703:
URL: https://github.com/apache/nifi/pull/6703#discussion_r1029704080
##########
nifi-nar-bundles/nifi-asn1-bundle/nifi-asn1-services/pom.xml:
##########
@@ -62,6 +62,12 @@
<artifactId>caffeine</artifactId>
<version>2.8.1</version>
</dependency>
+ <dependency>
+ <groupId>antlr</groupId>
+ <artifactId>antlr</artifactId>
+ <version>2.7.7</version>
Review Comment:
This version of ANTLR is dated from 2007, is it possible to use a newer version?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a diff in pull request #6703: NIFI-10765 - Added better error reporting in JASN1Reader.
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on code in PR #6703:
URL: https://github.com/apache/nifi/pull/6703#discussion_r1035256497
##########
nifi-nar-bundles/nifi-asn1-bundle/nifi-asn1-services/src/main/java/org/apache/nifi/jasn1/JASN1Reader.java:
##########
@@ -219,8 +236,55 @@ private void compileAsnToClass(String... asnFilePaths) {
asnCompilerArguments.add("-o");
asnCompilerArguments.add(asnOutDir.toString());
+ HashMap<String, AsnModule> modulesByName = new HashMap<>();
+
+ Exception parseException = null;
+ for (String asn1File : asnFilePaths) {
+ logger.info("Parsing " + asn1File);
+ try {
+ AsnModel model = getJavaModelFromAsn1File(asn1File);
+ modulesByName.putAll(model.modulesByName);
+ } catch (FileNotFoundException e) {
+ logger.error("Couldn't find " + asn1File, e);
+ parseException = e;
+ } catch (TokenStreamException | RecognitionException e) {
+ logger.error("Error while parsing " + asn1File, e);
+ parseException = e;
+ } catch (Exception e) {
+ logger.error("Couldn't parse " + asn1File, e);
+ parseException = e;
+ }
+ }
+
+ if (parseException != null) {
+ throw new ProcessException("Couldn't parse asn files.", parseException);
+ }
+
try {
- com.beanit.asn1bean.compiler.Compiler.main(asnCompilerArguments.toArray(new String[0]));
+ Constructor<BerClassWriter> berClassWriterConstructor = BerClassWriter.class.getDeclaredConstructor(
+ HashMap.class,
+ String.class,
+ String.class,
+ Boolean.TYPE,
+ Boolean.TYPE,
+ Boolean.TYPE
+ );
+
+ berClassWriterConstructor.setAccessible(true);
Review Comment:
Looks like a good solution, thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org