nandorsoma commented on code in PR #6769: URL: https://github.com/apache/nifi/pull/6769#discussion_r1101891721
########## nifi-nar-bundles/nifi-asn1-bundle/nifi-asn1-services/src/main/java/org/apache/nifi/jasn1/preprocess/preprocessors/ConstraintAsnPreprocessor.java: ########## @@ -0,0 +1,91 @@ +/* + * 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. + */ +package org.apache.nifi.jasn1.preprocess.preprocessors; + +import org.apache.nifi.jasn1.preprocess.NiFiASNPreprocessor; + +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +public class ConstraintAsnPreprocessor implements NiFiASNPreprocessor { Review Comment: I recommend using the same naming convention for asn in class names. ########## nifi-nar-bundles/nifi-asn1-bundle/nifi-asn1-services/src/main/java/org/apache/nifi/jasn1/preprocess/preprocessors/ConstraintAsnPreprocessor.java: ########## @@ -0,0 +1,91 @@ +/* + * 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. + */ +package org.apache.nifi.jasn1.preprocess.preprocessors; + +import org.apache.nifi.jasn1.preprocess.NiFiASNPreprocessor; + +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +public class ConstraintAsnPreprocessor implements NiFiASNPreprocessor { + public static final String OPEN_BRACKET = "("; + public static final String CLOSE_BRACKET = ")"; + + public static final Pattern ALLOWED = Pattern.compile("^(\\d+\\))(.*)"); + + @Override + public List<String> preprocessAsn(List<String> lines) { + List<String> preprocessedLines = new ArrayList<>(); Review Comment: finals are missing in multiple places, can you add them? ########## nifi-nar-bundles/nifi-asn1-bundle/nifi-asn1-services/src/test/java/org/apache/nifi/jasn1/JASN1ReaderTest.java: ########## @@ -84,6 +84,7 @@ public void testCanLoadClassCompiledFromAsn() throws Exception { ConfigurationContext context = mock(ConfigurationContext.class, RETURNS_DEEP_STUBS); when(context.getProperty(ASN_FILES).isSet()).thenReturn(true); when(context.getProperty(ASN_FILES).evaluateAttributeExpressions().getValue()).thenReturn(Paths.get("src", "test", "resources", "test.asn").toString()); +// when(context.getProperty(ASN_FILES).evaluateAttributeExpressions().getValue()).thenReturn(Paths.get("src", "test", "resources", "test_error.asn").toString()); Review Comment: Is this line commented out intentionally? ########## nifi-nar-bundles/nifi-asn1-bundle/nifi-asn1-services/src/main/java/org/apache/nifi/jasn1/preprocess/NiFiASNPreprocessor.java: ########## @@ -0,0 +1,23 @@ +/* + * 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. + */ +package org.apache.nifi.jasn1.preprocess; + +import java.util.List; + +public interface NiFiASNPreprocessor { Review Comment: It is usually recommended to avoid prefixing classes with NiFi. ########## nifi-nar-bundles/nifi-asn1-bundle/nifi-asn1-services/src/main/java/org/apache/nifi/jasn1/preprocess/preprocessors/ConstraintAsnPreprocessor.java: ########## @@ -0,0 +1,91 @@ +/* + * 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. + */ +package org.apache.nifi.jasn1.preprocess.preprocessors; + +import org.apache.nifi.jasn1.preprocess.NiFiASNPreprocessor; + +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +public class ConstraintAsnPreprocessor implements NiFiASNPreprocessor { + public static final String OPEN_BRACKET = "("; + public static final String CLOSE_BRACKET = ")"; + + public static final Pattern ALLOWED = Pattern.compile("^(\\d+\\))(.*)"); + + @Override + public List<String> preprocessAsn(List<String> lines) { + List<String> preprocessedLines = new ArrayList<>(); + + AtomicInteger unclosedCounter = new AtomicInteger(0); + lines.forEach(line -> { + StringBuilder preprocessedLine = new StringBuilder(); + + String contentToProcess = line; Review Comment: Do we need this declaration? ########## nifi-nar-bundles/nifi-asn1-bundle/nifi-asn1-services/src/test/java/org/apache/nifi/jasn1/preprocess/preprocessors/AbstractPreprocessorTest.java: ########## @@ -0,0 +1,48 @@ +/* + * 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. + */ +package org.apache.nifi.jasn1.preprocess.preprocessors; + +import org.apache.nifi.jasn1.preprocess.NiFiASNPreprocessor; + +import java.io.IOException; +import java.net.URISyntaxException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Paths; +import java.util.List; +import java.util.stream.Collectors; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +public abstract class AbstractPreprocessorTest { Review Comment: I recommend including ASN in the name of the class. ########## nifi-nar-bundles/nifi-asn1-bundle/nifi-asn1-services/src/main/java/org/apache/nifi/jasn1/JASN1Reader.java: ########## @@ -134,17 +135,32 @@ public class JASN1Reader extends AbstractConfigurableComponent implements Record .required(false) .build(); + private static final PropertyDescriptor PREPROCESS_OUTPUT_DIRECTORY = new PropertyDescriptor.Builder() + .name("additional-preprocesszing-output-directory") + .displayName("Additional Preprocessing Output Directory") + .description("When set, NiFi will do additional preprocessing steps that creates modified versions of the provided ASN files," + + " removing unsupported features in a way that makes them less strict but otherwise should still be compatible with incoming data." + + " The original files will remain intact and new ones will be created with the same names in the provided directory." + + " For more information about these additional preprocessing steps please see Additional Details - Additional Preprocessing.") + .expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY) Review Comment: Recommend removing support for Variable Registry expressions because this feature is subject to removal and Parameters should be used instead. ########## nifi-nar-bundles/nifi-asn1-bundle/nifi-asn1-services/src/main/java/org/apache/nifi/jasn1/JASN1Reader.java: ########## @@ -366,13 +387,13 @@ public void reportError(RecognitionException e) { } }; + AsnModel model = new AsnModel(); + parser.module_definitions(model); + if (parseError.get()) { Review Comment: Wouldn't it be less wordy if we threw the exception in the reportError method? ########## nifi-nar-bundles/nifi-asn1-bundle/nifi-asn1-services/src/main/java/org/apache/nifi/jasn1/preprocess/preprocessors/ConstraintAsnPreprocessor.java: ########## @@ -0,0 +1,91 @@ +/* + * 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. + */ +package org.apache.nifi.jasn1.preprocess.preprocessors; + +import org.apache.nifi.jasn1.preprocess.NiFiASNPreprocessor; + +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +public class ConstraintAsnPreprocessor implements NiFiASNPreprocessor { Review Comment: Overall I feel uneasy about this preprocessor. We are automatically removing constraints that the user otherwise would require. I understand that this is documented in the additionalDetails, but I don't think it is enough. When using this preprocessor, I would notify the user that we are doing something risky and will transfer files without checking for constraints. ########## nifi-nar-bundles/nifi-asn1-bundle/nifi-asn1-services/src/main/resources/docs/org.apache.nifi.jasn1.JASN1Reader/additionalDetails.html: ########## @@ -122,5 +122,61 @@ <h3>Troubleshooting</h3> </li> </ol> </p> + + <h3>Additional Preprocessing</h3> + + <p> + NiFi doesn't support every feature that the ASN standard allows. To alleviate problems when encountering ASN files with unsupported features, + NiFi can do additional preprocessing steps that creates modified versions of the provided ASN files, + removing unsupported features in a way that makes them less strict but otherwise should still be compatible with incoming data. + The original files will remain intact and new ones will be created with the same names in a directory set in the 'Additional Preprocessing Output Directory' property. + Please note that this is a best-effort attempt. It is also strongly recommended to compare the resulting ASN files to the originals and make sure they are still appropriate. + <br /> + <br /> + The following modification are applied: + <ol> + <li> Review Comment: I would instead order this list by creating some precedence. Since "hugging comments" are not ASN features, I would move them to the end of the list. ########## nifi-nar-bundles/nifi-asn1-bundle/nifi-asn1-services/src/test/java/org/apache/nifi/jasn1/preprocess/NiFiASNPreprocessorEngineTest.java: ########## @@ -0,0 +1,139 @@ +/* + * 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. + */ +package org.apache.nifi.jasn1.preprocess; + +import org.apache.nifi.logging.ComponentLog; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.Arrays; +import java.util.List; +import java.util.StringJoiner; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +public class NiFiASNPreprocessorEngineTest { Review Comment: I recommend adding a test which tests a file which requires all processors. Cases like `[[ integerField3 INTEGER(SIZE(1..8,...,10|12|20)) OPTIONAL]]--hugging`. What do you think? ########## nifi-nar-bundles/nifi-asn1-bundle/nifi-asn1-services/src/main/java/org/apache/nifi/jasn1/JASN1Reader.java: ########## @@ -134,17 +135,32 @@ public class JASN1Reader extends AbstractConfigurableComponent implements Record .required(false) .build(); + private static final PropertyDescriptor PREPROCESS_OUTPUT_DIRECTORY = new PropertyDescriptor.Builder() + .name("additional-preprocesszing-output-directory") + .displayName("Additional Preprocessing Output Directory") + .description("When set, NiFi will do additional preprocessing steps that creates modified versions of the provided ASN files," + + " removing unsupported features in a way that makes them less strict but otherwise should still be compatible with incoming data." + + " The original files will remain intact and new ones will be created with the same names in the provided directory." + + " For more information about these additional preprocessing steps please see Additional Details - Additional Preprocessing.") + .expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY) + .addValidator(StandardValidators.createDirectoryExistsValidator(true, false)) + .required(false) + .build(); + private final List<PropertyDescriptor> propertyDescriptors = Arrays.asList( ROOT_MODEL_NAME, ROOT_CLASS_NAME, - ASN_FILES + ASN_FILES, + PREPROCESS_OUTPUT_DIRECTORY ); private String identifier; ComponentLog logger; private RecordSchemaProvider schemaProvider = new RecordSchemaProvider(); + private NiFiASNPreprocessorEngine asnPreprocessorEngine = new NiFiASNPreprocessorEngine(); Review Comment: I recommend creating this engine only when the property is set that enables it. I'm wondering if it would make sense to make the preprocess (and the hierarchy behind it) static since it's not stateful. ########## nifi-nar-bundles/nifi-asn1-bundle/nifi-asn1-services/src/main/java/org/apache/nifi/jasn1/JASN1Reader.java: ########## @@ -134,17 +135,32 @@ public class JASN1Reader extends AbstractConfigurableComponent implements Record .required(false) .build(); + private static final PropertyDescriptor PREPROCESS_OUTPUT_DIRECTORY = new PropertyDescriptor.Builder() + .name("additional-preprocesszing-output-directory") Review Comment: There is a typo in preprocessing. ########## nifi-nar-bundles/nifi-asn1-bundle/nifi-asn1-services/src/main/java/org/apache/nifi/jasn1/JASN1Reader.java: ########## @@ -134,17 +135,32 @@ public class JASN1Reader extends AbstractConfigurableComponent implements Record .required(false) .build(); + private static final PropertyDescriptor PREPROCESS_OUTPUT_DIRECTORY = new PropertyDescriptor.Builder() Review Comment: I think this property is too crowded. I recommend separating it into one that enables preprocessing and one that allows printing out the modified asn files for debugging purposes. -- 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]
