Use Basic auth for credentials instead of query params #548
Conversation
|
Thank you @enqueue! Will it work when |
Good point! I will check it. |
| } | ||
|
|
||
| private static boolean isConfigurationValidForAuth(ClickHouseProperties props) { | ||
| return props.getHost() != null |
There was a problem hiding this comment.
I'm not sure why we need to check host here - it's always null in BalancedClickhouseDataSource. Will this cause authentication issue when connecting to a cluster?
There was a problem hiding this comment.
Good point. I had not thought about this. Will try to find out how the BalancedClickhouseDataSource works.
There was a problem hiding this comment.
Added some tests for common scenarios to BalancedClickhouseDataSourceTest, do you have any others you can think of? The host is set in 'ClickhouseJdbcUrlParser.parseClickhouseUrlfrom the URL, so we hopefully do not have to change the implementation ofBalancedClickhouseDataSource`
There was a problem hiding this comment.
I see. Thanks for the detailed explanation. I don't have anything to add then.
| String password, String expectedAuthHeader) throws Exception | ||
| { | ||
| ClickHouseProperties props = new ClickHouseProperties(); | ||
| props.setHost("localhost"); |
There was a problem hiding this comment.
Could you add one more test when host is null?
There was a problem hiding this comment.
Sure.
How would this happen? The properties being passed here already contain the host as parsed by ClickhouseJdbcUrlParser...
There was a problem hiding this comment.
Thought host is null when using BalancedDataSource but I'll double check :p
|
|
||
| private final CloseableHttpClient client; | ||
|
|
||
| private final HttpClientContext httpContext; |
There was a problem hiding this comment.
Why not make it protected and create the context in constructor?
There was a problem hiding this comment.
This is analogous to the client. This class does not know how to construct these, but gets ready to use "connection stuff".
There was a problem hiding this comment.
Well, I would say that the statement should simply get everything it needs to do its work. It does not need to know how to create connection context, but this is my subjective judgement. I can understand that we want to try and keep the number of ties with Apache HTTP Client under control. I am going to follow your suggestion.
There was a problem hiding this comment.
I see your point. Perhaps it's too early to consider refactoring. I'm fine to keep HttpContext argument now, but I think we may use a new interface like ClickHouseClient to replace both CloseableHtppClient and HttpContext, in order to hide protocol implementation details.
|
Thank you @enqueue for making this pull request. I think we're close to get it merged. Mean time, can we remove HttpContext parameter from Statement and PreparedStatement constructors? Instead, we can create http context inside the constructor. This is because HttpContext is tied to http, so when we add support for more protocols, we'll have to remove it anyway because we only need to pass a client(e.g. HttpClient, NativeClient or GrpcClient) for execution. Let me know if you have different opinion and we can discuss. |
|
Hi @zhicwu I updated the PR to take into account your suggestions. I need your help getting the build to green though. The maven output says something about |
2 participants
Fix for #522