Convenience methods for socket paths #235
Conversation
|
Can one of the admins verify this patch? |
3 similar comments
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
There was a problem hiding this comment.
I have mixed feelings. I think I'm broadly in favour of the URL constructor helpers, but the rest of the API surface feels like it might be devoting too much API surface area to something that can be addressed more simply. @weissi @artemredkin, what do you think?
| /// | ||
| /// - parameters: | ||
| /// - url: Request url. | ||
| /// - method: Request rethod. |
There was a problem hiding this comment.
| /// - method: Request rethod. | |
| /// - method: Request method. |
There was a problem hiding this comment.
Now that I think about this a bit more, I'm wondering if it should be func execute(method: HTTPMethod, url: String, body: Body? = nil, deadline: NIODeadline? = nil) (swapping method and url)
|
I’d also be happy rolling back the variations on the get/push/patch/put/delete methods, but keeping the execute variations, since they are a bit more general. Alternatively, if there were a way of specifying a default baseURL that I’m unaware of for the whole client, that would alleviate the need for much of this, but it would probably require more internal re-wiring. |
dimitribouniol
force-pushed
the
socket-path-convenience
branch
2 times, most recently
from
662cf46 to
cf4ff26
Compare
|
Rebased these commits on top of the logging changes. @Lukasa I removed the get/push/patch/put/delete methods in f314a6f that I added in favor of a pair of execute() methods: public func execute(socketPath: String, url: String, method: HTTPMethod, body: Body? = nil, deadline: NIODeadline? = nil, logger: Logger? = nil) -> EventLoopFuture<Response>
public func execute(secureSocketPath: String, url: String, method: HTTPMethod, body: Body? = nil, deadline: NIODeadline? = nil, logger: Logger? = nil) -> EventLoopFuture<Response>In cf4ff26, I actually rename the trio of URL execute methods to: public func execute(_ method: HTTPMethod = .GET, url: String, body: Body? = nil, deadline: NIODeadline? = nil, logger: Logger? = nil) -> EventLoopFuture<Response>
public func execute(_ method: HTTPMethod = .GET, socketPath: String, urlPath: String, body: Body? = nil, deadline: NIODeadline? = nil, logger: Logger? = nil) -> EventLoopFuture<Response>
public func execute(_ method: HTTPMethod = .GET, secureSocketPath: String, urlPath: String, body: Body? = nil, deadline: NIODeadline? = nil, logger: Logger? = nil) -> EventLoopFuture<Response>This allows them to be a bit more expressive when used directly: let httpClient = HTTPClient(eventLoopGroupProvider: .createNew)
httpClient.execute(socketPath: "/tmp/myServer.socket",
urlPath: "/path/to/resource").whenComplete (...) // Implicitly GET
httpClient.execute(.POST, secureSocketPath: "/tmp/myServer.socket",
urlPath: "/path/to/resource", body: .string("hello")).whenComplete (...)That said, we can drop the last commit if you prefer the previous naming and argument ordering that I proposed earlier on. @weissi Since I had changes in the same place, I noticed what I think are some leftover issues from logging that I fixed in b65f980, but I may be wrong. Also, please let me know if I handled adding a logger to the other new methods correctly in the later commits, as I opted to use an optional just like with deadline. I wasn't sure if that's OK though, since you had it as a required parameter, but in a separate method for the other examples (which I understand was to maintain backwards compatibility?). I think it should be fine since these are all new methods to begin with? |
|
@swift-server-bot test this please |
| try localClient.execute(.GET, secureSocketPath: path, urlPath: "echo-method").wait().headers[canonicalForm: "X-Method-Used"])) | ||
| XCTAssertNoThrow(XCTAssertEqual(["POST"[...]], | ||
| try localClient.execute(.POST, secureSocketPath: path, urlPath: "echo-method").wait().headers[canonicalForm: "X-Method-Used"])) | ||
| }) |
There was a problem hiding this comment.
would you mind adding the new execute methods to
func testNothingIsLoggedAtInfoOrHigher() {func testAllMethodsLog
You should be able to just c&p one case and use the new execute method, just to make sure logging works with them.
|
@swift-server-bot test this please |
dimitribouniol
force-pushed
the
socket-path-convenience
branch
from
34a2cd5 to
aee71b1
Compare
|
@weiss My apologies, I think I forgot to push the version after running sanity.sh - please test it again. (also rebased while I was at it) |
|
I guess you meant to mention @weissi 😄 |
|
Oh no 😅 |
dimitribouniol
force-pushed
the
socket-path-convenience
branch
from
aee71b1 to
191c095
Compare
|
@weissi So I rebased off the latest changes, but So I guess, don't bother kicking off the tests 😅 Should I file an issue for the crashing test? |
|
I'm looking into that crash right now, it seem to happen only on the mac, linux tests are working as expected. Could you confirm that the error being processed in |
|
@swift-server-bot test this please |
|
Seems like I originally thought it may be the versions of my dependencies, but I haven't tested with the latest of each: |
|
Interesting, yes, it is exactly was I was asking about, thank you! I'll look into that |
|
I can confirm the same crash occurs with a fresh checkout of each dependency, so not version specific at the very least 🙂: |
|
filed: #248 |
dimitribouniol
force-pushed
the
socket-path-convenience
branch
from
191c095 to
ff442bc
Compare
|
@artemredkin Rebased on top of latest changes. Although all tests pass on linux (tested via docker), there are two new concerns (also present on master):
Should I create issues for these? |
dimitribouniol
force-pushed
the
socket-path-convenience
branch
from
ff442bc to
5fd7d1c
Compare
|
@swift-server-bot test this please |
|
@swift-server-bot add to whitelist |
|
@artemredkin this just failed with |
Motivation: While going through the existing tests, I identified a few more instances where we could add some testing. Modifications: Added one test that verifies Requests are being decoded correctly, and improved three others to check for path parsing, error throwing, and schema casing respectively. Result: Tests that continue to pass, but that will also catch any incompatible changes in the future.
Motivation: Some of the convenience request methods weren't properly adapted for logging. Modifications: - Removed a doc comment from patch() that incorrectly referenced a logger. - Fixed an issue where patch() would call into post(). - Added a doc comment to delete() that references the logger. - Tests for the above come in the next commit... Result: Correct documentation and functionality for the patch() and delete() methods.
… making requests to socket paths Motivation: Creating URLs for connecting to servers bound to socket paths currently requires some additional code to get exactly right. It would be nice to have convenience methods on both URL and Request to assist here. Modifications: - Refactored the get/post/patch/put/delete methods so they all call into a one line execute() method. - Added variations on the above methods so they can be called with socket paths (both over HTTP and HTTPS). - Added public convenience initializers to URL to support the above, and so socket path URLs can be easily created in other situations. - Added unit tests for creating socket path URLs, and testing the new suite of convenience execute methods (that, er, test `HTTPMETHOD`s). (patch, put, and delete are now also tested as a result of these tests) - Updated the read me with basic usage instructions. Result: New methods that allow for easily creating requests to socket paths, and passing tests to go with them.
…th based request Motivation: I previously added too much new public API that will most likely not be necessary, and can be better accessed using a generic execute method. Modifications: Removed the get/post/patch/put/delete methods that were specific to socket paths. Result: Less new public API.
…argument in the parameter list Motivation: If these are intended to be general methods for building simple requests, then it makes sense to have the method be the first parameter in the list. Modifications: Moved the `method: HTTPMethod` parameter to the front of the list for all `execute([...] url: [...])` methods, and made it default to .GET. I also changed the url parameter to be `urlPath` for the two socketPath based execute methods. Result: A cleaner public interface for users of the API.
Motivation: The logging tests previously didn't check for socket path-based requests. Modifications: Updated the `testAllMethodsLog()` and `testAllMethodsLog()` tests to include checks for each of the new `execute()` methods. Result: Two more tests that pass.
dimitribouniol
force-pushed
the
socket-path-convenience
branch
from
5fd7d1c to
d8f9c1b
Compare
|
Rebased to keep up to date with master |
|
@weissi @artemredkin I don't know why its passing now - for what it's worth, #259 is failing with the same exact error on the same test, so it may only happen sometimes. |
|
@dimitribouniol thanks, sounds like a flaky test, filed #265 |
There was a problem hiding this comment.
Thank you! Looks good to me. Let's get @artemredkin to have a look because this adds quite a bit of new API.
|
Thank you! |
* Added additional tests for socketPath-based requests Motivation: While going through the existing tests, I identified a few more instances where we could add some testing. Modifications: Added one test that verifies Requests are being decoded correctly, and improved three others to check for path parsing, error throwing, and schema casing respectively. Result: Tests that continue to pass, but that will also catch any incompatible changes in the future. * Added some convenience initializers to URL and methods to Request for making requests to socket paths Motivation: Creating URLs for connecting to servers bound to socket paths currently requires some additional code to get exactly right. It would be nice to have convenience methods on both URL and Request to assist here. Modifications: - Refactored the get/post/patch/put/delete methods so they all call into a one line execute() method. - Added variations on the above methods so they can be called with socket paths (both over HTTP and HTTPS). - Added public convenience initializers to URL to support the above, and so socket path URLs can be easily created in other situations. - Added unit tests for creating socket path URLs, and testing the new suite of convenience execute methods (that, er, test `HTTPMETHOD`s). (patch, put, and delete are now also tested as a result of these tests) - Updated the read me with basic usage instructions. Result: New methods that allow for easily creating requests to socket paths, and passing tests to go with them. * Removed some of the new public methods added for creating a socket-path based request Motivation: I previously added too much new public API that will most likely not be necessary, and can be better accessed using a generic execute method. Modifications: Removed the get/post/patch/put/delete methods that were specific to socket paths. Result: Less new public API. * Renamed execute(url:) methods such that the HTTP method is the first argument in the parameter list Motivation: If these are intended to be general methods for building simple requests, then it makes sense to have the method be the first parameter in the list. Modifications: Moved the `method: HTTPMethod` parameter to the front of the list for all `execute([...] url: [...])` methods, and made it default to .GET. I also changed the url parameter to be `urlPath` for the two socketPath based execute methods. Result: A cleaner public interface for users of the API. * Fixed some minor issues introduces with logging Motivation: Some of the convenience request methods weren't properly adapted for logging. Modifications: - Removed a doc comment from patch() that incorrectly referenced a logger. - Fixed an issue where patch() would call into post(). - Added a doc comment to delete() that references the logger. - Tests for the above come in the next commit... Result: Correct documentation and functionality for the patch() and delete() methods. * Updated logging tests to also check the new execute methods Motivation: The logging tests previously didn't check for socket path-based requests. Modifications: Updated the `testAllMethodsLog()` and `testAllMethodsLog()` tests to include checks for each of the new `execute()` methods. Result: Two more tests that pass.
With the intent of making socket paths a bit easier to use, I added some convenience methods and initializers for creating Requests and URLs directly by specifying the socket path, and the resource path. I also added some additional tests for socket path based use that I missed the first time around.