Distinguish between "file" and "File" result types by fabien · Pull Request #370 · strongloop/strong-remoting

fabien · 2016-10-13T12:42:05Z

If you have a Loopback model called File you'll receive following error when it is set as a return type: Error: Cannot create a file response from object

In other words, because file is now a reserved keyword with a very specific response handling logic, and File is lowercased during normalization into resultType, the error above would prevent any Loopback being named File. This introduced a breaking change for my apps.

This is loosely related to #318 but in the above case, it's actually not desirable to return a file (buffer/stream) response. Perhaps a more specific keyword than simply file would have been better in hindsight.

See also: a85068e#commitcomment-19410422

@bajtos @richardpringle

fabien · 2016-10-13T12:56:34Z

Related (more or less duplicate of): strongloop/loopback#2554

bajtos · 2016-10-13T12:57:55Z

@fabien thank you for the pull request. I am ok to land this patch as short-term fix.

For longer term: I think the situation where type: 'file' is something else than type: 'File' is quite confusing. Let's discuss how can we improve the format of remoting metadata in strongloop/loopback#2554

fabien · 2016-10-13T13:00:22Z

@bajtos thanks for the quick reply and aim to land it ASAP! With strong-remoting being a dependency of loopback a short-term fix for me would have involved forking both in order to fix the package.json referencing my fork on Github ... much appreciated.

Will look into the discussion some more.

bajtos · 2016-10-13T13:05:05Z

@fabien there are few more places where we are dealing with type: 'file', see e.g.

strong-remoting/lib/shared-method.js

Line 417 in 1efefa6

case 'file':

,

strong-remoting/lib/shared-method.js

Line 567 in 1efefa6

var isFile = targetType === 'file';

and

strong-remoting/lib/shared-method.js

Line 578 in 1efefa6

if (targetType === 'file') {

. PTAL.

bajtos · 2016-10-13T13:05:55Z

lib/http-context.js

    // TODO(bajtos) call SharedMethod's convertToBasicRemotingType here?
    this.resultType = typeof returnDesc.type === 'string' ?
      returnDesc.type.toLowerCase() : returnDesc.type;
+    this.isFile = returnDesc.type === 'file';


Since this is the context, the nameisFile is misleading in my opinion. How about isReturningFile?

@bajtos what about returnsFile?

sure, that's fine too

fabien · 2016-10-13T13:37:05Z

@bajtos as you noted here:

https://github.com/strongloop/strong-remoting/blob/2.x/lib/http-context.js#L415

Why aren't we using convertToBasicRemotingType? If we were, everything could be centralized by internally prefixing the type (example: #file or $file), no?

fabien · 2016-10-13T13:57:03Z

@bajtos I have gone ahead and used convertToBasicRemotingType and then prefix/normalize actual file types as res:file (meaning reserved or response). We can use a different prefix if you want. With this change, handling such custom types is now much more generic. If you wanted to be explicit in your remote method declaration, you could add the prefix yourself:

{ type: 'res:file' }

And it would still work as expected. This means it's backwards compatible, because file, File and res:file will all be handled appropriately.

bajtos · 2016-10-14T12:36:12Z

lib/shared-method.js

-      return type;
+      return normalizedType;
+    case 'file':
+      return type === 'File' ? normalizedType : ('res:' + normalizedType);


IMO we need to use a different condition here: only file should be treated as a built-in file type, everything else is a custom model (FILE, FilE`, etc.).

bajtos · 2016-10-14T12:54:35Z

@fabien I have a different opinion here. I would rather not add a new set of type strings like res:file, I am worried it will create even more confusion (why there is object and not res:object for example?).

As I commented in strongloop/loopback#2554 (comment), I think we should simply make File a reserved type starting from 3.0 release, in which case this fix will stay in the 2.x branch only.

Anyhow, I went ahead and simplified/cleaned up your patch in 7691ff7 to a state that looks good to myself. Could you please take a look at the resulting change? (I pushed the commit to your feature branch thanks to a recently added GitHub feature described in https://github.com/blog/2247-improving-collaboration-with-forks).

If you are ok with what we have here now, then either me or you can squash the commits into a single one, provide a descriptive commit message per our contributor docs, and this pull request can be landed.

fabien · 2016-10-14T13:16:29Z

@bajtos thanks for the cleaned-up patch - I'm totally fine with your implementation! If you don't mind, please go ahead and squash & pull to get it landed 👍

fabien · 2016-10-14T13:18:09Z

Only concern would be the 3.0 release upgrade path for models named File, but I guess that's unavoidable.

bajtos · 2016-10-14T13:24:03Z

Only concern would be the 3.0 release upgrade path for models named File, but I guess that's unavoidable.

Yes, I do realise this means users will have to rework their apps a bit when upgrading from 2.x to 3.0. As you said, this is unavoidable :(

Fix regression introduced by a85068e where we started to treat "File" models as the new built-in type "file". In this commit, the implementation is fixed to recognize only the following form as the built-in file: { type: 'file' } All other spellings, most notably "File", are treated as custom types (models) now.

bajtos · 2016-10-14T14:19:34Z

Landed and released in strong-remoting@2.32.2, thank you for the contribution!

fabien · 2016-10-14T14:19:55Z

@bajtos thanks for landing it so quickly!

fabien added the bug label Oct 13, 2016

fabien assigned bajtos Oct 13, 2016

crandmck added the #review label Oct 13, 2016

bajtos mentioned this pull request Oct 13, 2016

Models called "File" cause an error on POST strongloop/loopback#2554

Closed

4 tasks

bajtos reviewed Oct 13, 2016

View reviewed changes

bajtos reviewed Oct 14, 2016

View reviewed changes

bajtos force-pushed the fix/return-type-file branch from 7691ff7 to 39b1956 Compare October 14, 2016 13:26

bajtos changed the title ~~Don't asume 'file' resultType if type is 'File'~~ Distinguish between "file" and "File" result types Oct 14, 2016

bajtos merged commit 69b13fe into strongloop:2.x Oct 14, 2016

bajtos removed the #review label Oct 14, 2016

bajtos mentioned this pull request Oct 27, 2016

Refuse to override builtin file type #379

Merged

bajtos mentioned this pull request Jul 18, 2017

Add support for file type #34 strongloop/loopback-swagger#90

Merged

2 tasks

Conversation

fabien commented Oct 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fabien commented Oct 13, 2016

Uh oh!

bajtos commented Oct 13, 2016

Uh oh!

fabien commented Oct 13, 2016

Uh oh!

bajtos commented Oct 13, 2016

Uh oh!

bajtos Oct 13, 2016

Choose a reason for hiding this comment

Uh oh!

fabien Oct 13, 2016

Choose a reason for hiding this comment

Uh oh!

bajtos Oct 14, 2016

Choose a reason for hiding this comment

Uh oh!

fabien commented Oct 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fabien commented Oct 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bajtos Oct 14, 2016

Choose a reason for hiding this comment

Uh oh!

bajtos commented Oct 14, 2016

Uh oh!

fabien commented Oct 14, 2016

Uh oh!

fabien commented Oct 14, 2016

Uh oh!

bajtos commented Oct 14, 2016

Uh oh!

bajtos commented Oct 14, 2016

Uh oh!

fabien commented Oct 14, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fabien commented Oct 13, 2016 •

edited

Loading

fabien commented Oct 13, 2016 •

edited

Loading

fabien commented Oct 13, 2016 •

edited

Loading