-
-
Notifications
You must be signed in to change notification settings - Fork 87
feat: Revise createFile logic to return modified filenames and location #242
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
3172564
be61240
d8c1e15
f3f685b
fb9c42e
394e175
9d760be
84e11a5
7afb0a2
f5616d9
3ba5566
227c7cf
6a3b0cf
b7d124f
bf88785
2cac314
c78165a
d87fc64
05a056a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -140,16 +140,24 @@ class S3Adapter { | |
|
||
// For a given config object, filename, and data, store a file in S3 | ||
// Returns a promise containing the S3 object creation response | ||
async createFile(filename, data, contentType, options = {}) { | ||
async createFile(filename, data, contentType, options = {}, config = {}) { | ||
let keyWithoutPrefix = filename; | ||
if (typeof this._generateKey === 'function') { | ||
const candidate = this._generateKey(filename, contentType, options); | ||
keyWithoutPrefix = | ||
candidate && typeof candidate.then === 'function' ? await candidate : candidate; | ||
if (typeof keyWithoutPrefix !== 'string' || keyWithoutPrefix.trim().length === 0) { | ||
throw new Error('generateKey must return a non-empty string'); | ||
} | ||
keyWithoutPrefix = keyWithoutPrefix.trim(); | ||
} | ||
|
||
const params = { | ||
Bucket: this._bucket, | ||
Key: this._bucketPrefix + filename, | ||
Key: this._bucketPrefix + keyWithoutPrefix, | ||
Body: data, | ||
}; | ||
|
||
if (this._generateKey instanceof Function) { | ||
params.Key = this._bucketPrefix + this._generateKey(filename); | ||
} | ||
if (this._fileAcl) { | ||
if (this._fileAcl === 'none') { | ||
delete params.ACL; | ||
|
@@ -177,11 +185,39 @@ class S3Adapter { | |
} | ||
await this.createBucket(); | ||
const command = new PutObjectCommand(params); | ||
const response = await this._s3Client.send(command); | ||
const endpoint = this._endpoint || `https://${this._bucket}.s3.${this._region}.amazonaws.com`; | ||
const location = `${endpoint}/${params.Key}`; | ||
await this._s3Client.send(command); | ||
|
||
return Object.assign(response || {}, { Location: location }); | ||
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}`; | ||
Comment on lines
+199
to
+200
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: is it valid to redirect a "domain" bucket to a "path" bucket ? not sure all S3 providers support this switch We need to be sure that current system works with all S3 providers like DO Spaces, Google Storage with S3, Minio S3 ... Did you test locally with this kind of providers to ensure S3 protocol compatibility ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we only need to test with S3, which is the spec others should follow when claiming S3 compatibility. However, it must be explicitly documented in the S3 specs, otherwise we may use a behavior that is not properly defined even for S3. If in doubt, ask AWS support. |
||
} 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 | ||
url = await this.getFileLocation(config, keyWithoutPrefix); | ||
} | ||
|
||
return { | ||
location: location, // actual upload location, used for tests | ||
name: keyWithoutPrefix, // filename in storage, consistent with other adapters | ||
...url ? { url: url } : {} // url (optionally presigned) or non-direct access url | ||
}; | ||
} | ||
|
||
async deleteFile(filename) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: i can suggestion to add comments with an url example to show at each line what you replace and why, it will help for the maintenance in the long term
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure