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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
