turcsanyip commented on code in PR #8368: URL: https://github.com/apache/nifi/pull/8368#discussion_r1487691256
########## nifi-nar-bundles/nifi-aws-bundle/nifi-aws-abstract-processors/src/main/java/org/apache/nifi/processors/aws/util/RegionUtilV1.java: ########## @@ -0,0 +1,97 @@ +/* + * 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.processors.aws.util; + +import com.amazonaws.regions.Region; +import com.amazonaws.regions.Regions; +import org.apache.commons.lang3.ArrayUtils; +import org.apache.nifi.components.AllowableValue; +import org.apache.nifi.components.PropertyDescriptor; +import org.apache.nifi.context.PropertyContext; +import org.apache.nifi.processor.exception.ProcessException; + +import java.util.ArrayList; +import java.util.List; +import java.util.Map; + +/** + * Utility class for AWS region methods. This class uses AWS SDK v1. + * + */ +public final class RegionUtilV1 { + + private RegionUtilV1() { + } + + public static final String S3_REGION_ATTRIBUTE = "s3.region" ; + public static final AllowableValue ATTRIBUTE_DEFINED_REGION = new AllowableValue("attribute-defined-region", + "Use '" + S3_REGION_ATTRIBUTE + "' Attribute", + "Uses '" + S3_REGION_ATTRIBUTE + "' FlowFile attribute as region."); + + public static final PropertyDescriptor REGION = new PropertyDescriptor.Builder() + .name("Region") + .description("The AWS Region to connect to.") + .required(true) + .allowableValues(getAvailableRegions()) + .defaultValue(createAllowableValue(Regions.DEFAULT_REGION).getValue()) + .build(); + + public static final PropertyDescriptor S3_REGION = new PropertyDescriptor.Builder() + .fromPropertyDescriptor(REGION) + .allowableValues(getAvailableS3Regions()) + .build(); + + public static Region parseRegionValue(String regionValue) { + if (regionValue == null) { + throw new ProcessException(String.format("[%s] was selected as region source but [%s] attribute does not exist", ATTRIBUTE_DEFINED_REGION, S3_REGION_ATTRIBUTE)); + } + + try { + return Region.getRegion(Regions.fromName(regionValue)); + } catch (Exception e) { + throw new ProcessException(String.format("The [%s] attribute contains an invalid region value [%s]", S3_REGION_ATTRIBUTE, regionValue), e); + } + } + + public static Region resolveRegion(final PropertyContext context, final Map<String, String> attributes) { + String regionValue = context.getProperty(S3_REGION).getValue(); Review Comment: The method is S3-specific. ```suggestion public static Region resolveS3Region(final PropertyContext context, final Map<String, String> attributes) { String regionValue = context.getProperty(S3_REGION).getValue(); ``` ########## nifi-nar-bundles/nifi-aws-bundle/nifi-aws-processors/src/main/java/org/apache/nifi/processors/aws/s3/S3FileResourceService.java: ########## @@ -0,0 +1,153 @@ +/* + * 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.processors.aws.s3; + +import com.amazonaws.SdkClientException; +import com.amazonaws.auth.AWSCredentialsProvider; +import com.amazonaws.regions.Region; +import com.amazonaws.services.s3.AmazonS3; +import com.amazonaws.services.s3.AmazonS3Client; +import com.amazonaws.services.s3.model.S3Object; +import com.github.benmanes.caffeine.cache.Cache; +import com.github.benmanes.caffeine.cache.Caffeine; +import org.apache.nifi.annotation.documentation.CapabilityDescription; +import org.apache.nifi.annotation.documentation.SeeAlso; +import org.apache.nifi.annotation.documentation.Tags; +import org.apache.nifi.annotation.documentation.UseCase; +import org.apache.nifi.annotation.lifecycle.OnDisabled; +import org.apache.nifi.annotation.lifecycle.OnEnabled; +import org.apache.nifi.components.PropertyDescriptor; +import org.apache.nifi.context.PropertyContext; +import org.apache.nifi.controller.AbstractControllerService; +import org.apache.nifi.controller.ConfigurationContext; +import org.apache.nifi.fileresource.service.api.FileResource; +import org.apache.nifi.fileresource.service.api.FileResourceService; +import org.apache.nifi.processor.exception.ProcessException; +import org.apache.nifi.processors.aws.credentials.provider.service.AWSCredentialsProviderService; +import org.apache.nifi.processors.aws.util.RegionUtilV1; + +import java.util.List; +import java.util.Map; + +import static org.apache.nifi.processors.aws.AbstractAWSCredentialsProviderProcessor.AWS_CREDENTIALS_PROVIDER_SERVICE; +import static org.apache.nifi.processors.aws.util.RegionUtilV1.resolveRegion; +import static org.apache.nifi.util.StringUtils.isBlank; + +@Tags({"Amazon", "S3", "AWS", "file", "resource"}) +@SeeAlso({FetchS3Object.class}) +@CapabilityDescription("Provides an Amazon Web Services (AWS) S3 file resource for other components.") +@UseCase( + description = "Fetch a specific file from S3. " + + "The service provides higher performance compared to fetch processors when the data should be moved between different storages without any transformation.", + configuration = """ + "Bucket" = "${s3.bucket}" + "Name" = "${filename}" Review Comment: Copy-paste error? ```suggestion "Object Key" = "${filename}" ``` ########## nifi-nar-bundles/nifi-aws-bundle/nifi-aws-processors/src/main/java/org/apache/nifi/processors/aws/s3/S3FileResourceService.java: ########## @@ -0,0 +1,153 @@ +/* + * 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.processors.aws.s3; Review Comment: I suggest moving `S3FileResourceService` into `...aws.s3.service` sub-package in order to separate processors and services. ########## nifi-nar-bundles/nifi-aws-bundle/nifi-aws-abstract-processors/src/main/java/org/apache/nifi/processors/aws/util/RegionUtilV1.java: ########## @@ -0,0 +1,97 @@ +/* + * 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.processors.aws.util; + +import com.amazonaws.regions.Region; +import com.amazonaws.regions.Regions; +import org.apache.commons.lang3.ArrayUtils; +import org.apache.nifi.components.AllowableValue; +import org.apache.nifi.components.PropertyDescriptor; +import org.apache.nifi.context.PropertyContext; +import org.apache.nifi.processor.exception.ProcessException; + +import java.util.ArrayList; +import java.util.List; +import java.util.Map; + +/** + * Utility class for AWS region methods. This class uses AWS SDK v1. + * + */ +public final class RegionUtilV1 { + + private RegionUtilV1() { + } + + public static final String S3_REGION_ATTRIBUTE = "s3.region" ; + public static final AllowableValue ATTRIBUTE_DEFINED_REGION = new AllowableValue("attribute-defined-region", + "Use '" + S3_REGION_ATTRIBUTE + "' Attribute", + "Uses '" + S3_REGION_ATTRIBUTE + "' FlowFile attribute as region."); + + public static final PropertyDescriptor REGION = new PropertyDescriptor.Builder() + .name("Region") + .description("The AWS Region to connect to.") + .required(true) + .allowableValues(getAvailableRegions()) + .defaultValue(createAllowableValue(Regions.DEFAULT_REGION).getValue()) + .build(); + + public static final PropertyDescriptor S3_REGION = new PropertyDescriptor.Builder() + .fromPropertyDescriptor(REGION) + .allowableValues(getAvailableS3Regions()) + .build(); + + public static Region parseRegionValue(String regionValue) { Review Comment: The method should be `private`. It is used in this class only. Also, the error messages are tightly related to the S3 Region attribute resolution, not a generic parse region function. ########## nifi-nar-bundles/nifi-aws-bundle/nifi-aws-processors/src/test/java/org/apache/nifi/processors/aws/sqs/AbstractSQSIT.java: ########## @@ -84,7 +85,7 @@ protected TestRunner initRunner(final Class<? extends Processor> processorClass) TestRunner runner = TestRunners.newTestRunner(processorClass); AuthUtils.enableAccessKey(runner, localstack.getAccessKey(), localstack.getSecretKey()); - runner.setProperty(AbstractS3Processor.S3_REGION, localstack.getRegion()); + runner.setProperty(RegionUtilV1.S3_REGION, localstack.getRegion()); runner.setProperty(AbstractS3Processor.ENDPOINT_OVERRIDE, localstack.getEndpointOverride(LocalStackContainer.Service.SQS).toString()); Review Comment: Not sure why `AbstractS3Processor` was used originally. The SQS processors' properties should have been referenced which come from `AbstractAwsProcessor`. ```suggestion runner.setProperty(AbstractAwsProcessor.REGION, localstack.getRegion()); runner.setProperty(AbstractAwsProcessor.ENDPOINT_OVERRIDE, localstack.getEndpointOverride(LocalStackContainer.Service.SQS).toString()); ``` ########## nifi-nar-bundles/nifi-aws-bundle/nifi-aws-processors/src/test/java/org/apache/nifi/processors/aws/s3/TestListS3.java: ########## @@ -102,7 +103,7 @@ protected AmazonS3Client createClient(final ProcessContext context, final AWSCre @Test public void testList() { - runner.setProperty(ListS3.REGION, "eu-west-1"); + runner.setProperty(RegionUtilV1.S3_REGION, "eu-west-1"); Review Comment: ```suggestion runner.setProperty(RegionUtilV1.REGION, "eu-west-1"); ``` ########## nifi-nar-bundles/nifi-aws-bundle/nifi-aws-abstract-processors/src/main/java/org/apache/nifi/processors/aws/util/RegionUtilV1.java: ########## @@ -0,0 +1,97 @@ +/* + * 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.processors.aws.util; + +import com.amazonaws.regions.Region; +import com.amazonaws.regions.Regions; +import org.apache.commons.lang3.ArrayUtils; +import org.apache.nifi.components.AllowableValue; +import org.apache.nifi.components.PropertyDescriptor; +import org.apache.nifi.context.PropertyContext; +import org.apache.nifi.processor.exception.ProcessException; + +import java.util.ArrayList; +import java.util.List; +import java.util.Map; + +/** + * Utility class for AWS region methods. This class uses AWS SDK v1. + * + */ +public final class RegionUtilV1 { + + private RegionUtilV1() { + } + + public static final String S3_REGION_ATTRIBUTE = "s3.region" ; + public static final AllowableValue ATTRIBUTE_DEFINED_REGION = new AllowableValue("attribute-defined-region", + "Use '" + S3_REGION_ATTRIBUTE + "' Attribute", + "Uses '" + S3_REGION_ATTRIBUTE + "' FlowFile attribute as region."); + + public static final PropertyDescriptor REGION = new PropertyDescriptor.Builder() + .name("Region") + .description("The AWS Region to connect to.") + .required(true) + .allowableValues(getAvailableRegions()) + .defaultValue(createAllowableValue(Regions.DEFAULT_REGION).getValue()) + .build(); + + public static final PropertyDescriptor S3_REGION = new PropertyDescriptor.Builder() + .fromPropertyDescriptor(REGION) + .allowableValues(getAvailableS3Regions()) + .build(); + + public static Region parseRegionValue(String regionValue) { + if (regionValue == null) { + throw new ProcessException(String.format("[%s] was selected as region source but [%s] attribute does not exist", ATTRIBUTE_DEFINED_REGION, S3_REGION_ATTRIBUTE)); + } + + try { + return Region.getRegion(Regions.fromName(regionValue)); + } catch (Exception e) { + throw new ProcessException(String.format("The [%s] attribute contains an invalid region value [%s]", S3_REGION_ATTRIBUTE, regionValue), e); + } + } + + public static Region resolveRegion(final PropertyContext context, final Map<String, String> attributes) { + String regionValue = context.getProperty(S3_REGION).getValue(); + + if (ATTRIBUTE_DEFINED_REGION.getValue().equals(regionValue)) { + regionValue = attributes.get(S3_REGION_ATTRIBUTE); + } + + return parseRegionValue(regionValue); + } + + public static AllowableValue[] getAvailableS3Regions() { + final AllowableValue[] availableRegions = getAvailableRegions(); + return ArrayUtils.addAll(availableRegions, ATTRIBUTE_DEFINED_REGION); Review Comment: Minor: ```suggestion return ArrayUtils.add(availableRegions, ATTRIBUTE_DEFINED_REGION); ``` ########## nifi-nar-bundles/nifi-aws-bundle/nifi-aws-processors/src/main/java/org/apache/nifi/processors/aws/s3/ListS3.java: ########## @@ -99,6 +99,9 @@ import java.util.function.Function; import java.util.stream.Collectors; +import static org.apache.nifi.processors.aws.util.RegionUtilV1.REGION; +import static org.apache.nifi.processors.aws.util.RegionUtilV1.S3_REGION; Review Comment: I don't think `S3_REGION` is needed in the List processor. The processor has the simple `REGION` property which does not have the `Use 's3.region' Attribute` option because the List processor has no incoming FlowFile. Appearing `S3_REGION` in the code seems to be due to improper usage of the properties in former commits. Could you please modify those references to `REGION` and omit the import? ########## nifi-nar-bundles/nifi-aws-bundle/nifi-aws-processors/src/main/java/org/apache/nifi/processors/aws/s3/S3FileResourceService.java: ########## @@ -0,0 +1,153 @@ +/* + * 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.processors.aws.s3; + +import com.amazonaws.SdkClientException; +import com.amazonaws.auth.AWSCredentialsProvider; +import com.amazonaws.regions.Region; +import com.amazonaws.services.s3.AmazonS3; +import com.amazonaws.services.s3.AmazonS3Client; +import com.amazonaws.services.s3.model.S3Object; +import com.github.benmanes.caffeine.cache.Cache; +import com.github.benmanes.caffeine.cache.Caffeine; +import org.apache.nifi.annotation.documentation.CapabilityDescription; +import org.apache.nifi.annotation.documentation.SeeAlso; +import org.apache.nifi.annotation.documentation.Tags; +import org.apache.nifi.annotation.documentation.UseCase; +import org.apache.nifi.annotation.lifecycle.OnDisabled; +import org.apache.nifi.annotation.lifecycle.OnEnabled; +import org.apache.nifi.components.PropertyDescriptor; +import org.apache.nifi.context.PropertyContext; +import org.apache.nifi.controller.AbstractControllerService; +import org.apache.nifi.controller.ConfigurationContext; +import org.apache.nifi.fileresource.service.api.FileResource; +import org.apache.nifi.fileresource.service.api.FileResourceService; +import org.apache.nifi.processor.exception.ProcessException; +import org.apache.nifi.processors.aws.credentials.provider.service.AWSCredentialsProviderService; +import org.apache.nifi.processors.aws.util.RegionUtilV1; + +import java.util.List; +import java.util.Map; + +import static org.apache.nifi.processors.aws.AbstractAWSCredentialsProviderProcessor.AWS_CREDENTIALS_PROVIDER_SERVICE; +import static org.apache.nifi.processors.aws.util.RegionUtilV1.resolveRegion; +import static org.apache.nifi.util.StringUtils.isBlank; + +@Tags({"Amazon", "S3", "AWS", "file", "resource"}) +@SeeAlso({FetchS3Object.class}) +@CapabilityDescription("Provides an Amazon Web Services (AWS) S3 file resource for other components.") +@UseCase( + description = "Fetch a specific file from S3. " + + "The service provides higher performance compared to fetch processors when the data should be moved between different storages without any transformation.", + configuration = """ + "Bucket" = "${s3.bucket}" + "Name" = "${filename}" + + The "Region" property must be set to denote the S3 region that the Bucket resides in. + If the flow being built is to be reused elsewhere, it's a good idea to parameterize this property by setting it to something like #{S3_REGION}. Review Comment: It is not clear for me what you would like to highlight here. Using `#{...}` parameters is quite a generic pattern and it could be mentioned for every properties (e.g. for Bucket too). Do you mean the `Use 's3.region' Attribute` option? In that case I would mention its name and `${s3.region}` attribute expression. -- 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]
