Differences

This shows you the differences between two versions of the page.

Link to this comparison view

irc:1474754400 [2017/05/27 13:44] (current)
Line 1: Line 1:
 +[14:21:45] <​AlexLehm>​ temporalfox:​ I posted a pr yesterday where i noticed a bug in it immediately after I posted it. I have fixed that now, I will think about that a bit further (since i will be offline anyway for the afternoon)
 +
 +[14:22:03] <​AlexLehm>​ the weather is way too good to spend the afternoon programming
 +
 +[14:22:04] <​temporalfox>​ AlexLehm in which repo ?
 +
 +[14:22:11] <​AlexLehm>​ vertx-mail-clinet
 +
 +[14:22:17] <​temporalfox>​ ok
 +
 +[14:22:22] <​temporalfox>​ so you're coming to F2F ?
 +
 +[14:22:57] <​AlexLehm>​ yes, i have registered the two days as vacation time, everything is checked
 +
 +[14:23:01] <​temporalfox>​ great
 +
 +[14:23:08] <​temporalfox>​ it's really cool you come
 +
 +[14:23:28] <​AlexLehm>​ yes, i think so too
 +
 +[14:23:45] <​AlexLehm>​ (not that I think it is cool i am coming, its cool that we can meet in general)
 +
 +[14:38:11] <​temporalfox>​ AlexLehm I think we should have more documentation about proxy support for HttpClient
 +
 +[14:38:24] <​temporalfox>​ document better what is possible and what is not
 +
 +[14:38:43] <​temporalfox>​ given taht NetClient and HttpClient don't exactly work the same
 +
 +[14:38:49] <​temporalfox>​ specially with TLS
 +
 +[19:46:11] <​BadApe>​ I am writing some integration tests, the strange thing is i don't seem to be able to make multiple calls from an http client, i am guessing i need to do something more after calling end()
 +
 +[21:10:02] <​AlexLehm>​ temporalfox:​ i think the operations now all work with proxy
 +
 +[21:10:15] <​AlexLehm>​ there should be no difference between the clients
 +
 +[21:10:40] <​temporalfox>​ but I think we said with non https we don't support connect for http client
 +
 +[21:10:40] <​temporalfox>​ don't we ?
 +
 +[21:11:14] <​temporalfox>​ ah
 +
 +[21:11:59] <​AlexLehm>​ yes, but the http client does a "​normal"​ connection to the proxy, which isn't done in netclient, since it doesn'​t http
 +
 +[21:13:23] <​AlexLehm>​ I am almost sure that we have convered all use-cases necessary
 +
 +[21:17:10] <​temporalfox>​ but the issue we had recently in the vertx google group
 +
 +[21:17:26] <​temporalfox>​ wasn't about someone that wanted to use http with connect ?
 +
 +[21:17:42] <​temporalfox>​ I always mix-up these ...
 +
 +[21:18:21] <​temporalfox>​ for httpclient and connect what are the rules ? can you remind me these ?
 +
 +[21:18:25] <​AlexLehm>​ yes, we fixed that in the 3.3.3 version
 +
 +[21:19:20] <​AlexLehm>​ httpclient uses connect when calling a https server by requesting the host/port address like CONNECT www.google.com:​443
 +
 +[21:20:18] <​temporalfox>​ but the proxy server is called in clear ?
 +
 +[21:20:26] <​temporalfox>​ or it does not matter ?
 +
 +[21:20:38] <​AlexLehm>​ for http servers, a request for GET / HTTP/1.0 for www.google.de is translated to GET http://​www.google.de/​ HTTP/1.0 to the proxy
 +
 +[21:21:13] <​AlexLehm>​ the CONNECT request to the proxy is sent in clear but the ssl connection is then established to the origin server with ssl so that this doesn'​t matter
 +
 +[21:21:15] <​aesteve>​ evening everyone
 +
 +[21:21:23] <​temporalfox>​ hi arnayd
 +
 +[21:21:24] <​temporalfox>​ arnaud
 +
 +[21:21:34] <​temporalfox>​ AlexLehm ok
 +
 +[21:21:40] <​AlexLehm>​ that is the common use, https connections to the proxy are not used
 +
 +[21:21:50] <​temporalfox>​ so how it works depends on the HttpClient ssl=true|false configuration
 +
 +[21:21:58] <​aesteve>​ I made two PRs, not sure if this is interesting for other users too, but that's a pattern I'm facing quite often
 +
 +[21:22:00] <​temporalfox>​ ssl=true + proxy=http => CONNECT
 +
 +[21:22:06] <​AlexLehm>​ yes
 +
 +[21:22:15] <​temporalfox>​ ssl=false + proxy=http => rewrite requestURI
 +
 +[21:22:22] <​AlexLehm>​ es
 +
 +[21:22:23] <​AlexLehm>​ yes
 +
 +[21:22:40] <​temporalfox>​ I think AlexLehm it's not clear in documentation
 +
 +[21:22:42] <​temporalfox>​ do we document that properly ?
 +
 +[21:23:27] <​AlexLehm>​ sure, i will take a look
 +
 +[21:23:32] <​temporalfox>​ thanks
 +
 +[21:23:48] <​temporalfox>​ @aesteve in the vertx core PR , I've seen a put with Map.Entry<​String,​ T>
 +
 +[21:23:57] <​temporalfox>​ I believe you should remove T and use "?"​ instead
 +
 +[21:24:12] <​temporalfox>​ that would be correct way to do in java
 +
 +[21:24:21] <​temporalfox>​ I can comment in the PR
 +
 +[21:24:46] <​aesteve>​ I'll correct it
 +
 +[21:24:58] <​aesteve>​ I'm always abit confused when it comes to generics
 +
 +[21:25:18] <​temporalfox>​ aesteve everyone is!
 +
 +[21:26:21] <​aesteve>​ I recently created a thin layer of the JPA Criteria API it drove me crazy with generics.... public <T, R extends Collection<?​ super V>, V> ...
 +
 +[21:26:46] <​temporalfox>​ I hate super/​extends
 +
 +[21:26:51] <​AlexLehm>​ its mentioned in the section about http client. but we can probably improve that http://​vertx.io/​docs/​vertx-core/​java/#​_using_a_proxy_for_http_https_connections
 +
 +[21:26:54] <​temporalfox>​ we don't support them in codegen
 +
 +[21:26:58] <​temporalfox>​ and I think it's a good thing :-)
 +
 +[21:27:45] <​temporalfox>​ AlexLehm I think that we should detail each case in a subsections
 +
 +[21:27:49] <​temporalfox>​ all this mix-up to me
 +
 +[21:27:54] <​temporalfox>​ with an example
 +
 +[21:28:09] <​aesteve>​ mmh actually temporalfox I broke my own stuff
 +
 +[21:28:16] <​temporalfox>​ like
 +
 +[21:28:24] <​temporalfox>​ client.get(...)
 +
 +[21:28:25] <​aesteve>​ don't merge it, it doesn'​t compile anylonger
 +
 +[21:28:28] <​temporalfox>​ commented as
 +
 +[21:29:00] <​temporalfox>​ client.get(443,​ "​www.google.com",​ "/",​ ...);
 +
 +[21:29:03] <​temporalfox>​ commented as
 +
 +[21:29:26] <​temporalfox>​ "// make a connect request on the proxy to establish a tunnel to https://​www.google.com:​443"​
 +
 +[21:29:33] <​temporalfox>​ so it is clear
 +
 +[21:29:40] <​temporalfox>​ etc...
 +
 +[21:29:45] <​temporalfox>​ and we make 4 sections
 +
 +[21:29:50] <​temporalfox>​ I mean one for each sock
 +
 +[21:30:10] <​temporalfox>​ one with connect proxy and one with rewrite proxy
 +
 +[21:30:38] <​temporalfox>​ repetition and consistency I think is a good thing in documentation
 +
 +[21:30:44] <​temporalfox>​ in such case
 +
 +[21:31:30] <​aesteve>​ ok fixed
 +
 +[21:31:35] <​aesteve>​ hope this is working now
 +
 +[21:31:51] <​aesteve>​ it's weird though :     ​assertEquals(json.getInteger(1),​ (Integer)3);​
 +
 +[21:32:02] <​temporalfox>​ that's the wonder of java boxing
 +
 +[21:37:53] <​aesteve>​ I was wondering about the name of the method
 +
 +[21:38:06] <​aesteve>​ collector() in both classes can clash with static imports
 +
 +[21:38:18] <​aesteve>​ + isn't java "​standard"​ but vert.x standard
 +
 +[21:38:40] <​temporalfox>​ what do you mean ?
 +
 +[21:38:53] <​temporalfox>​ how about instead having a constructor with a stream in JsonObject and JsonArray ?
 +
 +[21:38:59] <​aesteve>​ it's not getXXX but since it's a static method I think it shouldn'​t anyway
 +
 +[21:39:25] <​temporalfox>​ maybe we can look into other existing libraries to see how they name collectors
 +
 +[21:39:46] <​aesteve>​ yes maybe
 +
 +[21:40:06] <​aesteve>​ and about the Stream constructor let me check, I guess JsonObject has one
 +
 +[21:41:59] <​aesteve>​ no, just JsonObject.stream() and Json.asStream(iterable)
 +
 +[21:42:42] <​aesteve>​ would it be better with a stream constructor or the collector ? wdyt ?
 +
 +[21:43:36] <​aesteve>​ new JsonObject(list.stream().filter(...).map(...));​ or list.stream().filter(...).map(...).collect(JsonObject.collector());​
 +
 +[21:44:04] <​aesteve>​ or maybe both
 +
 +[21:44:23] <​temporalfox>​ using collect(..) seems more like the usual pattern I think
 +
 +[21:44:53] <​temporalfox>​ so we should rather go with that
 +
 +[21:46:20] <​aesteve>​ oh
 +
 +[21:46:41] <​aesteve>​ I named it entryCollector()
 +
 +[21:46:45] <​aesteve>​ forgot about that
 +
 +[21:46:53] <​aesteve>​ so no clash within static imports
 +
 +[21:48:24] <​temporalfox>​ why ?
 +
 +[21:48:42] <​temporalfox>​ ah it collect Map.Entry streams ?
 +
 +[21:50:08] <​aesteve>​ yes
 +
 +[21:50:26] <​aesteve>​ best would have been Tuple<​String,​ ?>
 +
 +[21:50:38] <​aesteve>​ actually, I was wondering
 +
 +[21:50:43] <​temporalfox>​ not for java
 +
 +[21:51:02] <​aesteve>​ instead of doing a collector, what about implementing Collector directly ?
 +
 +[21:51:14] <​temporalfox>​ in ?
 +
 +[21:51:18] <​aesteve>​ list.stream().filter(...).map(...).collect(new JsonObject());​
 +
 +[21:51:29] <​aesteve>​ not sure it makes sense
 +
 +[21:51:46] <​temporalfox>​ hard to tell :-)
 +
 +[21:52:12] <​temporalfox>​ there must be something wrong with that no ?
 +
 +[21:52:12] <​aesteve>​ mmh it's absolutely better !!
 +
 +[21:52:17] <​aesteve>​ check
 +
 +[21:52:27] <​temporalfox>​ because it measn that the same object could be used several times
 +
 +[21:52:41] <​temporalfox>​ i.e it couples the collector and the object itself
 +
 +[21:52:51] <​aesteve>​ JsonObject myJson = new JsonObject().put("​something",​ a); list.stream().filter().map().collect(myJson);​
 +
 +[21:53:19] <​aesteve>​ you can compose an existing jsonobject with the result of filtering a list
 +
 +[21:53:31] <​temporalfox>​ we could also have
 +
 +[21:53:40] <​temporalfox>​ collector(jsonObject)
 +
 +[21:53:42] <​temporalfox>​ with an existing json object
 +
 +[21:53:54] <​aesteve>​ yes why not
 +
 +[21:54:16] <​temporalfox>​ I think just collector is fine
 +
 +[21:54:28] <​temporalfox>​ but I'm not a  fan of static import
 +
 +[21:54:35] <​temporalfox>​ except for junit
 +
 +[21:54:38] <​temporalfox>​ Assert.*
 +
 +[21:54:59] <​aesteve>​ I'm using it a lot with HttpMethods.* and HttpHeaders.*
 +
 +[21:55:24] <​aesteve>​ (GET, POST, PUT, DELETE) is so much better than (HttpMethod.GET,​ HttpMethod.POST,​ ...)
 +
 +[21:55:43] <​aesteve>​ so we're going with collector()
 +
 +[21:55:48] <​aesteve>​ and collector(JsonObject)
 +
 +[21:56:06] <​aesteve>​ ?
 +
 +[21:56:38] <​aesteve>​ (actually I can't see the problem of having it implement the interface directly) but I'm far from being used to collectors
 +
 +[21:57:25] <​temporalfox>​ aesteve can you have a look at the HttpClientBuilder thread on vertx-dev ?
 +
 +[21:57:35] <​aesteve>​ sure
 +
 +[21:57:37] <​temporalfox>​ there are corresponding branches in a few projects
 +
 +[21:57:51] <​temporalfox>​ it's a usability feature for http client
 +
 +[21:57:58] <​temporalfox>​ that will benefit to rxjava
 +
 +[21:58:40] <​aesteve>​ never used RxJava yet
 +
 +[21:58:49] <​aesteve>​ but it'll benefit for Java users too ?
 +
 +[21:59:06] <​temporalfox>​ yes
 +
 +[22:01:09] <​aesteve>​ https://​github.com/​eclipse/​vert.x/​blob/​http-client-request-builder/​src/​test/​java/​io/​vertx/​test/​core/​HttpClientRequestBuilderTest.java#​L61
 +
 +[22:01:15] <​aesteve>​ createPost, I guess
 +
 +[22:02:27] <​temporalfox>​ yes :-)
 +
 +[22:02:34] <​temporalfox>​ but now it's really something not polished :-)
 +
 +[22:03:08] <​aesteve>​ I have to remember the traditional way of issuing a request
 +
 +[22:03:32] <​aesteve>​ request.end(Buffer),​ right ?
 +
 +[22:03:45] <​temporalfox>​ yes
 +
 +[22:03:51] <​aesteve>​ the main change is that you can add the handler when you issue the request ?
 +
 +[22:03:57] <​temporalfox>​ the idea here is to give a ReadStream<​Buffer>​
 +
 +[22:04:10] <​temporalfox>​ and also to copy the builder
 +
 +[22:04:21] <​temporalfox>​ so you can reuse builders several times
 +
 +[22:04:32] <​temporalfox>​ so
 +
 +[22:04:33] <​temporalfox>​ client.createGet(DEFAULT_HTTP_PORT,​ DEFAULT_HTTP_HOST,​ "/​somepath"​);​
 +
 +[22:04:34] <​temporalfox> ​      ​post.putHeader("​Content-Length",​ ""​ + expected.length())
 +
 +[22:04:34] <​aesteve>​ that's a very nice idea, it'll help for dealing with proxies
 +
 +[22:04:40] <​temporalfox>​ can be reused multiples times
 +
 +[22:04:51] <​temporalfox>​ with different ReadStream<​Buffer>​
 +
 +[22:04:57] <​temporalfox>​ and also other idea yes is that
 +
 +[22:04:58] <​temporalfox>​ you use
 +
 +[22:05:04] <​aesteve>​ OK I get it
 +
 +[22:05:12] <​temporalfox>​ request(Handler<​AsyncResult<​HttpClientResponse>>​)
 +
 +[22:05:24] <​temporalfox>​ so you don't need to set exceptionHandler on HttpClientRequest
 +
 +[22:05:32] <​temporalfox>​ and only manage a single error in the async result
 +
 +[22:06:02] <​temporalfox>​ so simplyfing
 +
 +[22:06:11] <​aesteve>​ oh it's an asyncresult
 +
 +[22:06:14] <​temporalfox>​ yes
 +
 +[22:06:20] <​aesteve>​ that'​ll be a pain in some cases though
 +
 +[22:06:25] <​aesteve>​ can't we have both ?
 +
 +[22:06:29] <​temporalfox>​ both ?
 +
 +[22:06:38] <​aesteve>​ oh no obviously
 +
 +[22:06:46] <​aesteve>​ the type is Handler<​...>​
 +
 +[22:06:52] <​temporalfox>​ if you don't use AsyncResult then you ignore failure
 +
 +[22:07:18] <​aesteve>​ In fact I'm thinking about my use cases
 +
 +[22:07:32] <​aesteve>​ I'm using clientRequests for 2 things : proxies and tests
 +
 +[22:08:03] <​aesteve>​ for testing, idc about the exception, that'​ll crash the test, exactly what I need
 +
 +[22:08:36] <​aesteve>​ for proxies actually, having the readstream and the re-usable stuff is a bless
 +
 +[22:09:16] <​temporalfox>​ yes for proxies it make simple indeed
 +
 +[22:09:33] <​temporalfox>​ inside it takes care of setting up the pump
 +
 +[22:09:42] <​temporalfox>​ and stopping if if there is an error
 +
 +[22:10:19] <​aesteve>​ if you want a veryconcrete use case, have a look at how : https://​github.com/​aesteve/​vertx-proxy/​blob/​master/​src/​main/​java/​io/​vertx/​examples/​proxy/​handler/​ProxyHandler.java#​L52-L88 would be simplified
 +
 +[22:10:33] <​aesteve>​ pure awesomeness
 +
 +[22:10:51] <​aesteve>​ just create a builder in the constructor and use it for every incomingRequest
 +
 +[22:11:45] <​temporalfox>​ indeed very similar the same code I have in the HttpClientBuilder
 +
 +[22:11:54] <​aesteve>​ for sure
 +
 +[22:12:04] <​temporalfox>​ now I'm working on HttpClientRequest.reset()
 +
 +[22:12:24] <​temporalfox>​ to abort the request if something goes wrong in the ReadStream
 +
 +[22:12:33] <​temporalfox>​ it works for Http2 as it's an HTTP2 feature
 +
 +[22:12:41] <​temporalfox>​ I'm trying to get something similar for HTTP1
 +
 +[22:12:47] <​temporalfox>​ i.e close gracefully the connection
 +
 +[22:12:54] <​temporalfox>​ specially if it's pipelined
 +
 +[22:13:49] <​aesteve>​ sounds difficult
 +
 +[22:14:08] <​aesteve>​ mmh the first commit I made was with the wrong git account
 +
 +[22:14:43] <​temporalfox>​ with pipelined it shall try to wait until all previous responses have been received and then close the connection
 +
 +[22:14:54] <​temporalfox>​ and not put it back in the pool
 +
 +[22:15:58] <​aesteve>​ mmh and I can't squash because there'​s a merge commit in between
 +
 +[22:16:52] <​temporalfox>​ redo a new PR with cherry-pick
 +
 +[22:19:51] <​aesteve>​ yes I'm goind to do that
 +
 +[22:32:27] <​aesteve>​ mmh actually, the third possible implementation for JsonObject
 +
 +[22:32:33] <​aesteve>​ is to create a class...
 +
 +[22:32:44] <​aesteve>​ the most obvious solution I didn't even think about
 +
 +[22:32:52] <​aesteve>​ new JsonObjectCollector()
 +
 +[22:33:12] <​aesteve>​ or new JsonObjectCollector(existingJsonObject)
 +
 +[22:43:24] <​aesteve>​ temporalfox:​ http://​stackoverflow.com/​questions/​22753755/​how-to-add-elements-of-a-java8-stream-into-an-existing-list
 +
 +[22:43:33] <​aesteve>​ I think it's a matter of parallelism
 +
 +[22:44:26] <​aesteve>​ (the same question I was asking about JsonObject is accurate for Collection<?>​ too, why can't we collect() into an existing Collection)
 +
 +[22:47:09] <​temporalfox>​ "The short answer is no, at least, not in general, you shouldn'​t use a Collector to modify an existing collection."​
 +
 +[22:47:11] <​temporalfox>​ you mean that ?
 +
 +[22:47:48] <​temporalfox>​ looks like it's quite over-engineered though :-)
 +
 +[22:49:29] <​temporalfox>​ interesting read though
 +
 +[22:49:54] <​temporalfox>​ aesteve I would rather prefer the static collector()
 +
 +[22:50:03] <​temporalfox>​ as it does not introduce a new visible type to the user
 +
 +[22:50:17] <​aesteve>​ ok
 +
 +[22:50:33] <​temporalfox>​ also one possible thing is to create a new JsonObject with a collected Map ?
 +
 +[22:50:45] <​temporalfox>​ although I find that collect map is quite difficult
 +
 +[22:50:54] <​temporalfox>​ collect(Collectors.toMap)
 +
 +[22:51:00] <​temporalfox>​ because it's generic ?
 +
 +[22:51:03] <​temporalfox>​ I mean not for entries
 +
 +[22:52:02] <​temporalfox>​ for list cone could do
 +
 +[22:52:24] <​temporalfox>​ new JsonArray(stream....collect(Collectors.toList()));​
 +
 +[22:53:23] <​temporalfox>​ my question is : is it really important to provide a collector over just having a constructor that takes a stream ?
 +
 +[22:56:17] <​aesteve>​ i don't know :\
 +
 +[22:57:35] <​aesteve>​ quite often, I hate instantiating an object just for the sake of fitting a method signature
 +
 +[22:57:50] <​temporalfox>​ ok
 +
 +[22:58:10] <​aesteve>​ I'm telling myself : why do I have to create a map, when a JsonObject already knows how to deal with map entries
 +
 +[22:58:46] <​aesteve>​ but for the collector(existingJson) I'm not really sure
 +
 +[22:59:07] <​aesteve>​ because it's gonna both mutate the original object and return it, maybe it's confusing
 +
 +[23:07:54] <​temporalfox>​ aesteve stackoverload say that collector(existing) is bad
 +
 +[23:08:22] <​aesteve>​ ok
 +
 +[23:09:01] <​temporalfox>​ because of // collectors multithreaded
 +
 +[23:09:48] <​aesteve>​ yes that's what I thought
 +
 +[23:09:51] <​aesteve>​ I removed it
 +
 +[23:12:10] <​temporalfox>​ ok
 +
 +[23:12:43] <​aesteve>​ https://​github.com/​eclipse/​vert.x/​pull/​1645
 +
 +[23:12:49] <​aesteve>​ that should be OK now
 +
 +[23:13:06] <​aesteve>​ correct username for the CLA, correct method names, correct tests
 +
 +[23:14:11] <​aesteve>​ as for the PR re. vertx-web I'm not sure this will be useful, but I've already implemented it in many projects and what decided me was this article : https://​dzone.com/​articles/​new-in-spring-5-functional-web-framework?​edition=216172&​utm_source=Daily%20Digest&​utm_medium=email&​utm_campaign=dd%202016-09-25
 +
 +[23:15:06] <​aesteve>​ the "​then"​ style is sometimes useful
 +
 +[23:15:49] <​aesteve>​ router.route("/​api/​1"​).handler(CorsHandler.create("​*"​)).then(BodyHandler.create()).then(this::​setContentType)...
 +
 +[23:16:04] <​aesteve>​ maybe "​and"​ is a better keyword but since handlers are chained...
 +
 +[23:18:39] <​temporalfox>​ what does the then do ?
 +
 +[23:19:00] <​aesteve>​ just create another route with the same path, same http methods
 +
 +[23:19:30] <​aesteve>​ that's something quite surprising when you first deal with vertx-web
 +
 +[23:19:56] <​aesteve>​ you can't do : Router.route(...).handler(1stHandler).handler(2ndHandler).handler(3rdHandler)
 +
 +[23:20:19] <​aesteve>​ you have to repeat the router.route(...) part
 +
 +[23:20:31] <​aesteve>​ (everytime you attach a handler)
 +
 +[23:21:00] <​aesteve>​ So I'm giving this solution a try, but maybe that's not the good one
 +
 +[23:21:34] <​aesteve>​ let's see what Paulo says
 +
 +[23:21:45] <​temporalfox>​ yep
 +
 +[23:24:56] <​aesteve>​ you could also imagine writing something like : router.routes("/​api/​v1/​*",​ POST, PUT).handler(BodyHandler.create()).onlyFor(POST,​ this::​addContentType)
 +
 +[23:25:10] <​aesteve>​ when you think about it that's abit like your httpclientbuilder
 +
 +[23:25:26] <​aesteve>​ I've already written a RouterBuilder but Groovy-DSL oriented
 +
 +[23:25:35] <​aesteve>​ maybe that's the idea, a RouterBuidler