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

KETTLE-66: Provide a standard Kettle middleware wrapper for Multer #41

Merged
merged 74 commits into from Aug 16, 2018

Conversation

Projects
None yet
2 participants
@waharnum
Copy link
Member

commented Jan 18, 2018

I'm at the point where the code works and I need feedback on the API and overall design in order to finalize those so I can write some documentation both for the new middleware and the use of https://www.npmjs.com/package/form-data as a means of testing it.

Running the tests:

node tests/MulterTests.js

waharnum added some commits Dec 14, 2017

KETTLE-66: make a generic function that the multer tests can use as w…
…ell out of kettle.tests.dataSource.ensureWriteableEmpty(); update use of ensureWriteableEmpty().
},
// Override this function for different behaviour in the file filter
// function
"getFileFilter": {

This comment has been minimized.

Copy link
@amb26

amb26 Jun 1, 2018

Member

These two are a bit peculiar and indirect - the invoker can simply be fileFilter: {
func: "kettle.npm.multer.allowAll"
}
etc.

This comment has been minimized.

Copy link
@waharnum

waharnum Jun 19, 2018

Author Member

Digging in further, this actually didn't even work at all - Multer doesn't export its own allowAll function (https://github.com/expressjs/multer/blob/80ee2f52432cc0c81c93b03c6b0b448af1f626e5/index.js#L7), but the issue was invisible because a "null" function for the file filter wasn't registered by Multer as a problem, as it defaulted to its internal allowAll (the one it doesn't export): https://github.com/expressjs/multer/blob/80ee2f52432cc0c81c93b03c6b0b448af1f626e5/index.js#L22

Middleware integration is fun! I'll clean this up and try to be less indirect and peculiar.

This comment has been minimized.

Copy link
@waharnum

waharnum Jun 20, 2018

Author Member

@amb26 - I think I've accomplished this part of the refactoring, which has reduced the size of the file and the number of functions considerably. Thanks for the suggestions! My only uncertainty is described at https://github.com/waharnum/kettle/blob/KETTLE-66/lib/KettleMiddleware-Multer.js#L46-L58 - is this the best way to accomplish what's needed? (passing in a keyed configuration object to a third-party library's function)

"args": ["{that}.diskStorageConfigurationObject"]
}
},
"memoryStorage": {

This comment has been minimized.

Copy link
@amb26

amb26 Jul 31, 2018

Member

It's odd to always create all of these in case any are required. A better approach would be to turn "storage" into a subcomponent, and for the polymorphism to be based on type - the user would supply

components: {
    storage: {
        type: "kettle.multer.diskStorage"
    }

etc.
Everything relating to "diskStorage" including the destination and filename go onto this subcomponent.
It would help the user if these strange callback-style pieces of configuration were unwrapped since they are really just pieces of data.
That is, the subcomponent should simply support a plain option which holds the destination, and then as a more private piece of implementation the destination function reads

kettle.middleware.multer.defaultDiskStorageDestination = function (req, file, destination, cb) {
    cb(null, destination);
};

written as an invoker so:

       "diskStorageDestination": {
            "funcName": "kettle.middleware.multer.defaultDiskStorageDestination",
            "args": ["{arguments}.0", "{arguments}.1", "{that}.options.destination", "{arguments}.2"]
        }

In this way the user can specify the destination in the expected way by simply overriding the value of the "destination" option

This comment has been minimized.

Copy link
@waharnum

waharnum Aug 1, 2018

Author Member

Agree this is better for common simple uses (including our own to date in the downstream project dependent on this work), will implement.

There are more complex use-cases that Multer supports for computing a destination, but a user can override the invoker in that case.

@@ -0,0 +1,6148 @@
{

This comment has been minimized.

Copy link
@amb26

amb26 Jul 31, 2018

Member

Our policy is to not commit these as per the current .gitignore in trunk - https://github.com/fluid-project/kettle/blob/master/.gitignore#L3 - perhaps this branch needs merging up?

This comment has been minimized.

Copy link
@waharnum

waharnum Aug 1, 2018

Author Member

It was merged up, but there a commit of package-lock.json from before the current .gitignore - I've removed it.

fluid.defaults("kettle.middleware.multer", {
gradeNames: "kettle.plainAsyncMiddleware",
// See https://github.com/expressjs/multer#multeropts;
// because Multer expects to receive functions for some

This comment has been minimized.

Copy link
@amb26

amb26 Jul 31, 2018

Member

Although it expects to receive functions we should do our best to unwrap these back to plain data as a courtesy for implementors

@waharnum

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2018

@amb26 Thanks for reviewing again - I've done another round of refactoring that I hope should address the comments about courtesy to implementors.

What I've done at a high level:

  • extracted both the storage and fileFilter functionality (the areas where Multer expects functions with specific signatures as arguments when constructing the middleware) to subcomponent grades that the main Multer middleware grade now uses
  • removed members except the seemingly necessary diskStorageConfigurationObject at line 199
  • given kettle.middleware.multer.storage.disk plain options for destination and filename that use fluid.stringTemplate to interpolate values from Multer's file information object in the internal implementation - this should allow some power and flexibility in computing destination and filename without having to write new functions
  • documented via comments how an advanced implementation would need to override the existing invokers

I think this is a lot cleaner now.

// object to an invoker where the object uses IoC references
// for the values of its keys?
diskStorageConfigurationObject: {
destination: "{that}.destination",

This comment has been minimized.

Copy link
@amb26

amb26 Aug 14, 2018

Member

Some kind of big problem here since both of these resolve to undefined since they should be referenced as {that}.options.destination. Suggests that the actual values here are never tested, and only the values supplied to the invokers are important. In that case it should probably be removed. If it is retained, it's ok for it to be encoded within "options" as with its source values, but somehow it should receive dedicated tests if it is kept.

This comment has been minimized.

Copy link
@waharnum

waharnum Aug 15, 2018

Author Member

This is confusing because of bad naming (which I'll correct) - {that}.options.destination (a configuration value) and {that}.destination (an invoker) are both valid and separate, but have identical names. I'll change the invokers to destinationImpl and filenameImpl or similar so this is clearer.

Multer's own diskStorage function expects two functions in a key-value structure like so:

{
  destination: function(req, file, cb)...,
  filename: function(req, file, cb)...
}

So the option values are used as arguments to the invoker functions (which use argument-shifting to provide the three-value signatures Multer requires), and the invoker functions are referenced in the diskStorageConfigurationObject, which is passed into the actual Multer diskStorage function by the multerStorage invoker.

I couldn't find an invoker syntax that would allow me to pass an object with IoC-resolved values directly, hence the indirection of the diskStorageConfiguration member that exists only to be passed to the invoker.

I find it confusing explaining this and I wrote the wrapping code, but it's how Multer's internals work - the implementation should hide this from an end user and allow most use cases to simply set filename and destination to string values with fluid.stringTemplate syntax.

This comment has been minimized.

Copy link
@amb26

amb26 Aug 15, 2018

Member

Hi Alan, sorry to misread the code, but I guess this does highlight that it could be confusing. Perhaps "destinationResolver" or "destinationCallback" would be clearer in terms of conveying the different roles of the various incarnations of these values.

I'm not quite sure what you mean by "an invoker syntax that would allow me to pass an object with IoC-resolved values directly" - but perhaps you just mean something like this?

        multerStorage: {
            funcName: "kettle.npm.multer.diskStorage",
            "args": [{
                destination: "{that}.destinationResolver",
                filename: "{that}.filenameResolver"
             }]
        }

This will let you do without the indirection onto diskStorageConfigurationObject. If this wasn't what you were expecting, perhaps you could list a couple of the things you tried that didn't seem to work?

This comment has been minimized.

Copy link
@waharnum

waharnum Aug 15, 2018

Author Member

That syntax works and accomplishes what I want, thanks - I don't remember what I tried before, but it wasn't that (an oversight on my part, as it's the most obvious way of doing it).

As far as the misread - yes, it definitely highlights the confusion and the invokers are better off not sharing names with the options. I've renamed them to use the destinationResolver naming, as I think this best conveys what's going on (to the extent a name can convey this) - the invokers resolve to functions in the style expected by Multer's own implementation.

}
});

// Storage grade using Multer's diskStorage

This comment has been minimized.

Copy link
@amb26

amb26 Aug 14, 2018

Member

These comments have received painfully short line lengths - I think the community considered that 110 or so was a reasonable line length

This comment has been minimized.

Copy link
@waharnum

waharnum Aug 15, 2018

Author Member

I normally break at 80 characters. Not sure what was going on to make me decide that 40-50 was more reasonable here. Fixed.

@amb26 amb26 merged commit 7606c18 into fluid-project:master Aug 16, 2018

1 check passed

license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.