feat: s3 compatible storage by VidocqH · Pull Request #15 · xavia-io/xavia-ota

VidocqH · 2025-04-02T23:50:33Z

Description

Support S3 compatible storage

Type of change

Bug fix (non-breaking change which fixes an issue)
Documentation update
Refactoring (no functional changes)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Testing

Modify fs mock as aws-sdk breaks it.
Related issue:
aws/aws-sdk-js-v3#3547 (comment)

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings

Related issue: aws/aws-sdk-js-v3#3547 (comment)

nilpntr · 2025-05-04T14:06:28Z

@VidocqH still working on this?

VidocqH · 2025-05-06T12:35:39Z

@VidocqH still working on this?

I tested with Cloudflare R2, this PR is working correctly.

Cyberistic · 2025-05-24T21:12:18Z

Can we get this merged?

remyoudemans

I have no authority over whether this PR gets merged, but left a couple comments 😄

remyoudemans · 2025-06-02T12:26:17Z

+  constructor() {
+    if (!process.env.S3_ACCESS_KEY_ID || !process.env.S3_SECRET_ACCESS_KEY) {
+      throw new Error('S3 credentials not configured');
+    }
+    if (!process.env.S3_BUCKET_NAME) {
+      throw new Error('S3 bucket name not configured');
+    }
+    this.client = new S3Client({
+      region: process.env.S3_REGION ?? 'auto',
+      endpoint: process.env.S3_ENDPOINT,
+      credentials: {
+        accessKeyId: process.env.S3_ACCESS_KEY_ID,
+        secretAccessKey: process.env.S3_SECRET_ACCESS_KEY,
+      },
+    });
+    this.bucketName = process.env.S3_BUCKET_NAME;
+  }


Ideally this would also work with IAM roles where the AWS SDK automatically handles fetching credentials (this is how I would need to use it).

constructor() { if (!process.env.S3_ACCESS_KEY_ID || !process.env.S3_SECRET_ACCESS_KEY) { console.warn('S3 credentials not configured. AWS will fetch temporary credentials based on the IAM role.'); } if (!process.env.S3_BUCKET_NAME) { throw new Error('S3 bucket name not configured'); } this.client = new S3Client({ region: process.env.S3_REGION ?? 'auto', endpoint: process.env.S3_ENDPOINT, credentials: process.env.S3_ACCESS_KEY_ID && process.env.S3_SECRET_ACCESS_KEY ? { accessKeyId: process.env.S3_ACCESS_KEY_ID, secretAccessKey: process.env.S3_SECRET_ACCESS_KEY, } : undefined, }); this.bucketName = process.env.S3_BUCKET_NAME; }

remyoudemans · 2025-06-02T12:29:15Z

Could be handy to have some logging in here. There's quite a lot of logging in the GCS equivalent.

That GCS one just uses console log, but there is also a logger in this repo, as used here

MoElkhidir · 2025-06-02T16:21:16Z

@remyoudemans would you like to become a contributor? I have been so busy recently with the started I couldn't manage to review the contributions on a frequent basis unfortunately :/

remyoudemans · 2025-06-04T08:17:17Z

@remyoudemans would you like to become a contributor? I have been so busy recently with the started I couldn't manage to review the contributions on a frequent basis unfortunately :/

I can review some PRs for sure. I'm pretty fastidious 😄
I'm not likely to do more than that though, as I'm not currently invested in this. Still hoping to use Expo, but have to see if the business case goes through. It probably will, but if it doesn't I could get more involved.

MoElkhidir · 2025-06-16T08:06:34Z

@remyoudemans would you like to become a contributor? I have been so busy recently with the started I couldn't manage to review the contributions on a frequent basis unfortunately :/

I can review some PRs for sure. I'm pretty fastidious 😄 I'm not likely to do more than that though, as I'm not currently invested in this. Still hoping to use Expo, but have to see if the business case goes through. It probably will, but if it doesn't I could get more involved.

Reviewing would be more than what I can ask for currently ^^ Thank you and welcome aboard <3

Copilot

Pull Request Overview

This PR adds support for AWS S3 compatible storage as a new storage backend option. It allows the application to work with any S3-compatible storage service including AWS S3, Digital Ocean Spaces, and Cloudflare R2.

Implements S3Storage class with full StorageInterface compliance
Adds S3 configuration documentation and environment variables
Modifies test mocking to be compatible with aws-sdk

Reviewed Changes

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

Show a summary per file

File	Description
package.json	Adds @aws-sdk/client-s3 dependency
docs/supportedStorageAlternatives.md	Documents S3 storage configuration and environment variables
apiUtils/storage/StorageFactory.ts	Adds S3 storage option to factory pattern
apiUtils/storage/S3Storage.ts	Implements complete S3Storage class with all required methods
tests/upload.test.ts	Updates fs mocking to be compatible with aws-sdk
README.md	Updates supported storage providers list

Comments suppressed due to low confidence (1)

package.json:16

The version 3.779.0 of @aws-sdk/client-s3 appears to be very high. The latest stable version as of my knowledge cutoff was around 3.400.x. Please verify this version exists.

    "@aws-sdk/client-s3": "^3.779.0",

Copilot · 2025-08-04T14:53:52Z

+  async copyFile(sourcePath: string, destinationPath: string): Promise<void> {
+    const copyCommand = new CopyObjectCommand({
+      Bucket: this.bucketName,
+      CopySource: sourcePath,


The CopySource parameter should include the bucket name. It should be ${this.bucketName}/${sourcePath} instead of just sourcePath.

Suggested change

CopySource: sourcePath,

CopySource: `${this.bucketName}/${sourcePath}`,

Copilot · 2025-08-04T14:53:53Z

+    try {
+      const headCommand = new HeadObjectCommand({
+        Bucket: this.bucketName,
+        Key: path.split('/').shift(),


Using shift() returns the first part of the path, but for file existence check you should use the full path. This should be Key: path instead.

Suggested change

Key: path.split('/').shift(),

Key: path,

Copilot · 2025-08-04T14:53:53Z

+
+  async fileExists(path: string): Promise<boolean> {
+    try {
+      const files = await this.listDirectories(path);


The fileExists method is checking for directories first, but this is inefficient and incorrect logic. A file path should not be checked as a directory. This will cause incorrect behavior when checking for actual files.

remyoudemans · 2025-08-04T15:03:17Z

Sorry it has taken me so long to come back to this. The PR looks good, but to be honest I haven't worked with S3 so I can't tell if there might be a lot of edge cases or issues. All I can really confirm is that the code doesn't look malicious, so that's a start 😄 .
I got Copilot to review it and it left some comments.

@VidocqH would you mind taking a look through Copilot's comments and mine?
I know this PR has been a long time coming and it's a highly requested feature, so if it works well enough and Copilot's comments aren't all that relevant, let me know and I'll approve it and make a separate issue for the other method of AWS authentication I mentioned.

@nilpntr and @Cyberistic, have either of you tried this out?

MoElkhidir · 2025-09-27T17:35:39Z

LGTM! Thank you all for your contribution to this!

Cyberistic · 2025-09-27T20:46:02Z

@nilpntr and @Cyberistic, have either of you tried this out?

Sorry for not answering earlier, I wasn’t pinged and randomly stumbled into the same issue while browsing latest solutions.

Seems like it’s working.

VidocqH added 3 commits April 3, 2025 07:34

feat: support S3 storage

ac05430

fix(test): fs mock issue

f93e2ce

Related issue: aws/aws-sdk-js-v3#3547 (comment)

docs: s3 compatible storage

310e545

remyoudemans reviewed Jun 2, 2025

View reviewed changes

remyoudemans requested a review from Copilot August 4, 2025 14:53

Merge branch 'main' into feat/s3-storage

5910701

Copilot AI reviewed Aug 4, 2025

View reviewed changes

remyoudemans and others added 2 commits August 7, 2025 11:45

Merge branch 'main' into feat/s3-storage

63519ab

Merge branch 'main' into feat/s3-storage

cb70329

MoElkhidir merged commit 53007f1 into xavia-io:main Sep 27, 2025
1 check passed

	CopySource: sourcePath,
	CopySource: `${this.bucketName}/${sourcePath}`,

Conversation

VidocqH commented Apr 2, 2025

Description

Type of change

Testing

Checklist

Uh oh!

nilpntr commented May 4, 2025

Uh oh!

VidocqH commented May 6, 2025

Uh oh!

Cyberistic commented May 24, 2025

Uh oh!

remyoudemans left a comment

Choose a reason for hiding this comment

Uh oh!

remyoudemans Jun 2, 2025

Choose a reason for hiding this comment

Uh oh!

remyoudemans Jun 2, 2025

Choose a reason for hiding this comment

Uh oh!

MoElkhidir commented Jun 2, 2025

Uh oh!

remyoudemans commented Jun 4, 2025

Uh oh!

MoElkhidir commented Jun 16, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Aug 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 4, 2025

Choose a reason for hiding this comment

Uh oh!

remyoudemans commented Aug 4, 2025

Uh oh!

MoElkhidir commented Sep 27, 2025

Uh oh!

Uh oh!

Cyberistic commented Sep 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants