add static factory for custom S3 endpoint in S3ObjectMonitor #171
Conversation
| Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward? This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
| @alexsunday Apologies for the delay here; let's reopen and review this. |
| Perhaps the original intention of Signal's open-source release was to enable code review for verifying its security, but in reality, very few people independently follow up and deploy the latest version of the server software? These changes are critical for self-deployment, while maintaining excessive patches and branches independently becomes overly complex. Therefore, I hope they can be merged. Thank you. |
| public S3ObjectMonitor build(final AwsCredentialsProvider awsCredentialsProvider, | ||
| final ScheduledExecutorService refreshExecutorService) { | ||
| | ||
| if (endpointOverride != null && !endpointOverride.toString().isEmpty()) { |
There was a problem hiding this comment.
| if (endpointOverride != null && !endpointOverride.toString().isEmpty()) { | |
| if (StringUtils.isNotBlank(endpointOverride)) { |
There was a problem hiding this comment.
thanks for the suggestion.
I agree the current code is a bit verbose. However, endpointOverride is a @Nullable URI object, not a String. StringUtils.isNotBlank(endpointOverride.toString()) would throw a NullPointerException if endpointOverride is null.
I prefer to keep its type as URI to stay consistent with other similar configurations and to leverage the framework's built-in validation for the URI type. Therefore, the current null check is the safest approach here.
| // allows specifying a custom S3 endpoint | ||
| static public S3ObjectMonitor createCustomS3(final AwsCredentialsProvider awsCredentialsProvider, |
There was a problem hiding this comment.
It seems like we're mixing metaphors here; this would mean we have a mix of constructors and static create methods. I think we should stick to one or the other.
Although reasonable minds might disagree, I'd suggest sticking with overloaded constructors since that's what's already happening here (even if it is only for a visible-for-testing constructor). Specifically, I'd introduce a second public constructor that accepts an s3Endpoint argument, then have the last (package-private) constructor in the chain conditionally modify the S3ClientBuilder depending on the presence of the s3Endpoint argument.
There was a problem hiding this comment.
It has been changed to use the overloaded constructor style. Please review it again.
| How do I submit code to your repository to add support for supergroups and superchannels? |
3 participants
This change add a static method createCustomS3 to S3ObjectMonitor, allowing the creation of S3 clients with custom endpoints. This is useful for development and testing environments, enabling Signal to interact with local or non-standard S3-compatible storage. The method supports endpoint override and path-style access, making it easier to run integration tests without relying on AWS infrastructure.