Fix image not found response #179

davidcheung · 2016-12-01T20:38:30Z

right now when download function is called and image is not found, it returns a JSON in content-type: image/jpeg which is suboptimal, this fixes it.

This came from trying to make CI green on LB PR's
This PR found an issue in strong-remoting and a PR was created to fix (Rest-adapter errorHandler set content-type json strong-remoting#390)

6/12/16:

Will follow up with @bajtos to get this landed (everything seems green just need to discuss validity of this PR)

Update 7/12/16

Approval from @bajtos for Rest-adapter errorHandler set content-type json strong-remoting#390.

davidcheung · 2016-12-01T20:52:24Z

lib/storage-handler.js

    reader.pipe(res);

    reader.on('error', function onReaderError(err) {
+      res.set('content-type', 'application/json');


this error wasnt being caught before due to the test case, i believe this is also an issue on master (also returning the error in image/jpeg), but the test case doesnt cover it, even tho there are errors

Error: expected "Content-Type" matching /json/, got "image/jpeg; charset=utf-8" at Test.assert (/Users/davidcheung/node-web/loopback-component-storage/node_modules/supertest/lib/test.js:196:25) at assert (/Users/davidcheung/node-web/loopback-component-storage/node_modules/supertest/lib/test.js:132:12) at /Users/davidcheung/node-web/loopback-component-storage/node_modules/supertest/lib/test.js:129:5 at Test.Request.callback (/Users/davidcheung/node-web/loopback-component-storage/node_modules/supertest/node_modules/superagent/lib/node/index.js:746:30) at /Users/davidcheung/node-web/loopback-component-storage/node_modules/supertest/node_modules/superagent/lib/node/index.js:880:14 at IncomingMessage.<anonymous> (/Users/davidcheung/node-web/loopback-component-storage/node_modules/supertest/node_modules/superagent/lib/node/parsers/image.js:8:7) at emitNone (events.js:91:20) at IncomingMessage.emit (events.js:185:7) at endReadableNT (_stream_readable.js:973:12) at /Users/davidcheung/node-web/loopback-component-storage/node_modules/async-listener/glue.js:188:31 at _combinedTickCallback (internal/process/next_tick.js:74:11) at process._tickDomainCallback [as _tickCallback] (internal/process/next_tick.js:122:9)

perhaps i should also pass res into processError() then set it there? @Amir-61

perhaps i should also pass res into processError() then set it there?

@davidcheung I'm not clear what you mean by passing res into processError()... could you please elaborate? AFAICT, processError(err, fileName) accepts err and fileName.

Also having the contect-type as application/json makes sense, in case of error.

hard coding the content-type might be a bad idea because it might mean if your error handler returns a text/html 404, or anything else, the download()'s onError should not really make the assumption that the error must be in a form of application/json, and maybe we should perform content-negotiation with the request's accept to see what we set it as

where in our case processError() is the function that performs operation on the errorObject, so it might make sense to have such logic at the same place

my suggestion was to modify the signature of processError, but im not sure if that make sense either

my suggestion was to modify the signature of processError, but im not sure if that make sense either

ummm that refactoring of processError() could make sense... @bajtos what do you think?

There is one thing I don't understand here. AFAIK, the error handler should be already setting the content-type header, taking into account content types acceptable by the client. Why do we need to set it again here, in the storage component?

I feel we may not understand the full picture and perhaps more investigation is needed?

@bajtos
it calls res.type https://github.com/strongloop/loopback-component-storage/blob/1.x/lib/storage-handler.js#L251 and never changes again, I believe because it is a download method, it does not perform a content-negotiation. but when has an error, it keeps such content-type

lb 2.x
then in rest-adapter strong-remoting@2.x it only does a res.send()

lb 3.x
in 3.x because the errorHandler defaults to sendJson(which also sets content-type) its not a problem https://github.com/strongloop/strong-error-handler/blob/f1d5f064f01fc3c00c15dc62cbde31a1767a77bd/lib/content-negotiation.js#L77-L85

lb 2.x
then in rest-adapter strong-remoting@2.x it only does a res.send()

That looks like a bug in strong-remoting's default error handler. Can we fix strong-remoting instead of working around the problem here?

fixed in strongloop/strong-remoting#390

rmg · 2016-12-01T23:09:14Z

@slnode test please

Amir-61 · 2016-12-02T16:24:33Z

test/upload-download.test.js

      .expect('Content-Type', /json/)
      .expect(200, function(err, res) {
+        if (err) return done(err);
        done();


Why not replacing L351 and L352 with just:

done(err);

Amir-61 · 2016-12-02T16:27:24Z

test/upload-download.test.js

      .expect('Content-Type', /json/)
      .expect(200, function(err, res) {
+        if (err) done(err);
        done();


The same as https://github.com/strongloop/loopback-component-storage/pull/179/files#r90672793

davidcheung · 2016-12-07T19:42:08Z

@slnode test please

davidcheung · 2016-12-07T21:49:42Z

@Amir-61 @bajtos PTAL

Amir-61 · 2016-12-07T21:54:50Z

test/upload-download.test.js

      .set('Content-Type', 'application/json')
      .expect('Content-Type', /json/)
      .expect(200, function(err, res) {
+        if (err) done(err);


Shouldn't this be the following?

if (err) return done(err);

Can you clarify what you are not returning here.

The same applies for other places.

is there a difference whether its returned?
the callback ends the test

is there a difference whether its returned?
the callback ends the test

You call the callback, but that does not mean it terminates the execution (unless you return the callback). In your case it also goes to the next line which is verifyMetadata(res.body, 'test-container');. @pthieu @bajtos to confirm.

Depending on the framework, test might stop at done(err) (but most of the time it's not); I'd say clear and objective code will be more easily debuggable in the future, so I'd suggest:

if (err) { return done(err); }

It's more clear, and no one can argue with you on behaviour

okay, looked at other places in the code also returns the err
i guess it would avoid done() calling more than once

If you don't put the return, it doesn't short circuit at that line. You need it in this case to stop execution unless it's the last line in the func.

Amir-61 · 2016-12-07T22:08:56Z

test/upload-download.test.js

      .expect(200, function(err, res) {
-        if (err) done(err);
+        if (err) return done(err);
        done();


You could have less code here; replacing L281-L282 with just:

done(err);

If you're not returning res, you could just:
.expect(200, done);

Amir-61

After fixing https://github.com/strongloop/loopback-component-storage/pull/179/files#r91399148 and squashing fixups, it LGTM.

pthieu · 2016-12-07T22:13:20Z

test/upload-download.test.js

      .expect(200, function(err, res) {
-        if (err) done(err);
+        if (err) return done(err);
        done();


If you're not returning res, you could just:
.expect(200, done);

pthieu · 2016-12-07T22:14:27Z

test/upload-download.test.js

      .expect('Content-Type', /json/)
      .expect(200, function(err, res) {
-        done();
+        done(err);


if (err)?

maybe just

.expect(200, done);

davidcheung added the #review label Dec 1, 2016

davidcheung commented Dec 1, 2016

View reviewed changes

Amir-61 suggested changes Dec 2, 2016

View reviewed changes

Amir-61 reviewed Dec 2, 2016

View reviewed changes

pthieu added the epitome-team label Dec 5, 2016

pthieu assigned davidcheung Dec 5, 2016

pthieu mentioned this pull request Dec 5, 2016

Allow password reset request for users in realms strongloop/loopback#2980

Merged

2 tasks

davidcheung mentioned this pull request Dec 7, 2016

Rest-adapter errorHandler set content-type json strongloop/strong-remoting#390

Merged

make sure tests fail upon error

9a2de90

davidcheung force-pushed the fix-image-not-found-response branch from bd46bb6 to f475082 Compare December 7, 2016 17:08

Amir-61 suggested changes Dec 7, 2016

View reviewed changes

Amir-61 reviewed Dec 7, 2016

View reviewed changes

Amir-61 approved these changes Dec 7, 2016

View reviewed changes

pthieu suggested changes Dec 7, 2016

View reviewed changes

pthieu approved these changes Dec 7, 2016

View reviewed changes

Image not found should return 404

2df5bc1

davidcheung force-pushed the fix-image-not-found-response branch from de76fdf to 2df5bc1 Compare December 7, 2016 22:29

Amir-61 merged commit 88f9a3a into 1.x Dec 7, 2016

Amir-61 deleted the fix-image-not-found-response branch December 7, 2016 22:50

Amir-61 removed the #review label Dec 7, 2016

Fix image not found response #179

Fix image not found response #179

Uh oh!

Conversation

davidcheung commented Dec 1, 2016 • edited by pthieu Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Amir-61 Dec 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidcheung Dec 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rmg commented Dec 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidcheung commented Dec 7, 2016

Uh oh!

davidcheung commented Dec 7, 2016

Uh oh!

Amir-61 Dec 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Amir-61 Dec 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pthieu Dec 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Amir-61 Dec 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Amir-61 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pthieu Dec 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

davidcheung commented Dec 1, 2016 •

edited by pthieu

Loading

Amir-61 Dec 2, 2016 •

edited

Loading

davidcheung Dec 2, 2016 •

edited

Loading

Amir-61 Dec 7, 2016 •

edited

Loading

Amir-61 Dec 7, 2016 •

edited

Loading

pthieu Dec 7, 2016 •

edited

Loading

Amir-61 Dec 7, 2016 •

edited

Loading

Amir-61 left a comment •

edited

Loading

pthieu Dec 7, 2016 •

edited

Loading