trim & test key after generation & assmble endpoint accordingly

More tests and changes to make coderabbit happy

Author: AdrianCurtin

index.js

@@ -146,9 +146,10 @@ class S3Adapter {
       const candidate = this._generateKey(filename, contentType, options);
       key_without_prefix =
         candidate && typeof candidate.then === 'function' ? await candidate : candidate;
-      if (typeof key_without_prefix !== 'string' || key_without_prefix.length === 0) {
+      if (typeof key_without_prefix !== 'string' || key_without_prefix.trim().length === 0) {
         throw new Error('generateKey must return a non-empty string');
       }
+      key_without_prefix = key_without_prefix.trim();
     }
 
     const params = {
@@ -185,8 +186,27 @@ class S3Adapter {
     await this.createBucket();
     const command = new PutObjectCommand(params);
     await this._s3Client.send(command);
-    const endpoint = this._endpoint || `https://${this._bucket}.s3.${this._region}.amazonaws.com`;
-    const location = `${endpoint}/${params.Key}`;
+    
+    let locationBase;
+    if (this._endpoint) {
+      try {
+        const u = new URL(this._endpoint);
+        const origin = `${u.protocol}//${u.host}`;
+        const basePath = (u.pathname || '').replace(/\/$/, '');
+        const hasBucketInHostOrPath =
+          u.hostname.startsWith(`${this._bucket}.`) ||
+          basePath.split('/').includes(this._bucket);
+        const pathWithBucket = hasBucketInHostOrPath ? basePath : `${basePath}/${this._bucket}`;
+        locationBase = `${origin}${pathWithBucket}`;
+      } catch {
+        // Fallback for non-URL endpoints (assume hostname)
+        locationBase = `https://${String(this._endpoint).replace(/\/$/, '')}/${this._bucket}`;
+      }
+    } else {
+      const regionPart = this._region ? `.s3.${this._region}` : '.s3';
+      locationBase = `https://${this._bucket}${regionPart}.amazonaws.com`;
+    }
+    const location = `${locationBase}/${params.Key}`;
 
     let url;
     if (config?.mount && config?.applicationId) { // if config has required properties for getFileLocation

spec/test.spec.js

@@ -894,6 +894,10 @@ describe('S3Adapter tests', () => {
         { mount: 'http://example.com', applicationId: 'test123' }
       );
 
+      expect(s3.getFileLocation).toHaveBeenCalledWith(
+        jasmine.objectContaining({ mount: 'http://example.com', applicationId: 'test123' }),
+        'file.txt'
+      );
       expect(result).toEqual({
         location: jasmine.any(String),
         name: 'file.txt',
@@ -929,13 +933,14 @@ describe('S3Adapter tests', () => {
       s3ClientMock.send.and.returnValue(Promise.resolve({}));
       s3._s3Client = s3ClientMock;
 
-      await s3.createFile('file.txt', 'hello world', 'text/utf8', {});
+      const out = await s3.createFile('file.txt', 'hello world', 'text/utf8', {});
 
       expect(s3ClientMock.send).toHaveBeenCalledTimes(2);
       const commands = s3ClientMock.send.calls.all();
       const commandArg = commands[1].args[0];
       expect(commandArg).toBeInstanceOf(PutObjectCommand);
       expect(commandArg.input.Key).toBe('async-file.txt');
+      expect(out.name).toBe('async-file.txt');
     });
 
     it('should handle generateKey that returns a Promise', async () => {
@@ -949,13 +954,14 @@ describe('S3Adapter tests', () => {
       s3ClientMock.send.and.returnValue(Promise.resolve({}));
       s3._s3Client = s3ClientMock;
 
-      await s3.createFile('file.txt', 'hello world', 'text/utf8', {});
+      const out = await s3.createFile('file.txt', 'hello world', 'text/utf8', {});
 
       expect(s3ClientMock.send).toHaveBeenCalledTimes(2);
       const commands = s3ClientMock.send.calls.all();
       const commandArg = commands[1].args[0];
       expect(commandArg).toBeInstanceOf(PutObjectCommand);
       expect(commandArg.input.Key).toBe('promise-file.txt');
+      expect(out.name).toBe('promise-file.txt');
     });
 
     it('should validate generateKey returns a non-empty string', async () => {
@@ -984,6 +990,19 @@ describe('S3Adapter tests', () => {
       ).toBeRejectedWithError('generateKey must return a non-empty string');
     });
 
+    it('should reject when generateKey returns only whitespace', async () => {
+      const options = {
+        bucket: 'bucket-1',
+        generateKey: () => '   '
+      };
+      const s3 = new S3Adapter(options);
+      s3._s3Client = s3ClientMock;
+
+      await expectAsync(
+        s3.createFile('file.txt', 'hello world', 'text/utf8', {})
+      ).toBeRejectedWithError('generateKey must return a non-empty string');
+    });
+
     it('should validate async generateKey returns a string', async () => {
       const options = {
         bucket: 'bucket-1',
@@ -998,6 +1017,88 @@ describe('S3Adapter tests', () => {
     });
   });
 
+  describe('URL construction with custom endpoints', () => {
+    let s3ClientMock;
+
+    beforeEach(() => {
+      s3ClientMock = jasmine.createSpyObj('S3Client', ['send']);
+      s3ClientMock.send.and.returnValue(Promise.resolve({}));
+    });
+
+    it('should handle endpoint with path and query correctly', async () => {
+      const s3 = new S3Adapter({
+        bucket: 'test-bucket',
+        s3overrides: {
+          endpoint: 'https://example.com:8080/path?foo=bar'
+        }
+      });
+      s3._s3Client = s3ClientMock;
+
+      const result = await s3.createFile('test.txt', 'hello world', 'text/utf8');
+
+      // Should construct proper URL without breaking query parameters
+      expect(result.location).toBe('https://example.com:8080/path/test-bucket/test.txt');
+    });
+
+    it('should handle path-style endpoint without bucket in hostname', async () => {
+      const s3 = new S3Adapter({
+        bucket: 'test-bucket',
+        s3overrides: {
+          endpoint: 'https://minio.example.com'
+        }
+      });
+      s3._s3Client = s3ClientMock;
+
+      const result = await s3.createFile('test.txt', 'hello world', 'text/utf8');
+
+      // Should add bucket to path for path-style
+      expect(result.location).toBe('https://minio.example.com/test-bucket/test.txt');
+    });
+
+    it('should handle virtual-hosted-style endpoint with bucket in hostname', async () => {
+      const s3 = new S3Adapter({
+        bucket: 'test-bucket',
+        s3overrides: {
+          endpoint: 'https://test-bucket.s3.example.com'
+        }
+      });
+      s3._s3Client = s3ClientMock;
+
+      const result = await s3.createFile('test.txt', 'hello world', 'text/utf8');
+
+      // Should not duplicate bucket when it's already in hostname
+      expect(result.location).toBe('https://test-bucket.s3.example.com/test.txt');
+    });
+
+    it('should fallback for malformed endpoint', async () => {
+      const s3 = new S3Adapter({
+        bucket: 'test-bucket',
+        s3overrides: {
+          endpoint: 'not-a-valid-url'
+        }
+      });
+      s3._s3Client = s3ClientMock;
+
+      const result = await s3.createFile('test.txt', 'hello world', 'text/utf8');
+
+      // Should fallback to safe construction
+      expect(result.location).toBe('https://not-a-valid-url/test-bucket/test.txt');
+    });
+
+    it('should use default AWS endpoint when no custom endpoint', async () => {
+      const s3 = new S3Adapter({
+        bucket: 'test-bucket',
+        region: 'us-west-2'
+      });
+      s3._s3Client = s3ClientMock;
+
+      const result = await s3.createFile('test.txt', 'hello world', 'text/utf8');
+
+      // Should use standard AWS S3 URL
+      expect(result.location).toBe('https://test-bucket.s3.us-west-2.amazonaws.com/test.txt');
+    });
+  });
+
   describe('handleFileStream', () => {
     const filename = 'file.txt';
     let s3;