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.

Sign up for GitHub

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

Issue 374: Allow an option to choose the hashing algorithm #375

Merged
merged 2 commits into from
Jul 13, 2024
Merged

Issue 374: Allow an option to choose the hashing algorithm #375

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a230423
Issue 374: Allow an option to choose the hashing algorithm
Kpovoc Jun 25, 2024
7349650
PR 375: Suggested changes
Kpovoc Jun 26, 2024
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
3 changes: 2 additions & 1 deletion lib/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@ const DEFAULT_OPTIONS = {
createParentPath: false,
parseNested: false,
useTempFiles: false,
tempFileDir: path.join(process.cwd(), 'tmp')
tempFileDir: path.join(process.cwd(), 'tmp'),
hashAlgorithm: 'md5'
RomanBurunkov marked this conversation as resolved.
Show resolved Hide resolved
};

/**
Expand Down
2 changes: 1 addition & 1 deletion lib/memHandler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ const { debugLog } = require('./utilities');
*/
module.exports = (options, fieldname, filename) => {
const buffers = [];
const hash = crypto.createHash('md5');
const hash = crypto.createHash(options.hashAlgorithm);
let fileSize = 0;
let completed = false;

Expand Down
4 changes: 2 additions & 2 deletions lib/tempFileHandler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ module.exports = (options, fieldname, filename) => {
checkAndMakeDir({ createParentPath: true }, tempFilePath);

debugLog(options, `Temporary file path is ${tempFilePath}`);
const hash = crypto.createHash('md5');

const hash = crypto.createHash(options.hashAlgorithm);
let fileSize = 0;
let completed = false;

Expand Down
15 changes: 15 additions & 0 deletions lib/utilities.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

const fs = require('fs');
const path = require('path');
const crypto = require('crypto');
const { Readable } = require('stream');

// Parameters for safe file name parsing.
Expand Down Expand Up @@ -75,6 +76,20 @@ const buildOptions = function() {
if (!options || typeof options !== 'object') return;
Object.keys(options).forEach(i => result[i] = options[i]);
});

// Ensure a hashAlgorithm option exists.
if (!result.hashAlgorithm) {
result.hashAlgorithm = 'md5';
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would suggest to throw an error in this case, since we already have a default value and this probably a result of an error.

What do u think?

Copy link
Author

Choose a reason for hiding this comment

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

To save you time, my current commit has your suggestion implemented. Feel free to ignore the below text if that satisfies you.

You did, however, ask for my thoughts. 0:-)

I went back and forth on this.

Keep in mind, all of the text below is internal library design discussion. As written, all of these options work the same from an outside user's perspective.

What I think buildOptions should be

In my opinion, the purpose of the buildOptions function should be to provide a valid Options object to be used by the rest of the application. It should be able to take in an empty object or no object, and still provide back a valid Options object with the minimal/required properties set. It should also do some kind of value validation before the return, as the user may have provided incorrect values or value types in their argument. It should provide the default values in the scope of the function, rather than requiring them to be passed in as the 1st argument to the function.

What buildOptions currently is

Default values is not an intrinsic part of buildOptions. All it does currently is it creates a union of the provided objects, where overlapping values are overwritten by the last object in the arguments.

What buildOptions is with my changes

The same as current, but if no argument provides hashAlgorithm, we return an object that has the default value. Developer can still pass in objects that don't contain a hashAlgorithm, and we just add it to the returned options.
We throw an error if the Developer provides an unsupported value for hashAlgorithm.

What buildOptions is with the above suggestion

The same as current, but if no argument provides hashAlgorithm, we throw an error. The same behavior for if it receives an invalid hashAlgorithm.

I feel like the last one changes the intent of the function (as the current utilities tests describe it) as currently written, more than my previous implementation.

}

// Ensure the configured hashAlgorithm is available on the system
if (crypto.getHashes().find(h => result.hashAlgorithm === h) === undefined) {
throw Error(
`Hashing algorithm '${result.hashAlgorithm}' is not supported by this system's OpenSSL ` +
`version`
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the purpose of such formatting with concats?

Copy link
Author

Choose a reason for hiding this comment

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

I'm never sure if the + should be trailing, or start the new line. I moved to the new line for you.

If the question is "why did I concatenate?": It exceeded the defined linter character limit.

);
}

return result;
};

Expand Down
23 changes: 15 additions & 8 deletions test/utilities.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,25 +199,32 @@ describe('utilities: Test of the utilities functions', function() {
describe('Test buildOptions function', () => {

const source = { option1: '1', option2: '2' };
const sourceAddon = { option3: '3'};
const expected = { option1: '1', option2: '2' };
const expectedAddon = { option1: '1', option2: '2', option3: '3'};
const sourceAddon = { option3: '3', hashAlgorithm: 'sha256'};
const expected = { option1: '1', option2: '2', hashAlgorithm: 'md5' };
const expectedAddon = { option1: '1', option2: '2', option3: '3', hashAlgorithm: 'sha256'};

it('buildOptions returns and equal object to the object which was paased', () => {
let result = buildOptions(source);
assert.deepStrictEqual(result, source);
});
it(
'buildOptions returns an equal object to the object which was passed + hashAlgorithm '
+ 'property',
() => {
let result = buildOptions(source);
assert.deepStrictEqual(result, expected);
}
);

it('buildOptions doesnt add non object or null arguments to the result', () => {
let result = buildOptions(source, 2, '3', null);
assert.deepStrictEqual(result, expected);
});

it('buildOptions adds value to the result from the several source argumets', () => {
it('buildOptions adds value to the result from the several source arguments', () => {
let result = buildOptions(source, sourceAddon);
assert.deepStrictEqual(result, expectedAddon);
});

it('buildOptions throws an error when given an unsupported hashAlgorithm', () => {
assert.throws(() => buildOptions({ hashAlgorithm: 'not-actual-algo' }));
});
});
//buildFields tests
describe('Test buildFields function', () => {
Expand Down