Issue 374: Allow an option to choose the hashing algorithm #375
Conversation
- hashAlgorithm key added to DEFAULT_OPTIONS
- hashAlgorithm key can be overriden by value given in options argument
- buildOptions function will ensure a default a hashAlgorithm exists,
and throw an error if the provided option is not supported by the
system.
- throws early error during app setup, rather than waiting
until an upload is attempted
- memHandler and tempFileHandler use the hashAlgorithm option now
- Updated tests for buildOptions function in utilities.spec.js
ref: richardgirges#374
lib/utilities.js
Outdated
|
|
||
| // Ensure a hashAlgorithm option exists. | ||
| if (!result.hashAlgorithm) { | ||
| result.hashAlgorithm = 'md5'; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
lib/utilities.js
Outdated
| if (crypto.getHashes().find(h => result.hashAlgorithm === h) === undefined) { | ||
| throw Error( | ||
| `Hashing algorithm '${result.hashAlgorithm}' is not supported by this system's OpenSSL ` + | ||
| `version` |
There was a problem hiding this comment.
What is the purpose of such formatting with concats?
There was a problem hiding this comment.
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.
- `buildOptions` now throws an error if `hashAlgorithm` is not defined, instead of resetting to the default value of 'md5' - Moved + symbol for a string concatination to the beginning of the next line - Updated the README.md with information on the new hashAlgorithm option, and how the `md5` property is defined in the new version - Iterated the version in package.json from 1.5.0 -> 1.5.1 ref: richardgirges#375
|
Any chance to get this merged and released soon 🙏🏼 ? |
|
@RomanBurunkov @Kpovoc Gentle ping 👀 |
|
@barucoh Yeah, I poked him on the linked issue. Waiting on either a merge of feedback for further changes. |
|
Sorry for delays lads...lot of work and also two small kids ;) |
| @@ -1,6 +1,6 @@ | |||
| { | |||
| "name": "express-fileupload", | |||
| "version": "1.5.0", | |||
| "version": "1.5.1", | |||
There was a problem hiding this comment.
new option is a feature — it should have been minor release (1.6.0)
There was a problem hiding this comment.
Hi @RobinTail
Partially agree with u.
In this case decided not to change 5 to 6, since change quite small and 100% backward compatible.
There was a problem hiding this comment.
|
Thanks again @Kpovoc & @RomanBurunkov !!! |
ref: #374