Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

refactor: migrate http api to use hapi 18 #1844

Merged
merged 49 commits into from
Feb 5, 2019
Merged

refactor: migrate http api to use hapi 18 #1844

merged 49 commits into from
Feb 5, 2019

Conversation

alanshaw
Copy link
Member

@alanshaw alanshaw commented Jan 24, 2019

DEAR REVIEWERS, SORRY THAT THIS PR IS HUGE 😞


This PR refactors the HTTP API for use with Hapi 18.

The current code works with Hapi 16. This is now two major versions behind the current version and we need to update our code to keep up to date with the latest bug/security fixes and performance improvements.

Hapi 17 introduced significant breaking changes, along with a switch from a callback API to a promises API hence the code changes to support this are also significant.

This PR includes:

  • A switch to using async/await with try/catch over .then/.catch for easier to understand and more succinct code - Hapi 17+ uses this syntax and it's supported in Node.js since version 8
  • Numerous fixes where API was returning a string error message - now returns a proper JSON object
  • Numerous fixes where API was returning 500 for a client error - now returns 400
  • Makes full use of the error handler, instead of repeating code to return an error response with a particular code, it instead throws errors and uses boom to throw errors with specific HTTP status codes e.g.
    // Replaces this:
    return reply({
      Message: "Argument 'arg' is required",
      Code: 0
    }).code(400)
    
    // ...with this:
    throw Boom.badRequest("Argument 'arg' is required")
  • Perf improvement to /api/v0/add - response is no longer buffered in memory, instead it is streamed to the client

refs #1670

@ghost ghost assigned alanshaw Jan 24, 2019
@ghost ghost added the status/in-progress In progress label Jan 24, 2019
alanshaw pushed a commit to ipfs-inactive/js-ipfs-mfs that referenced this pull request Jan 28, 2019
This PR refactors the HTTP API for use with Hapi 18 in preparation for ipfs/js-ipfs#1844.

License: MIT
Signed-off-by: Alan Shaw <[email protected]>
@alanshaw alanshaw changed the title [WIP] refactor: http api for hapi 18 refactor: http api for hapi 18 Jan 29, 2019
@alanshaw alanshaw changed the title refactor: http api for hapi 18 refactor: migrate http api to use hapi 18 Jan 29, 2019
@hugomrdias
Copy link
Member

// Replaces this:
return reply({
Message: "Argument 'arg' is required",
Code: 0
}).code(400)

// ...with this:
throw Boom.badRequest("Argument 'arg' is required")

i think the reply was like that for Go interop, am i wrong ?

@alanshaw
Copy link
Member Author

i think the reply was like that for Go interop, am i wrong ?

The error handler catches the error and transforms it into a { Message, Code, Type } JSON response so we're just removing the boilerplate.

Alan Shaw added 16 commits January 31, 2019 21:37
License: MIT
Signed-off-by: Alan Shaw <[email protected]>
License: MIT
Signed-off-by: Alan Shaw <[email protected]>
License: MIT
Signed-off-by: Alan Shaw <[email protected]>
License: MIT
Signed-off-by: Alan Shaw <[email protected]>
License: MIT
Signed-off-by: Alan Shaw <[email protected]>
License: MIT
Signed-off-by: Alan Shaw <[email protected]>
License: MIT
Signed-off-by: Alan Shaw <[email protected]>
License: MIT
Signed-off-by: Alan Shaw <[email protected]>
Infer error code from status code if not set.

License: MIT
Signed-off-by: Alan Shaw <[email protected]>
License: MIT
Signed-off-by: Alan Shaw <[email protected]>
License: MIT
Signed-off-by: Alan Shaw <[email protected]>
License: MIT
Signed-off-by: Alan Shaw <[email protected]>
License: MIT
Signed-off-by: Alan Shaw <[email protected]>
Alan Shaw added 22 commits January 31, 2019 21:37
License: MIT
Signed-off-by: Alan Shaw <[email protected]>
License: MIT
Signed-off-by: Alan Shaw <[email protected]>
License: MIT
Signed-off-by: Alan Shaw <[email protected]>
License: MIT
Signed-off-by: Alan Shaw <[email protected]>
License: MIT
Signed-off-by: Alan Shaw <[email protected]>
License: MIT
Signed-off-by: Alan Shaw <[email protected]>
License: MIT
Signed-off-by: Alan Shaw <[email protected]>
License: MIT
Signed-off-by: Alan Shaw <[email protected]>
License: MIT
Signed-off-by: Alan Shaw <[email protected]>
License: MIT
Signed-off-by: Alan Shaw <[email protected]>
License: MIT
Signed-off-by: Alan Shaw <[email protected]>
License: MIT
Signed-off-by: Alan Shaw <[email protected]>
License: MIT
Signed-off-by: Alan Shaw <[email protected]>
License: MIT
Signed-off-by: Alan Shaw <[email protected]>
License: MIT
Signed-off-by: Alan Shaw <[email protected]>
License: MIT
Signed-off-by: Alan Shaw <[email protected]>
License: MIT
Signed-off-by: Alan Shaw <[email protected]>
License: MIT
Signed-off-by: Alan Shaw <[email protected]>
License: MIT
Signed-off-by: Alan Shaw <[email protected]>
@alanshaw
Copy link
Member Author

alanshaw commented Feb 3, 2019

Slightly more urgent now - Hapi 16 depends on [email protected] which has a security vulnerability. refs #1862

Copy link
Member

@daviddias daviddias left a comment

Choose a reason for hiding this comment

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

Didn't review in depth, but I applause upgrading to hapi 18 👏🏽👏🏽👏🏽👏🏽

@alanshaw
Copy link
Member Author

alanshaw commented Feb 5, 2019

Okay, I'm going to merge this. It makes me nervous having a PR with so much change hanging around! It LGTM 🤣 and all the tests are passing. Lets PR for any breaks / changes after merge.

@alanshaw alanshaw merged commit dba3085 into master Feb 5, 2019
@ghost ghost removed the status/in-progress In progress label Feb 5, 2019
@alanshaw alanshaw deleted the refactor/hapi-18 branch February 5, 2019 14:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants