Skip to content
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

feat: Allow File adapter to create file with specific locations or dynamic filenames #9557

Open
wants to merge 19 commits into
base: alpha
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions spec/CloudCode.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -3695,11 +3695,13 @@ describe('saveFile hooks', () => {
foo: 'bar',
},
};
const config = Config.get('test');
expect(createFileSpy).toHaveBeenCalledWith(
jasmine.any(String),
newData,
'text/plain',
newOptions
newOptions,
config
);
});

Expand Down Expand Up @@ -3727,11 +3729,13 @@ describe('saveFile hooks', () => {
foo: 'bar',
},
};
const config = Config.get('test');
expect(createFileSpy).toHaveBeenCalledWith(
jasmine.any(String),
newData,
newContentType,
newOptions
newOptions,
config
);
const expectedFileName = 'donald_duck.pdf';
expect(file._name.indexOf(expectedFileName)).toBe(file._name.length - expectedFileName.length);
Expand All @@ -3757,11 +3761,13 @@ describe('saveFile hooks', () => {
metadata: { foo: 'bar' },
tags: { bar: 'foo' },
};
const config = Config.get('test');
expect(createFileSpy).toHaveBeenCalledWith(
jasmine.any(String),
jasmine.any(Buffer),
'text/plain',
options
options,
config
);
});

Expand Down
95 changes: 95 additions & 0 deletions spec/FilesController.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -218,4 +218,99 @@ describe('FilesController', () => {
expect(gridFSAdapter.validateFilename(fileName)).not.toBe(null);
done();
});

it('should return valid filename or url from createFile response when provided', async () => {
const config = Config.get(Parse.applicationId);

// Test case 1: adapter returns new filename and url
Copy link
Member

Choose a reason for hiding this comment

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

Could you please split this into separate tests, each one with a distinct title to describe what it's testing.

Copy link
Author

Choose a reason for hiding this comment

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

Sure

Copy link
Member

Choose a reason for hiding this comment

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

You also may want to take a look at the failing tests in the CI

const adapterWithReturn = { ...mockAdapter };
adapterWithReturn.createFile = () => {
return Promise.resolve({
name: 'newFilename.txt',
url: 'http://new.url/newFilename.txt'
});
};
adapterWithReturn.getFileLocation = () => {
return Promise.resolve('http://default.url/file.txt');
};
const controllerWithReturn = new FilesController(adapterWithReturn, null, { preserveFileName: true });
// preserveFileName is true to make filename behaviors predictable
const result1 = await controllerWithReturn.createFile(
config,
'originalFile.txt',
'data',
'text/plain'
);
expect(result1.name).toBe('newFilename.txt');
expect(result1.url).toBe('http://new.url/newFilename.txt');

// Test case 2: adapter returns nothing, falling back to default behavior
const adapterWithoutReturn = { ...mockAdapter };
adapterWithoutReturn.createFile = () => {
return Promise.resolve();
};
adapterWithoutReturn.getFileLocation = (config, filename) => {
return Promise.resolve(`http://default.url/${filename}`);
};

const controllerWithoutReturn = new FilesController(adapterWithoutReturn, null, { preserveFileName: true });
const result2 = await controllerWithoutReturn.createFile(
config,
'originalFile.txt',
'data',
'text/plain',
{}
);

expect(result2.name).toBe('originalFile.txt');
expect(result2.url).toBe('http://default.url/originalFile.txt');

// Test case 3: adapter returns partial info (only url)
// This is a valid scenario, as the adapter may return a modified filename
// but may result in a mismatch between the filename and the resource URL
const adapterWithOnlyURL = { ...mockAdapter };
adapterWithOnlyURL.createFile = () => {
return Promise.resolve({
url: 'http://new.url/partialFile.txt'
});
};
adapterWithOnlyURL.getFileLocation = () => {
return Promise.resolve('http://default.url/file.txt');
};

const controllerWithPartial = new FilesController(adapterWithOnlyURL, null, { preserveFileName: true });
const result3 = await controllerWithPartial.createFile(
config,
'originalFile.txt',
'data',
'text/plain',
{}
);

expect(result3.name).toBe('originalFile.txt');
expect(result3.url).toBe('http://new.url/partialFile.txt'); // Technically, the resource does not need to match the filename

// Test case 4: adapter returns only filename
const adapterWithOnlyFilename = { ...mockAdapter };
adapterWithOnlyFilename.createFile = () => {
return Promise.resolve({
name: 'newname.txt'
});
};
adapterWithOnlyFilename.getFileLocation = (config, filename) => {
return Promise.resolve(`http://default.url/${filename}`);
};

const controllerWithOnlyFilename = new FilesController(adapterWithOnlyFilename, null, { preserveFileName: true });
const result4 = await controllerWithOnlyFilename.createFile(
config,
'originalFile.txt',
'data',
'text/plain',
{}
);

expect(result4.name).toBe('newname.txt');
expect(result4.url).toBe('http://default.url/newname.txt');
});
});
8 changes: 5 additions & 3 deletions src/Adapters/Files/FilesAdapter.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,14 @@ export class FilesAdapter {
* @discussion the contentType can be undefined if the controller was not able to determine it
* @param {object} options - (Optional) options to be passed to file adapter (S3 File Adapter Only)
* - tags: object containing key value pairs that will be stored with file
* - metadata: object containing key value pairs that will be sotred with file (https://docs.aws.amazon.com/AmazonS3/latest/user-guide/add-object-metadata.html)
* - metadata: object containing key value pairs that will be stored with file (https://docs.aws.amazon.com/AmazonS3/latest/user-guide/add-object-metadata.html)
* @discussion options are not supported by all file adapters. Check the your adapter's documentation for compatibility
* @param {Config} config - (Optional) server configuration
Copy link
Member

Choose a reason for hiding this comment

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

Why would the whole Parse Server config be passed into the adapter? Or am I misreading this?

Copy link
Author

Choose a reason for hiding this comment

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

@mtrezza
Allowing the server config to be passed to the createFile would allow a couple of things.

  1. files can be named based on serer attributes (ex: generateKey could store log files with the app id)
  2. getFileLocation can be called within createFile, either to test or check that the file has been created (especially if getFileLocation depends on the server config which it always has access to)

There may be other situations in which the server config may be useful apart from customizing file paths and running getFileLocation, but I don't see why createFile shouldn't have access to it if getFileLocation does.

If this was a smaller project I would probably have refactored adapter.createFile to use config as the first argument, but that had the potential to be fairly breaking and maybe its nicer for some people to use X.createFile('filename')

In any case tacking it at the end was the compromise I made.

In the current S3-adapter PR, this only allows getFileLocation to run inside and I didn't pass the parameters to generateKey.

I would say though that removing that expectation and removing getFileLocation from the adapter would make the changes more explicit, but then [location] = createFile would not always match the call from getFileLocation (if it depended in any way on server parameters)

* @discussion config may be passed to adapter to allow for more complex configuration and internal call of getFileLocation (if needed). This argument is not supported by all file adapters. Check the your adapter's documentation for compatibility
*
* @return {Promise} a promise that should fail if the storage didn't succeed
* @return {Promise<{url?: string, name?: string, location?: string}>|Promise<undefined>} Either a plain promise that should fail if storage didn't succeed, or a promise resolving to an object containing url and/or an updated filename and/or location (if relevant)
*/
createFile(filename: string, data, contentType: string, options: Object): Promise {}
createFile(filename: string, data, contentType: string, options: Object, config: Config): Promise {}

/** Responsible for deleting the specified file
*
Expand Down
9 changes: 6 additions & 3 deletions src/Controllers/FilesController.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,13 @@ export class FilesController extends AdaptableController {
filename = randomHexString(32) + '_' + filename;
}

const location = await this.adapter.getFileLocation(config, filename);
await this.adapter.createFile(filename, data, contentType, options);
const createResult = await this.adapter.createFile(filename, data, contentType, options, config);
filename = createResult?.name || filename; // if createFile returns a new filename, use it

const url = createResult?.url || await this.adapter.getFileLocation(config, filename); // if createFile returns a new url, use it otherwise get the url from the adapter

return {
url: location,
url: url,
name: filename,
}
}
Expand Down
Loading