-
Notifications
You must be signed in to change notification settings - Fork 67
SWIFT-876 Implement more complex example Vapor application #493
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
Conversation
| <a href="/">Home</a> | ||
| <div id="error-info"></div> | ||
|
|
||
| <script type="text/javascript"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL (or maybe I used to know this, but I'd long forgotten) that you can't use a form to send a DELETE or PATCH request, so I had to break out the jQuery here to exercise those code paths. sigh.
Codecov Report
@@ Coverage Diff @@
## master #493 +/- ##
==========================================
+ Coverage 76.96% 77.59% +0.62%
==========================================
Files 125 128 +3
Lines 12994 14641 +1647
==========================================
+ Hits 10001 11360 +1359
- Misses 2993 3281 +288
Continue to review full report at Codecov.
|
| dependencies: [ | ||
| .package(url: "https://github.com/vapor/vapor", .upToNextMajor(from: "4.7.0")), | ||
| .package(url: "https://github.com/vapor/leaf", .exact("4.0.0-rc.1.2")), | ||
| .package(url: "https://github.com/mongodb/mongo-swift-driver", .branch("master")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be updated to 1.0.0 before merging but I am leaving it on master now while in development
nbbeeken
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I don't think this is an issue on our end but when I go to ctrl-c the app it hangs for a bit and then I get:
2020-06-01T12:43:40-0400 error: Could not stop HTTP server: Abort(identifier: "500", status: NIOHTTP1.HTTPResponseStatus.internalServerError, headers: , reason: "Server stop took too long.", source: Vapor.ErrorSource(file: "/Users/neal/code/drivers/swift/mongo-swift-driver/Examples/ComplexVaporExample/.build/checkouts/vapor/Sources/Vapor/HTTP/Server/HTTPServer.swift", function: "close()", line: 310, column: 31, range: nil))
ERROR: Cannot schedule tasks on an EventLoop that has already shut down. This will be upgraded to a forced crash in future SwiftNIO versions.
|
|
||
| This is a fully asynchronous application. At its core is [SwiftNIO](https://github.com/apple/swift-nio), which is used to implement both Vapor and the MongoDB driver. | ||
|
|
||
| The application is a basic HTTP server combined with a minimal frontend, which supports storing a list of kittens and details about them. The application provides examples of various common CRUD operations through its HTTP endpointsS: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: endpoints
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
| return collection.findOne(idFilter) | ||
| // Hop to ensure that the final response future happens on the request's event loop. | ||
| .hop(to: req.eventLoop) | ||
| .unwrap(or: Abort(.notFound, reason: "No kitten with matching ID") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: missing paren? )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch, I must have deleted that somehow before committing 😅
| } | ||
| .flatMapErrorThrowing { error in | ||
| // Give a more helpful error message in case of a duplicate key error. | ||
| if let err = error as? MongoSwift.WriteError, err.writeFailure?.code == 11000 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: MongoSwift.MongoError.WriteError
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch, that wasn't merged yet when I started working on this and I hadn't pulled in the new changes!
kmahar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I am having that shutdown issue as well, even with a totally blank template app that doesn't use the driver. not sure what's up with it 🤔
|
|
||
| This is a fully asynchronous application. At its core is [SwiftNIO](https://github.com/apple/swift-nio), which is used to implement both Vapor and the MongoDB driver. | ||
|
|
||
| The application is a basic HTTP server combined with a minimal frontend, which supports storing a list of kittens and details about them. The application provides examples of various common CRUD operations through its HTTP endpointsS: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
| return collection.findOne(idFilter) | ||
| // Hop to ensure that the final response future happens on the request's event loop. | ||
| .hop(to: req.eventLoop) | ||
| .unwrap(or: Abort(.notFound, reason: "No kitten with matching ID") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch, I must have deleted that somehow before committing 😅
| } | ||
| .flatMapErrorThrowing { error in | ||
| // Give a more helpful error message in case of a duplicate key error. | ||
| if let err = error as? MongoSwift.WriteError, err.writeFailure?.code == 11000 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch, that wasn't merged yet when I started working on this and I hadn't pulled in the new changes!
patrickfreed
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, just have a few suggestions
|
|
||
| This is a fully asynchronous application. At its core is [SwiftNIO](https://github.com/apple/swift-nio), which is used to implement both Vapor and the MongoDB driver. | ||
|
|
||
| The application is a basic HTTP server combined with a minimal frontend, which supports storing a list of kittens and details about them. The application provides examples of various common CRUD operations through its HTTP endpoints: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think one of the last two sentences was meant to be omitted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops, condensed
| This application is a basic HTTP server built using [Vapor](vapor.codes). The server will handle the following types of requests: | ||
| 1. A GET request at the root URL `/` loads the main index page containing a list of kittens. | ||
| 1. A POST request at the root URL `/` adds a new kitten. | ||
| 1. A GET request at the URL `/kittens/{_id}` loads information about the kitten with the specified MongoDB `_id`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since we have a unique index on the names, I think it might be preferable to use them for URLs. they'll look much nicer in the server log and in the browser header having the kitten's names instead of a long hexstring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point. done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good, but this prose section still needs to be updated to reflect the changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
When starting up the application, it gives an error when trying to get the favicon. It would clean up the log a little bit (and make the tab a little cuter in the browse) if we could use a royalty-free kitten favicon. |
|
hmmm, I'm not seeing the favicon error. what browser are you using? |
patrickfreed
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I start up the server and load the page, I see the following logs:
[ NOTICE ] Server starting on http://127.0.0.1:8080
[ INFO ] GET /
[ INFO ] GET /favicon.ico
[ ERROR ] Not Found
From looking at the docs, it looks like it would need to go in the Public directory. The favicon itself kind of a surface level thing, but I think the [ ERROR ] in the logs might distract users.
| This application is a basic HTTP server built using [Vapor](vapor.codes). The server will handle the following types of requests: | ||
| 1. A GET request at the root URL `/` loads the main index page containing a list of kittens. | ||
| 1. A POST request at the root URL `/` adds a new kitten. | ||
| 1. A GET request at the URL `/kittens/{_id}` loads information about the kitten with the specified MongoDB `_id`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good, but this prose section still needs to be updated to reflect the changes.
kmahar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added favicon
| This application is a basic HTTP server built using [Vapor](vapor.codes). The server will handle the following types of requests: | ||
| 1. A GET request at the root URL `/` loads the main index page containing a list of kittens. | ||
| 1. A POST request at the root URL `/` adds a new kitten. | ||
| 1. A GET request at the URL `/kittens/{_id}` loads information about the kitten with the specified MongoDB `_id`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
patrickfreed
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
favicon is very cute 🐱
lgtm!
|
credits to @nbbeeken for finding it 🙂 |
Implements the application laid out in the recent scope/design doc.
I'm definitely not following best web dev practices on the frontend stuff here, but the aim of this application is really to demonstrate the backend, so I think that's fine. I put a disclaimer in the README that the app is not production-ready and the HTML/js is not following best practices. That said if you think I've done anything glaringly bad that is easy to fix, let me know.