Add support for file type #34 by zbarbuto · Pull Request #90 · strongloop/loopback-swagger

zbarbuto · 2017-07-18T04:02:10Z

Also closes strongloop/loopback-component-explorer#103

Description

Allows File type to be turned into a file upload field.

Related issues

Checklist

New tests added or existing tests modified to cover all changes
Code conforms with the style
guide

slnode · 2017-07-18T04:02:11Z

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

slnode · 2017-07-18T04:02:17Z

Can one of the admins verify this patch?

slnode · 2017-07-18T04:02:17Z

Can one of the admins verify this patch?

slnode · 2017-07-18T04:02:17Z

Can one of the admins verify this patch?

slnode · 2017-07-18T04:02:17Z

Can one of the admins verify this patch?

bajtos · 2017-07-18T11:12:02Z

@slnode ok to test

bajtos · 2017-07-18T11:14:42Z

Hello @zbarbuto, thank you for the pull request! The changes looks mostly good to me, however I think we need to be more careful about the new type name.

TL;DR: let's use file as the type name please.

Background: see the discussion in strongloop/loopback#2554. As of strongloop/strong-remoting#370, only file is recognized as a type representing a file (arbitrary stream). The name File is left available for an application-defined model.

zbarbuto · 2017-07-18T12:19:09Z

@bajtos Fair enough. Updated.

The name File is left available for an application-defined model.

Interesting. I thought this wasn't the case as the model name gets lower-cased loopback throws an error if the application tries to define a model called 'file' or 'File':
strongloop/strong-remoting@2c4cc6e

Or by 'application-defined' do you mean core to loopback?

zbarbuto · 2017-07-18T12:20:23Z

lib/codegen/spec-converter.js

          }
-          // Files hasn't made it over yet.
-          //
-          if (target.type === 'File') {


@bajtos Also removed this bock as doesn't seem to be the case anymore after this PR?

Yeah, makes sense. Just note that AFAIK, strong-remoting does not support file uploads (input parameters of file type) yet - see strongloop/loopback#3266

bajtos · 2017-07-19T13:54:04Z

The name File is left available for an application-defined model.

Interesting. I thought this wasn't the case as the model name gets lower-cased loopback throws an error if the application tries to define a model called 'file' or 'File':
strongloop/strong-remoting@2c4cc6e

Or by 'application-defined' do you mean core to loopback?

By application-defined I mean a model defined by a 3rd-party/user application using LoopBack as the framework. E.g. https://github.com/strongloop/loopback-sandbox (it does not have any File model defined though).

After I looked around in strong-remoting, I am little bit confused about the benefits of your proposed changes TBH. If strong-remoting which treats file as a built-in/reserved type that's not supported for input arguments yet, then what's the point of supporting input parameters with file type in swagger spec? What am I missing?

bajtos · 2017-07-19T13:50:58Z

lib/specgen/type-registry.js

    },
  });
  this.registerLoopbackType('DateString', {type: 'string', format: 'date-time'});
+  this.registerLoopbackType('File', {type: 'file'});


I think the type name should be file for consistency with other places where we are using file too.

bajtos · 2017-07-19T13:51:11Z

test/specgen/route-helper.test.js

      });
  });

+  it('converts { type: File\' } to { schema: { type: \'file\' } }', function() {


ditto, type: file

bajtos · 2017-07-19T13:51:17Z

test/specgen/route-helper.test.js

+    var TestModel = loopback.createModel('TestModel', {street: String});
+    var entry = createAPIDoc({
+      accepts: [
+        {name: 'changes', type: 'File'},


and here too

zbarbuto · 2017-07-19T14:04:10Z

After I looked around in strong-remoting, I am little bit confused about the benefits of your proposed changes TBH. If strong-remoting which treats file as a built-in/reserved type that's not supported for input arguments yet, then what's the point of supporting input parameters with file type in swagger spec? What am I missing?

The crux of it is: I want to be able to use the api explorer in conjunction with https://github.com/strongloop/loopback-component-storage in order to test file upload endpoints.

At the moment, I'm having to use third-party API tools just to test my file upload endpoints. Since swagger definitely supports file uploads I thought it would just be a case of modifying the swagger gen to actually output a file upload type in the event of an accepts: type: file situation, then allow swagger to take it from there.

In short: the purpose of this pull request is to allow us to get a file input field in our API explorer instead of just strings as are allowed now.

bajtos · 2017-08-02T12:40:46Z

@zbarbuto first of all, sorry for the delay.

The crux of it is: I want to be able to use the api explorer in conjunction with https://github.com/strongloop/loopback-component-storage in order to test file upload endpoints.

That makes sense 👍

However, I think the fix will require more changes than just here in loopback-swagger.

Take a look at how the file upload method is defined in lib/storage-service.js:

StorageService.prototype.upload.shared = true;
StorageService.prototype.upload.accepts = [
  {arg: 'req', type: 'object', 'http': {source: 'req'}},
  {arg: 'res', type: 'object', 'http': {source: 'res'}},
];
StorageService.prototype.upload.returns = {arg: 'result', type: 'object'};
StorageService.prototype.upload.http =
{verb: 'post', path: '/:container/upload'};

It is accepting a full request object, there is no mention of file uploads. As a result, strong-remoting and loopback-swagger don't have any visibility into what kind of data is the method expecting, we cannot tell that this method expect file uploads.

The proper solution is to implement support for file uploads in strong-remoting and then rework loopback-component-storage to let strong-remoting handle file uploads, instead of the current implementation where loopback-component-storage is parsing multipart requests itself (see lib/storage-handler.js). With that two changes in place, and this pull request landed, you should finally be able to upload files directly from our swagger-ui powered Explorer.

Thoughts?

zbarbuto · 2017-08-02T13:20:38Z

@bajtos you're correct in that it would take multiple changes to get it to work with storage component out of the box. However in my case I'm not using the default methods provided by storage component. I've wrapped them in a custom endpoint. So what this PR achieves is allowing arbitrary endpoints to specify a file type for swagger. Then a separate issue/PR could be made for storage to support this by default.

bajtos · 2017-08-03T08:10:44Z

So what this PR achieves is allowing arbitrary endpoints to specify a file type for swagger. Then a separate issue/PR could be made for storage to support this by default.

Sure, makes sense.

bajtos · 2017-08-03T08:36:04Z

I have rebased you patch on top of the latest master and squashed the commits into a single one.

Landed, thank you for the contribution!

bajtos · 2017-08-10T08:21:24Z

For posterity, strongloop/loopback#3266 is keeping track of the feature allowing remote method to accept file uploads.

zbarbuto force-pushed the feature/file-upload branch from 1f61fa5 to 2891952 Compare July 18, 2017 04:03

bajtos self-assigned this Jul 18, 2017

bajtos added the feature label Jul 18, 2017

bajtos requested review from Amir-61, STRML, bajtos and kjdelisle July 18, 2017 11:14

zbarbuto commented Jul 18, 2017

View reviewed changes

bajtos suggested changes Jul 19, 2017

View reviewed changes

bajtos approved these changes Aug 3, 2017

View reviewed changes

Add support for file-type parameters

978ebbf

bajtos force-pushed the feature/file-upload branch from 08376ca to 978ebbf Compare August 3, 2017 08:12

bajtos merged commit 5292530 into strongloop:master Aug 3, 2017

zbarbuto mentioned this pull request Aug 14, 2017

File type does not correctly upload the file #94

Closed

bajtos mentioned this pull request Oct 13, 2017

Allow remote methods to accept uploaded files strongloop/loopback#3266

Closed

bajtos mentioned this pull request Jan 12, 2018

Support the new return arg type "file" #34

Closed

Conversation

zbarbuto commented Jul 18, 2017

Description

Related issues

Checklist

Uh oh!

slnode commented Jul 18, 2017

Uh oh!

slnode commented Jul 18, 2017

Uh oh!

slnode commented Jul 18, 2017

Uh oh!

slnode commented Jul 18, 2017

Uh oh!

slnode commented Jul 18, 2017

Uh oh!

bajtos commented Jul 18, 2017

Uh oh!

bajtos commented Jul 18, 2017

Uh oh!

zbarbuto commented Jul 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zbarbuto Jul 18, 2017

Choose a reason for hiding this comment

Uh oh!

bajtos Jul 19, 2017

Choose a reason for hiding this comment

Uh oh!

bajtos commented Jul 19, 2017

Uh oh!

bajtos Jul 19, 2017

Choose a reason for hiding this comment

Uh oh!

bajtos Jul 19, 2017

Choose a reason for hiding this comment

Uh oh!

bajtos Jul 19, 2017

Choose a reason for hiding this comment

Uh oh!

zbarbuto commented Jul 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bajtos commented Aug 2, 2017

Uh oh!

zbarbuto commented Aug 2, 2017

Uh oh!

bajtos commented Aug 3, 2017

Uh oh!

bajtos commented Aug 3, 2017

Uh oh!

bajtos commented Aug 10, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zbarbuto commented Jul 18, 2017 •

edited

Loading

zbarbuto commented Jul 19, 2017 •

edited

Loading