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

feat: Allow custom express handler after deployd middleware #869

Conversation

evaisse
Copy link
Contributor

@evaisse evaisse commented Aug 30, 2018

I would like to let express or node http server to handle 404 & eventual redirects AFTER deployd routing consider unable to handle the request.

Description

We look into the first handleRequestcall to check if their is a next() handler function. If it as one, we consider that the app is part of an custom deployment script or an express one. We do pass the native handler to deployd router to override deployd default 404 handler.

Motivation and Context

I would redirecting all non handled request to my own express logic afterward the request had been pass through deployd routing. On some other projects make custom 404 pages this way.

For exemple, attaching an express handler AFTER the deployd middleware :

// test deployd routing pass to following handler 
// when a given page match an express handler
app.get('*', function(req, res) {
	res.sendFile('public/index.html');
});

But we can see many more examples.

How Has This Been Tested?

Don't exactly how test it, I do have updated the test-attach.js script but I don't see any test running it...

Screenshots (if appropriate):

Types of changes

  • Breaking change

Possible breaking changes if we consider changing the final handling behavior of deployd routing.

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Add example of custom fallback handler in test-attach.js
@andreialecu
Copy link
Contributor

Tests seem to be failing, not sure if it is related to this PR or some other issue. Did you get tests to run locally?

@evaisse
Copy link
Contributor Author

evaisse commented Sep 3, 2018

Looks like this parts don't work for me.



> deployd@1.1.2 test /Users/evaisse/Sites/projects/deployd
> mocha --timeout 5000 --exit && cd test-app && node runtests.js



Creating a user without roles is deprecated in MongoDB >= 2.6
  1) "before all" hook
  2) "after all" hook

  0 passing (48ms)
  2 failing

  1) "before all" hook:
     MongoError: Use of SCRAM-SHA-256 requires undigested passwords
      at Function.MongoError.create (node_modules/mongodb-core/lib/error.js:31:11)
      at /Users/evaisse/Sites/projects/deployd/node_modules/mongodb-core/lib/connection/pool.js:497:72
      at authenticateStragglers (node_modules/mongodb-core/lib/connection/pool.js:443:16)
      at Connection.messageHandler (node_modules/mongodb-core/lib/connection/pool.js:477:5)
      at Socket.<anonymous> (node_modules/mongodb-core/lib/connection/connection.js:333:22)
      at addChunk (_stream_readable.js:263:12)
      at readableAddChunk (_stream_readable.js:250:11)
      at Socket.Readable.push (_stream_readable.js:208:10)
      at TCP.onread (net.js:597:20)

  2) "after all" hook:
     MongoError: User 'foo@dpd-faux-remote' not found
      at Function.MongoError.create (node_modules/mongodb-core/lib/error.js:31:11)
      at /Users/evaisse/Sites/projects/deployd/node_modules/mongodb-core/lib/connection/pool.js:497:72
      at authenticateStragglers (node_modules/mongodb-core/lib/connection/pool.js:443:16)
      at Connection.messageHandler (node_modules/mongodb-core/lib/connection/pool.js:477:5)
      at Socket.<anonymous> (node_modules/mongodb-core/lib/connection/connection.js:333:22)
      at addChunk (_stream_readable.js:263:12)
      at readableAddChunk (_stream_readable.js:250:11)
      at Socket.Readable.push (_stream_readable.js:208:10)
      at TCP.onread (net.js:597:20)

@evaisse
Copy link
Contributor Author

evaisse commented Sep 4, 2018

Using a custom docker env, I did manage to run the tests :

docker run --name some-mongo -p 27017:27017 mongo:3.2

But still i have some troubles whit one test, but I'm pretty sure it's related to my changes.


  Context
    .url
      ✓ should not have the base url
    .done
      ✓ should provide default headers
      1) should provide default headers

  56 passing (2s)
  1 failing

  1) Context
       .done
         should provide default headers:
     Uncaught TypeError: Cannot read property 'apply' of undefined
      at Request._callback (test/support.js:43:16)
      at Request.self.callback (node_modules/request/request.js:187:22)
      at Request.<anonymous> (node_modules/request/request.js:1044:10)
      at IncomingMessage.<anonymous> (node_modules/request/request.js:965:12)
      at endReadableNT (_stream_readable.js:1064:12)
      at _combinedTickCallback (internal/process/next_tick.js:138:11)
      at process._tickDomainCallback (internal/process/next_tick.js:218:9)


@christianguevara
Copy link

Using a custom docker env, I did manage to run the tests :

docker run --name some-mongo -p 27017:27017 mongo:3.2

But still i have some troubles whit one test, but I'm pretty sure it's related to my changes.


  Context
    .url
      ✓ should not have the base url
    .done
      ✓ should provide default headers
      1) should provide default headers

  56 passing (2s)
  1 failing

  1) Context
       .done
         should provide default headers:
     Uncaught TypeError: Cannot read property 'apply' of undefined
      at Request._callback (test/support.js:43:16)
      at Request.self.callback (node_modules/request/request.js:187:22)
      at Request.<anonymous> (node_modules/request/request.js:1044:10)
      at IncomingMessage.<anonymous> (node_modules/request/request.js:965:12)
      at endReadableNT (_stream_readable.js:1064:12)
      at _combinedTickCallback (internal/process/next_tick.js:138:11)
      at process._tickDomainCallback (internal/process/next_tick.js:218:9)

Is not related, forked the repo, installed deps and run 'npm run test' and the same error on the same lines.

@andreialecu
Copy link
Contributor

@christianguevara a fix for those problems was merged. There are still some problems with this PR though.

@evaisse
Copy link
Contributor Author

evaisse commented Apr 8, 2019

I did just fix the last tests, can you give another review ?

Copy link
Contributor

@andreialecu andreialecu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -31,8 +31,7 @@ Files.prototype.handle = function (ctx, next) {

send(ctx.req, url.parse(ctx.url).pathname, {root: path.resolve(this.public)})
.on('error', function (err) {
ctx.res.statusCode = 404;
respond('Resource Not Found', ctx.req, ctx.res);
return next();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens with inexistent resources here? Do we still return 404 resource not found somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do return a 404. this handler is only run after no resources can be found.

@andreialecu andreialecu merged commit 81b84b1 into deployd:master Apr 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants