Skip to content

Conversation

@boris-w
Copy link
Contributor

@boris-w boris-w commented Oct 21, 2025

No description provided.

@boris-w boris-w requested review from Copilot and tea-artist October 21, 2025 10:23
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds support for custom domain endpoints for S3 private buckets, allowing URLs to be rewritten to use a configured custom domain instead of the default S3 endpoint.

  • Introduces a new configuration parameter privateBucketEndpoint for custom domain support
  • Implements URL replacement logic to swap S3 endpoints with custom domains
  • Updates internal operations to consistently use s3ClientPrivateNetwork instead of s3Client

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File Description
apps/nestjs-backend/src/features/attachments/plugins/s3.ts Adds replaceBucketEndpoint method, applies URL replacement to presigned URLs, switches internal S3 operations to use private network client, adds debug logging
apps/nestjs-backend/src/configs/storage.ts Adds privateBucketEndpoint configuration parameter from environment variable

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

private replaceBucketEndpoint(url: string, bucket: string): string {
const { privateBucketEndpoint, privateBucket } = this.config;
if (privateBucketEndpoint && bucket === privateBucket) {
const resUrl = new URL(url, privateBucketEndpoint);
Copy link

Copilot AI Oct 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The second parameter of URL constructor is intended to be a base URL for relative paths. However, url is already an absolute URL from getSignedUrl. This will cause the privateBucketEndpoint to be ignored. Remove the second parameter and create two separate URL objects.

Suggested change
const resUrl = new URL(url, privateBucketEndpoint);
const resUrl = new URL(url);

Copilot uses AI. Check for mistakes.
};
// eslint-disable-next-line @typescript-eslint/no-explicit-any
} catch (e: any) {
console.log('S3 presigned error', e);
Copy link

Copilot AI Oct 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debug console.log should be replaced with proper logging using a logger service. Console logs are not appropriate for production code and don't support log levels or structured logging.

Copilot uses AI. Check for mistakes.
});
return this.replaceBucketEndpoint(res, bucket);
}
uploadFileWidthPath(
Copy link

Copilot AI Oct 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Corrected spelling of 'Width' to 'With' in method name.

Copilot uses AI. Check for mistakes.
filePath: string,
metadata: Record<string, unknown>
) {
console.log('start uploadFileWidthPath', bucket, path, filePath, metadata);
Copy link

Copilot AI Oct 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debug console.log should be replaced with proper logging using a logger service. Console logs are not appropriate for production code and don't support log levels or structured logging.

Copilot uses AI. Check for mistakes.
.finally(() => {
readStream.removeAllListeners();
readStream.destroy();
console.log('end uploadFileWidthPath');
Copy link

Copilot AI Oct 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debug console.log should be replaced with proper logging using a logger service. Console logs are not appropriate for production code and don't support log levels or structured logging.

Copilot uses AI. Check for mistakes.
publicUrl: process.env.BACKEND_STORAGE_PUBLIC_URL,
publicBucket: process.env.BACKEND_STORAGE_PUBLIC_BUCKET || 'public',
privateBucket: process.env.BACKEND_STORAGE_PRIVATE_BUCKET || 'private',
privateBucketEndpoint: process.env.BACKEND_STORAGE_PRIVATE_BUCKET_ENDPOINT,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add example and doc for this .env

@coveralls
Copy link
Collaborator

coveralls commented Oct 21, 2025

Pull Request Test Coverage Report for Build 18744952466

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 7 of 134 (5.22%) changed or added relevant lines in 4 files are covered.
  • 306 unchanged lines in 8 files lost coverage.
  • Overall coverage decreased (-0.006%) to 75.084%

Changes Missing Coverage Covered Lines Changed/Added Lines %
apps/nestjs-backend/src/features/attachments/plugins/storage.ts 2 3 66.67%
apps/nestjs-backend/src/features/attachments/plugins/aliyun.ts 3 53 5.66%
apps/nestjs-backend/src/features/attachments/plugins/s3.ts 0 76 0.0%
Files with Coverage Reduction New Missed Lines %
apps/nestjs-backend/src/features/attachments/plugins/s3.ts 1 0.79%
apps/nestjs-backend/src/features/record/computed/services/computed-dependency-collector.service.ts 6 94.25%
apps/nestjs-backend/src/features/record/query-builder/providers/pg-record-query-dialect.ts 7 88.28%
apps/nestjs-backend/src/features/data-loader/resource/table-common-loader.ts 9 35.0%
apps/nestjs-backend/src/features/view/view.service.ts 29 91.24%
apps/nestjs-backend/src/features/record/query-builder/field-cte-visitor.ts 41 75.86%
apps/nestjs-backend/src/features/view/open-api/view-open-api.service.ts 82 76.86%
apps/nestjs-backend/src/features/record/record.service.ts 131 88.18%
Totals Coverage Status
Change from base Build 18680464543: -0.006%
Covered Lines: 50358
Relevant Lines: 67069

💛 - Coveralls

@boris-w boris-w requested a review from Copilot October 22, 2025 09:32
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@boris-w boris-w requested a review from Copilot October 22, 2025 10:09
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@boris-w boris-w requested a review from Copilot October 23, 2025 08:55
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@boris-w boris-w merged commit 09fe334 into develop Oct 23, 2025
13 checks passed
@boris-w boris-w deleted the feat/s3-support-custom-domain branch October 23, 2025 13:04
@github-actions
Copy link

🧹 Preview Environment Cleanup

tea-artist pushed a commit that referenced this pull request Nov 13, 2025
* feat: add  support s3 custom domain

* chore: remove log code and env example

* refactor: use AWS official method for S3 custom domain

* feat: support aliyun oss

* feat: debug s3 sockets connection count

* chore: safe isNaN
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants