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