Fix #1418 Expose AwsCredentialsProvider to AmazonS3InstallationService by seratch · Pull Request #1419 · slackapi/java-slack-sdk

seratch · 2025-01-14T00:57:52Z

This pull request resolves #1418

Category (place an `x` in each of the `[ ]`)

bolt (Bolt for Java)
bolt-{sub modules} (Bolt for Java - optional modules)
slack-api-client (Slack API Clients)
slack-api-model (Slack API Data Models)
slack-api-*-kotlin-extension (Kotlin Extensions for Slack API Clients)
slack-app-backend (The primitive layer of Bolt for Java)

Requirements

Please read the Contributing guidelines and Code of Conduct before creating this issue or pull request. By submitting, you agree to those rules.

codecov · 2025-01-14T01:04:04Z

Codecov Report

Attention: Patch coverage is 57.14286% with 12 lines in your changes missing coverage. Please review.

Project coverage is 73.17%. Comparing base (253cffc) to head (b53e018).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...olt/service/builtin/AmazonS3OAuthStateService.java	50.00%	5 Missing and 2 partials ⚠️
...t/service/builtin/AmazonS3InstallationService.java	64.28%	3 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1419      +/-   ##
============================================
- Coverage     73.26%   73.17%   -0.09%     
- Complexity     4370     4372       +2     
============================================
  Files           474      474              
  Lines         14185    14205      +20     
  Branches       1433     1437       +4     
============================================
+ Hits          10393    10395       +2     
- Misses         2947     2961      +14     
- Partials        845      849       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bruceadowns · 2025-01-14T01:30:18Z

This change would work fine, however I also need to add region and endpointUrl.

I can put this in a PR if you want, but here is a diff showing you my thinking:

diff --git a/bolt/src/main/java/com/slack/api/bolt/service/builtin/AmazonS3InstallationService.java b/bolt/src/main/java/com/slack/api/bolt/service/builtin/AmazonS3InstallationService.java
index 5aaa58b85..abe68acf2 100644
--- a/bolt/src/main/java/com/slack/api/bolt/service/builtin/AmazonS3InstallationService.java
+++ b/bolt/src/main/java/com/slack/api/bolt/service/builtin/AmazonS3InstallationService.java
@@ -7,13 +7,16 @@ import com.slack.api.bolt.model.builtin.DefaultBot;
 import com.slack.api.bolt.model.builtin.DefaultInstaller;
 import com.slack.api.bolt.service.InstallationService;
 import com.slack.api.bolt.util.JsonOps;
+import java.net.URI;
 import lombok.extern.slf4j.Slf4j;
+import org.apache.commons.lang3.StringUtils;
 import software.amazon.awssdk.auth.credentials.AwsCredentials;
 import software.amazon.awssdk.auth.credentials.AwsCredentialsProvider;
 import software.amazon.awssdk.auth.credentials.DefaultCredentialsProvider;
 import software.amazon.awssdk.core.ResponseBytes;
 import software.amazon.awssdk.core.sync.RequestBody;
 import software.amazon.awssdk.core.sync.ResponseTransformer;
+import software.amazon.awssdk.regions.Region;
 import software.amazon.awssdk.services.s3.S3Client;
 import software.amazon.awssdk.services.s3.model.*;
 
@@ -26,18 +29,44 @@ import java.util.Optional;
 public class AmazonS3InstallationService implements InstallationService {
 
     private final String bucketName;
-    private AwsCredentialsProvider credentialsProvider;
+    private final AwsCredentialsProvider credentialsProvider;
+    private final Region region;
+    private final URI endpointOverride;
     private boolean historicalDataEnabled;
 
     public AmazonS3InstallationService(String bucketName) {
         this.bucketName = bucketName;
+        this.credentialsProvider = DefaultCredentialsProvider.create();
+        this.region = Region.of(System.getenv("AWS_REGION"));
+        this.endpointOverride = null;
+    }
+
+    public AmazonS3InstallationService(String bucketName,
+        AwsCredentialsProvider credentialsProvider) {
+        this.bucketName = bucketName;
+        this.credentialsProvider = credentialsProvider;
+        this.region = Region.of(System.getenv("AWS_REGION"));
+        this.endpointOverride = null;
+    }
+
+    public AmazonS3InstallationService(String bucketName,
+        AwsCredentialsProvider credentialsProvider,
+        String region,
+        String endpointUrl) {
+        this.bucketName = bucketName;
+        this.credentialsProvider = credentialsProvider;
+        this.region = Region.of(region);
+        if (StringUtils.isBlank(endpointUrl)) {
+            this.endpointOverride = null;
+        } else {
+            this.endpointOverride = URI.create(endpointUrl);
+        }
     }
 
     @Override
     public Initializer initializer() {
         return (app) -> {
             // The first access to S3 tends to be slow on AWS Lambda.
-            this.credentialsProvider = DefaultCredentialsProvider.create();
             AwsCredentials credentials = createCredentials(credentialsProvider);
             if (credentials == null || credentials.accessKeyId() == null) {
                 throw new IllegalStateException("AWS credentials not found");
@@ -304,7 +333,11 @@ public class AmazonS3InstallationService implements InstallationService {
     }
 
     protected S3Client createS3Client() {
-        return S3Client.builder().credentialsProvider(this.credentialsProvider).build();
+        return S3Client.builder()
+            .credentialsProvider(this.credentialsProvider)
+            .region(this.region)
+            .endpointOverride(this.endpointOverride)
+            .build();
     }
 
     private String getInstallerKey(Installer i) {
@@ -330,4 +363,5 @@ public class AmazonS3InstallationService implements InstallationService {
                 + "-"
                 + Optional.ofNullable(teamId).orElse("none");
     }
+
 }

seratch · 2025-01-14T02:05:02Z

Thanks for the suggestions; I've revised this PR

bruceadowns · 2025-01-14T02:41:18Z

Thanks for the suggestions; I've revised this PR

That looks like it'll work. 👍 I'll try it out, once merged.

bolt/src/main/java/com/slack/api/bolt/service/builtin/AmazonS3InstallationService.java

WilliamBergamin · 2025-01-14T15:13:20Z

bolt/src/main/java/com/slack/api/bolt/service/builtin/AmazonS3OAuthStateService.java

+    ) {
        this.bucketName = bucketName;
+        this.credentialsProvider = credentialsProvider;
+        this.region = region != null || System.getenv("AWS_REGION") == null ? region : Region.of(System.getenv("AWS_REGION"));


bruceadowns · 2025-01-14T18:04:25Z

bolt/src/main/java/com/slack/api/bolt/service/builtin/AmazonS3InstallationService.java

        this.bucketName = bucketName;
+        this.credentialsProvider = credentialsProvider;
+        this.region = region != null || System.getenv("AWS_REGION") == null ? region : Region.of(System.getenv("AWS_REGION"));
+        this.endpointOverride = (endpointOverride != null && !endpointOverride.isEmpty()) ? URI.create(endpointOverride) : null;


Suggested change

this.endpointOverride = (endpointOverride != null && !endpointOverride.isEmpty()) ? URI.create(endpointOverride) : null;

this.endpointOverride = StringUtils.isBlank(endpointOverride) ? null : URI.create(endpointOverride);

bolt/src/main/java/com/slack/api/bolt/service/builtin/AmazonS3OAuthStateService.java

…onService

seratch added enhancement M-T: A feature request for new functionality project:bolt labels Jan 14, 2025

seratch added this to the 1.45.2 milestone Jan 14, 2025

seratch self-assigned this Jan 14, 2025

seratch requested a review from WilliamBergamin January 14, 2025 00:58

This was referenced Jan 14, 2025

Expose AwsCredentialsProvider to AmazonS3InstallationService #1418

Closed

AmazonS3InstallationService add constructor #1168

Closed

seratch force-pushed the issue-1418 branch from be1505e to a890288 Compare January 14, 2025 02:04

WilliamBergamin approved these changes Jan 14, 2025

View reviewed changes

bruceadowns reviewed Jan 14, 2025

View reviewed changes

seratch commented Jan 14, 2025

View reviewed changes

bolt/src/main/java/com/slack/api/bolt/service/builtin/AmazonS3OAuthStateService.java Outdated Show resolved Hide resolved

Fix slackapi#1418 Expose AwsCredentialsProvider to AmazonS3Installati…

b53e018

…onService

seratch force-pushed the issue-1418 branch from 28ee451 to b53e018 Compare January 14, 2025 22:25

seratch merged commit 6a7fe19 into slackapi:main Jan 15, 2025
6 checks passed

seratch deleted the issue-1418 branch January 15, 2025 01:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

Fix #1418 Expose AwsCredentialsProvider to AmazonS3InstallationService#1419

Fix #1418 Expose AwsCredentialsProvider to AmazonS3InstallationService#1419
seratch merged 1 commit intoslackapi:mainfrom
seratch:issue-1418

seratch commented Jan 14, 2025

Uh oh!

codecov bot commented Jan 14, 2025 •

edited

Loading

Uh oh!

bruceadowns commented Jan 14, 2025

Uh oh!

seratch commented Jan 14, 2025

Uh oh!

bruceadowns commented Jan 14, 2025

Uh oh!

Uh oh!

WilliamBergamin Jan 14, 2025

Uh oh!

bruceadowns Jan 14, 2025

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	this.endpointOverride = (endpointOverride != null && !endpointOverride.isEmpty()) ? URI.create(endpointOverride) : null;
	this.endpointOverride = StringUtils.isBlank(endpointOverride) ? null : URI.create(endpointOverride);

Comments

Conversation

seratch commented Jan 14, 2025

Category (place an x in each of the [ ])

Requirements

Uh oh!

codecov bot commented Jan 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bruceadowns commented Jan 14, 2025

Uh oh!

seratch commented Jan 14, 2025

Uh oh!

bruceadowns commented Jan 14, 2025

Uh oh!

Uh oh!

WilliamBergamin Jan 14, 2025

Choose a reason for hiding this comment

Uh oh!

bruceadowns Jan 14, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Category (place an `x` in each of the `[ ]`)

codecov bot commented Jan 14, 2025 •

edited

Loading