galovics commented on code in PR #2915: URL: https://github.com/apache/fineract/pull/2915#discussion_r1091617459
########## fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/export/CsvDatatableReportExportServiceImpl.java: ########## @@ -0,0 +1,51 @@ +/** + * 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.fineract.infrastructure.dataqueries.service.export; + +import java.util.Map; +import javax.ws.rs.core.MultivaluedMap; +import javax.ws.rs.core.Response; +import javax.ws.rs.core.StreamingOutput; +import lombok.RequiredArgsConstructor; +import org.apache.fineract.infrastructure.dataqueries.service.DatatableExportTargetParameter; +import org.apache.fineract.infrastructure.dataqueries.service.ReadReportingService; +import org.springframework.stereotype.Service; + +@Service +@RequiredArgsConstructor +public class CsvDatatableReportExportServiceImpl implements DatatableReportExportService { + + private final ReadReportingService readExtraDataAndReportingService; + + @Override + public Response export(String reportName, MultivaluedMap<String, String> queryParams, Map<String, String> reportParams, Review Comment: I'm not gonna push back on the PR because of this but just a note. This is a service layer class yet it's returning a Response which is a view layer class. We're mixing up the layers. ########## .github/workflows/build-mariadb.yml: ########## @@ -52,12 +52,39 @@ jobs: run: | ./gradlew --no-daemon -q createDB -PdbName=fineract_tenants ./gradlew --no-daemon -q createDB -PdbName=fineract_default + - name: Start LocalStack + env: + AWS_ENDPOINT_URL: http://localhost:4566 + AWS_ACCESS_KEY_ID: localstack + AWS_SECRET_ACCESS_KEY: localstack + AWS_REGION: us-east-1 + run: | + echo "Update python pyopenssl" + pip install --upgrade pyopenssl + echo "Install localstack" + pip install localstack awscli-local[ver1] # install LocalStack cli and awslocal + docker pull localstack/localstack # Make sure to pull the latest version of the image Review Comment: Aren't we mixing up 2 different approaches here? One is starting localstack via its own CLI tool and one via docker? ########## fineract-provider/src/main/java/org/apache/fineract/infrastructure/s3/AmazonS3Config.java: ########## @@ -0,0 +1,75 @@ +/** + * 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.fineract.infrastructure.s3; + +import java.net.URI; +import org.apache.fineract.infrastructure.configuration.domain.ConfigurationDomainService; +import org.apache.fineract.infrastructure.dataqueries.service.ReadReportingService; +import org.apache.fineract.infrastructure.dataqueries.service.export.S3DatatableReportExportServiceImpl; +import org.apache.poi.util.StringUtil; +import org.springframework.boot.autoconfigure.condition.ConditionalOnBean; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Conditional; +import org.springframework.context.annotation.Configuration; +import org.springframework.context.annotation.Profile; +import software.amazon.awssdk.auth.credentials.DefaultCredentialsProvider; +import software.amazon.awssdk.regions.providers.AwsRegionProvider; +import software.amazon.awssdk.regions.providers.DefaultAwsRegionProviderChain; +import software.amazon.awssdk.services.s3.S3Client; +import software.amazon.awssdk.services.s3.S3ClientBuilder; + +@Configuration +public class AmazonS3Config { + + @Bean + public DefaultCredentialsProvider awsCredentialsProvider() { + return DefaultCredentialsProvider.create(); + } + + @Bean + public AwsRegionProvider awsRegionProvider() { + return DefaultAwsRegionProviderChain.builder().build(); + } + + @Bean("s3Client") + @Profile("!test") + @Conditional(AmazonS3ConfigCondition.class) + public S3Client s3Client(DefaultCredentialsProvider awsCredentialsProvider, AwsRegionProvider awsRegionProvider) { + return S3Client.builder().credentialsProvider(awsCredentialsProvider).region(awsRegionProvider.getRegion()).build(); + } + + @Bean("s3Client") + @Profile("test") + @Conditional(AmazonS3ConfigCondition.class) + public S3Client s3ClientTest(DefaultCredentialsProvider awsCredentialsProvider, AwsRegionProvider awsRegionProvider) { + S3ClientBuilder builder = S3Client.builder(); + String env = System.getenv().getOrDefault("AWS_ENDPOINT_URL", ""); Review Comment: You can use the Environment class from Spring. ########## fineract-provider/src/main/java/org/apache/fineract/infrastructure/s3/AmazonS3Config.java: ########## @@ -0,0 +1,75 @@ +/** + * 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.fineract.infrastructure.s3; + +import java.net.URI; +import org.apache.fineract.infrastructure.configuration.domain.ConfigurationDomainService; +import org.apache.fineract.infrastructure.dataqueries.service.ReadReportingService; +import org.apache.fineract.infrastructure.dataqueries.service.export.S3DatatableReportExportServiceImpl; +import org.apache.poi.util.StringUtil; +import org.springframework.boot.autoconfigure.condition.ConditionalOnBean; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Conditional; +import org.springframework.context.annotation.Configuration; +import org.springframework.context.annotation.Profile; +import software.amazon.awssdk.auth.credentials.DefaultCredentialsProvider; +import software.amazon.awssdk.regions.providers.AwsRegionProvider; +import software.amazon.awssdk.regions.providers.DefaultAwsRegionProviderChain; +import software.amazon.awssdk.services.s3.S3Client; +import software.amazon.awssdk.services.s3.S3ClientBuilder; + +@Configuration +public class AmazonS3Config { + + @Bean + public DefaultCredentialsProvider awsCredentialsProvider() { + return DefaultCredentialsProvider.create(); + } + + @Bean + public AwsRegionProvider awsRegionProvider() { + return DefaultAwsRegionProviderChain.builder().build(); + } + + @Bean("s3Client") Review Comment: No need to specify the bean name, it'll automatically get the method name. ########## fineract-provider/src/main/java/org/apache/fineract/infrastructure/s3/AmazonS3ConfigCondition.java: ########## @@ -0,0 +1,53 @@ +/** + * 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.fineract.infrastructure.s3; + +import org.apache.commons.lang3.StringUtils; +import org.springframework.beans.factory.annotation.Value; +import org.springframework.context.annotation.Condition; +import org.springframework.context.annotation.ConditionContext; +import org.springframework.core.type.AnnotatedTypeMetadata; +import software.amazon.awssdk.auth.credentials.AwsCredentialsProvider; +import software.amazon.awssdk.regions.providers.AwsRegionProvider; + +public class AmazonS3ConfigCondition implements Condition { + + @Value("${fineract.report.export.s3.enabled}") Review Comment: FineractProperties? ########## integration-tests/src/test/java/org/apache/fineract/integrationtests/client/ReportExportTest.java: ########## @@ -0,0 +1,63 @@ +/** + * 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.fineract.integrationtests.client; + +import java.io.IOException; +import java.util.Map; +import okhttp3.MediaType; +import okhttp3.ResponseBody; +import org.apache.fineract.integrationtests.GithubOnly; +import org.apache.fineract.integrationtests.common.Utils; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Disabled; +import org.junit.jupiter.api.Tag; +import org.junit.jupiter.api.Test; +import retrofit2.Response; + +/** + * Integration Test for /runreports/ API. + * + * @author Michael Vorburger.ch + */ +@Tag("github_only") Review Comment: Do we need this tag? ########## fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/export/S3DatatableReportExportServiceImpl.java: ########## @@ -0,0 +1,71 @@ +/** + * 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.fineract.infrastructure.dataqueries.service.export; + +import java.io.ByteArrayOutputStream; +import java.io.IOException; +import java.util.Map; +import javax.ws.rs.core.MultivaluedMap; +import javax.ws.rs.core.Response; +import javax.ws.rs.core.StreamingOutput; +import lombok.RequiredArgsConstructor; +import org.apache.fineract.infrastructure.configuration.domain.ConfigurationDomainService; +import org.apache.fineract.infrastructure.dataqueries.service.DatatableExportTargetParameter; +import org.apache.fineract.infrastructure.dataqueries.service.ReadReportingService; +import org.springframework.beans.factory.annotation.Value; +import software.amazon.awssdk.core.sync.RequestBody; +import software.amazon.awssdk.services.s3.S3Client; + +@RequiredArgsConstructor +public class S3DatatableReportExportServiceImpl implements DatatableReportExportService { + + public static final int AWS_S3_MAXIMUM_KEY_LENGTH = 1024; + private final ReadReportingService readExtraDataAndReportingService; + + private final ConfigurationDomainService configurationDomainService; + private final S3Client s3Client; + + @Value("${fineract.report.export.s3.bucket:reports}") Review Comment: FineractProperties class? ########## fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/export/CsvDatatableReportExportServiceImpl.java: ########## @@ -0,0 +1,51 @@ +/** + * 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.fineract.infrastructure.dataqueries.service.export; + +import java.util.Map; +import javax.ws.rs.core.MultivaluedMap; +import javax.ws.rs.core.Response; +import javax.ws.rs.core.StreamingOutput; +import lombok.RequiredArgsConstructor; +import org.apache.fineract.infrastructure.dataqueries.service.DatatableExportTargetParameter; +import org.apache.fineract.infrastructure.dataqueries.service.ReadReportingService; +import org.springframework.stereotype.Service; + +@Service +@RequiredArgsConstructor +public class CsvDatatableReportExportServiceImpl implements DatatableReportExportService { + + private final ReadReportingService readExtraDataAndReportingService; + + @Override + public Response export(String reportName, MultivaluedMap<String, String> queryParams, Map<String, String> reportParams, + boolean isSelfServiceUserReport, String parameterTypeValue) { + final StreamingOutput result = this.readExtraDataAndReportingService.retrieveReportCSV(reportName, parameterTypeValue, reportParams, + isSelfServiceUserReport); + + return Response.ok().entity(result).type("text/csv") + .header("Content-Disposition", "attachment;filename=" + reportName.replaceAll(" ", "") + ".csv").build(); Review Comment: When it comes to the attachment's name, shouldn't we do the same thing as for DatatableExportUtil#normalizeFolderName? ########## fineract-provider/src/main/java/org/apache/fineract/infrastructure/s3/AmazonS3Config.java: ########## @@ -0,0 +1,75 @@ +/** + * 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.fineract.infrastructure.s3; + +import java.net.URI; +import org.apache.fineract.infrastructure.configuration.domain.ConfigurationDomainService; +import org.apache.fineract.infrastructure.dataqueries.service.ReadReportingService; +import org.apache.fineract.infrastructure.dataqueries.service.export.S3DatatableReportExportServiceImpl; +import org.apache.poi.util.StringUtil; +import org.springframework.boot.autoconfigure.condition.ConditionalOnBean; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Conditional; +import org.springframework.context.annotation.Configuration; +import org.springframework.context.annotation.Profile; +import software.amazon.awssdk.auth.credentials.DefaultCredentialsProvider; +import software.amazon.awssdk.regions.providers.AwsRegionProvider; +import software.amazon.awssdk.regions.providers.DefaultAwsRegionProviderChain; +import software.amazon.awssdk.services.s3.S3Client; +import software.amazon.awssdk.services.s3.S3ClientBuilder; + +@Configuration +public class AmazonS3Config { + + @Bean + public DefaultCredentialsProvider awsCredentialsProvider() { + return DefaultCredentialsProvider.create(); + } + + @Bean + public AwsRegionProvider awsRegionProvider() { + return DefaultAwsRegionProviderChain.builder().build(); + } + + @Bean("s3Client") + @Profile("!test") + @Conditional(AmazonS3ConfigCondition.class) + public S3Client s3Client(DefaultCredentialsProvider awsCredentialsProvider, AwsRegionProvider awsRegionProvider) { + return S3Client.builder().credentialsProvider(awsCredentialsProvider).region(awsRegionProvider.getRegion()).build(); + } + + @Bean("s3Client") Review Comment: Btw I'd approach this differently a bit. I'm not saying it's not going to work but would be cleaner. Let's assume we have this config: ``` @Bean @Conditional(AmazonS3ConfigCondition.class) public S3Client s3Client(DefaultCredentialsProvider awsCredentialsProvider, AwsRegionProvider awsRegionProvider) { return S3Client.builder().credentialsProvider(awsCredentialsProvider).region(awsRegionProvider.getRegion()).build(); } ``` Let's create a custom interface: ``` interface S3ClientCustomizer { void customize(S3ClientBuilder builder); } ``` Then let's change the bean config: ``` @Bean @Conditional(AmazonS3ConfigCondition.class) public S3Client s3Client(DefaultCredentialsProvider awsCredentialsProvider, AwsRegionProvider awsRegionProvider, List<S3ClientCustomizer> customizers) { S3ClientBuilder builder = S3Client.builder().credentialsProvider(awsCredentialsProvider).region(awsRegionProvider.getRegion()); customizers.forEach(c -> c.customize(builder)); return builder.build(); } ``` And then we create a customizer for the tests: ``` @Component @RequiredArgsConstructor @Profile("test") public class LocalstackS3ClientCustomizer implements S3ClientCustomizer { private final Environment environment; @Override public void customize(S3ClientBuilder builder) { String env = environment..... if (StringUtil.isNotBlank(env)) { builder.endpointOverride(URI.create(env)).forcePathStyle(true); } } } ``` ########## integration-tests/src/test/java/org/apache/fineract/integrationtests/client/ReportExportTest.java: ########## @@ -0,0 +1,63 @@ +/** + * 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.fineract.integrationtests.client; + +import java.io.IOException; +import java.util.Map; +import okhttp3.MediaType; +import okhttp3.ResponseBody; +import org.apache.fineract.integrationtests.GithubOnly; +import org.apache.fineract.integrationtests.common.Utils; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Disabled; +import org.junit.jupiter.api.Tag; +import org.junit.jupiter.api.Test; +import retrofit2.Response; + +/** + * Integration Test for /runreports/ API. + * + * @author Michael Vorburger.ch + */ +@Tag("github_only") +public class ReportExportTest extends IntegrationTest { + + @BeforeEach + public void setup() { + Utils.initializeRESTAssured(); + } + + @Test + void runClientListingTableReportCSV() throws IOException { + Response<ResponseBody> result = okR( + fineract().reportsRun.runReportGetFile("Client Listing", Map.of("R_officeId", "1", "exportCSV", "true"), false)); + assertThat(result.body().contentType()).isEqualTo(MediaType.parse("text/csv")); + assertThat(result.body().string()).contains("Office/Branch"); + } + + @Test + @GithubOnly + @Disabled Review Comment: Can't we put the `@Disabled` annotation into the GitHubOnly annotation? This is kinda confusing to me that it's disabled but it's actually not. Also, if you move this to the GitHubOnly annotation, let's put a comment there and explain why. ########## fineract-provider/src/main/java/org/apache/fineract/infrastructure/s3/AmazonS3Config.java: ########## @@ -0,0 +1,75 @@ +/** + * 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.fineract.infrastructure.s3; + +import java.net.URI; +import org.apache.fineract.infrastructure.configuration.domain.ConfigurationDomainService; +import org.apache.fineract.infrastructure.dataqueries.service.ReadReportingService; +import org.apache.fineract.infrastructure.dataqueries.service.export.S3DatatableReportExportServiceImpl; +import org.apache.poi.util.StringUtil; +import org.springframework.boot.autoconfigure.condition.ConditionalOnBean; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Conditional; +import org.springframework.context.annotation.Configuration; +import org.springframework.context.annotation.Profile; +import software.amazon.awssdk.auth.credentials.DefaultCredentialsProvider; +import software.amazon.awssdk.regions.providers.AwsRegionProvider; +import software.amazon.awssdk.regions.providers.DefaultAwsRegionProviderChain; +import software.amazon.awssdk.services.s3.S3Client; +import software.amazon.awssdk.services.s3.S3ClientBuilder; + +@Configuration +public class AmazonS3Config { + + @Bean + public DefaultCredentialsProvider awsCredentialsProvider() { + return DefaultCredentialsProvider.create(); + } + + @Bean + public AwsRegionProvider awsRegionProvider() { + return DefaultAwsRegionProviderChain.builder().build(); + } + + @Bean("s3Client") + @Profile("!test") + @Conditional(AmazonS3ConfigCondition.class) + public S3Client s3Client(DefaultCredentialsProvider awsCredentialsProvider, AwsRegionProvider awsRegionProvider) { + return S3Client.builder().credentialsProvider(awsCredentialsProvider).region(awsRegionProvider.getRegion()).build(); + } + + @Bean("s3Client") + @Profile("test") + @Conditional(AmazonS3ConfigCondition.class) + public S3Client s3ClientTest(DefaultCredentialsProvider awsCredentialsProvider, AwsRegionProvider awsRegionProvider) { + S3ClientBuilder builder = S3Client.builder(); + String env = System.getenv().getOrDefault("AWS_ENDPOINT_URL", ""); + if (StringUtil.isNotBlank(env)) { + builder.endpointOverride(URI.create(env)).forcePathStyle(true); + } + return builder.credentialsProvider(awsCredentialsProvider).region(awsRegionProvider.getRegion()).build(); + } + + @Bean + @ConditionalOnBean(S3Client.class) + public S3DatatableReportExportServiceImpl s3DatatableReportExportServiceImpl(ReadReportingService reportServiceImpl, Review Comment: I don't think this bean definition should be here. Btw why don't we remove the definition completely and use the Component annotation on the class itelf? ########## fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/export/S3DatatableReportExportServiceImpl.java: ########## @@ -0,0 +1,71 @@ +/** + * 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.fineract.infrastructure.dataqueries.service.export; + +import java.io.ByteArrayOutputStream; +import java.io.IOException; +import java.util.Map; +import javax.ws.rs.core.MultivaluedMap; +import javax.ws.rs.core.Response; +import javax.ws.rs.core.StreamingOutput; +import lombok.RequiredArgsConstructor; +import org.apache.fineract.infrastructure.configuration.domain.ConfigurationDomainService; +import org.apache.fineract.infrastructure.dataqueries.service.DatatableExportTargetParameter; +import org.apache.fineract.infrastructure.dataqueries.service.ReadReportingService; +import org.springframework.beans.factory.annotation.Value; +import software.amazon.awssdk.core.sync.RequestBody; +import software.amazon.awssdk.services.s3.S3Client; + +@RequiredArgsConstructor +public class S3DatatableReportExportServiceImpl implements DatatableReportExportService { + + public static final int AWS_S3_MAXIMUM_KEY_LENGTH = 1024; + private final ReadReportingService readExtraDataAndReportingService; + + private final ConfigurationDomainService configurationDomainService; + private final S3Client s3Client; + + @Value("${fineract.report.export.s3.bucket:reports}") + private String bucketName; + + @Override + public Response export(String reportName, MultivaluedMap<String, String> queryParams, Map<String, String> reportParams, + boolean isSelfServiceUserReport, String parameterTypeValue) { + try { + StreamingOutput output = this.readExtraDataAndReportingService.retrieveReportCSV(reportName, parameterTypeValue, reportParams, + isSelfServiceUserReport); + try (ByteArrayOutputStream byteArrayOutputStream = new ByteArrayOutputStream()) { + output.write(byteArrayOutputStream); + String folder = configurationDomainService.retrieveReportExportS3FolderName(); + String filePath = DatatableExportUtil.generateDatatableExportFileName(AWS_S3_MAXIMUM_KEY_LENGTH, folder, ".csv", reportName, Review Comment: Can't we change the extension parameter to not need the dot prefix? ########## integration-tests/src/test/java/org/apache/fineract/integrationtests/GithubOnly.java: ########## @@ -0,0 +1,30 @@ +/** + * 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.fineract.integrationtests; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; +import org.junit.jupiter.api.condition.EnabledIfEnvironmentVariable; + +@Retention(RetentionPolicy.RUNTIME) +@Target({ ElementType.TYPE, ElementType.METHOD }) +@EnabledIfEnvironmentVariable(named = "GITHUB_ACTIONS", matches = "true") +public @interface GithubOnly {} Review Comment: Let's rename this to CIOnly. ########## fineract-provider/src/main/java/org/apache/fineract/infrastructure/s3/AmazonS3ConfigCondition.java: ########## @@ -0,0 +1,53 @@ +/** + * 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.fineract.infrastructure.s3; + +import org.apache.commons.lang3.StringUtils; +import org.springframework.beans.factory.annotation.Value; +import org.springframework.context.annotation.Condition; +import org.springframework.context.annotation.ConditionContext; +import org.springframework.core.type.AnnotatedTypeMetadata; +import software.amazon.awssdk.auth.credentials.AwsCredentialsProvider; +import software.amazon.awssdk.regions.providers.AwsRegionProvider; + +public class AmazonS3ConfigCondition implements Condition { + + @Value("${fineract.report.export.s3.enabled}") + private boolean amazonS3Enabled; + + @Value("${fineract.report.export.s3.bucket:reports}") + private String bucketName; + + @Override + public boolean matches(ConditionContext context, AnnotatedTypeMetadata metadata) { + return context.getEnvironment().getRequiredProperty("fineract.report.export.s3.enabled", Boolean.class) + && StringUtils.isNotBlank(context.getEnvironment().getProperty("fineract.report.export.s3.bucket", "")) + && isAwsCredentialValid(context); + } + + private boolean isAwsCredentialValid(ConditionContext context) { + try { + context.getBeanFactory().getBean(AwsCredentialsProvider.class).resolveCredentials(); Review Comment: This is not necessarily the best idea. You're accessing the container before it even gets properly initialized. It's better if you explicitly instantiate the default credentials provider and region provider to test this. https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/context/annotation/Condition.html > Conditions must follow the same restrictions as [BeanFactoryPostProcessor](https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/beans/factory/config/BeanFactoryPostProcessor.html) and take care to never interact with bean instances. ########## fineract-provider/src/main/java/org/apache/fineract/infrastructure/s3/AmazonS3ConfigCondition.java: ########## @@ -0,0 +1,53 @@ +/** + * 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.fineract.infrastructure.s3; + +import org.apache.commons.lang3.StringUtils; +import org.springframework.beans.factory.annotation.Value; +import org.springframework.context.annotation.Condition; +import org.springframework.context.annotation.ConditionContext; +import org.springframework.core.type.AnnotatedTypeMetadata; +import software.amazon.awssdk.auth.credentials.AwsCredentialsProvider; +import software.amazon.awssdk.regions.providers.AwsRegionProvider; + +public class AmazonS3ConfigCondition implements Condition { Review Comment: You can use PropertiesCondition. -- 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]
