paul-rogers commented on a change in pull request #2270: URL: https://github.com/apache/drill/pull/2270#discussion_r692548066
########## File path: contrib/storage-http/README.md ########## @@ -45,6 +45,30 @@ The `connection` property can accept the following options. `url`: The base URL which Drill will query. +##### Parameters in the URL +Many APIs require parameters to be passed directly in the URL instead of as query arguments. For example, github's API allows you to query an organization's repositories with the following +URL: https://github.com/orgs/{org}/repos + +As of Drill 1.20.0, you can simply set the URL in the connection using the curly braces. If your API includes URL parameters you MUST +include them in the `WHERE` clause in your query, or Drill will throw an error. Review comment: Here we say "MUST", but below we walk it back. Can we improve the flow? ########## File path: contrib/storage-http/README.md ########## @@ -45,6 +45,30 @@ The `connection` property can accept the following options. `url`: The base URL which Drill will query. +##### Parameters in the URL +Many APIs require parameters to be passed directly in the URL instead of as query arguments. For example, github's API allows you to query an organization's repositories with the following +URL: https://github.com/orgs/{org}/repos + +As of Drill 1.20.0, you can simply set the URL in the connection using the curly braces. If your API includes URL parameters you MUST +include them in the `WHERE` clause in your query, or Drill will throw an error. + +As an example, the API above, you would have to query as shown below: + +```sql +SELECT * +FROM api.github +WHERE org = 'apache' +``` + +This query would replace the `org`in the URL with the value from the `WHERE` clause, in this case `apache`. You can specify a default value as follows: `https://someapi.com/ +{param1}/{param2=default}`. In this case, the default would be used if and only if there isn't a parameter supplied in the query. + +#### Limitations on URL Parameters +* Drill does not support combinations of parameters in queries. For instance, for the above example, you could not include `WHERE org='apache' OR org='linux'` Review comment: "combinations of parameters" could be misleading. Can I "combine" query parameters with URL parameters? Of course. I thing what you mean is that Drill does not support multiple values for a single parameter: `OR` or `IN` expressions. ########## File path: contrib/storage-http/README.md ########## @@ -45,6 +45,30 @@ The `connection` property can accept the following options. `url`: The base URL which Drill will query. +##### Parameters in the URL +Many APIs require parameters to be passed directly in the URL instead of as query arguments. For example, github's API allows you to query an organization's repositories with the following +URL: https://github.com/orgs/{org}/repos + +As of Drill 1.20.0, you can simply set the URL in the connection using the curly braces. If your API includes URL parameters you MUST +include them in the `WHERE` clause in your query, or Drill will throw an error. + +As an example, the API above, you would have to query as shown below: + +```sql +SELECT * +FROM api.github +WHERE org = 'apache' +``` + +This query would replace the `org`in the URL with the value from the `WHERE` clause, in this case `apache`. You can specify a default value as follows: `https://someapi.com/ +{param1}/{param2=default}`. In this case, the default would be used if and only if there isn't a parameter supplied in the query. + +#### Limitations on URL Parameters +* Drill does not support combinations of parameters in queries. For instance, for the above example, you could not include `WHERE org='apache' OR org='linux'` +* All URL parameter clauses must be equality only. +* Review comment: Stray asterisk? ########## File path: contrib/storage-http/src/test/java/org/apache/drill/exec/store/http/TestURLParameters.java ########## @@ -0,0 +1,121 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.drill.exec.store.http; + +import okhttp3.HttpUrl; +import org.apache.drill.common.map.CaseInsensitiveMap; +import org.apache.drill.exec.store.http.util.SimpleHttp; +import org.junit.Test; + +import static org.junit.Assert.assertEquals; + +public class TestURLParameters { + + @Test + public void testUrlParameters() { + // Http client setup + HttpUrl githubSingleParam = HttpUrl.parse("https://github.com/orgs/{org}/repos";); + CaseInsensitiveMap<String> filters = CaseInsensitiveMap.newHashMap(); + filters.put("org", "apache"); + filters.put("param1", "value1"); + filters.put("param2", "value2"); + assertEquals("https://github.com/orgs/apache/repos";, SimpleHttp.mapURLParameters(githubSingleParam, filters)); + + + HttpUrl githubMultiParam = HttpUrl.parse("https://github.com/orgs/{org}/{repos}";); + CaseInsensitiveMap<String> filters2 = CaseInsensitiveMap.newHashMap(); + filters2.put("org", "apache"); + filters2.put("param1", "value1"); + filters2.put("repos", "drill"); + assertEquals("https://github.com/orgs/apache/drill";, SimpleHttp.mapURLParameters(githubMultiParam, filters2)); + + HttpUrl githubNoParam = HttpUrl.parse("https://github.com/orgs/org/repos";); + CaseInsensitiveMap<String> filters3 = CaseInsensitiveMap.newHashMap(); + + filters3.put("org", "apache"); + filters3.put("param1", "value1"); + filters3.put("repos", "drill"); + assertEquals("https://github.com/orgs/org/repos";, SimpleHttp.mapURLParameters(githubNoParam, filters3)); + } + + @Test + public void testParamAtEnd() { + HttpUrl pokemonUrl = HttpUrl.parse("https://pokeapi.co/api/v2/pokemon/{pokemon_name}";); + CaseInsensitiveMap<String> filters = CaseInsensitiveMap.newHashMap(); + filters.put("pokemon_name", "Misty"); + filters.put("param1", "value1"); + filters.put("repos", "drill"); + assertEquals("https://pokeapi.co/api/v2/pokemon/Misty";, SimpleHttp.mapURLParameters(pokemonUrl, filters)); + } + + @Test + public void testUpperCase() { + HttpUrl githubSingleParam = HttpUrl.parse("https://github.com/orgs/{ORG}/repos";); + CaseInsensitiveMap<String> filters = CaseInsensitiveMap.newHashMap(); + filters.put("org", "apache"); + filters.put("param1", "value1"); + filters.put("param2", "value2"); + assertEquals("https://github.com/orgs/apache/repos";, SimpleHttp.mapURLParameters(githubSingleParam, filters)); + } + + @Test + public void testURLDefaultParameters() { + HttpUrl githubSingleParam = HttpUrl.parse("https://github.com/orgs/{org=apache}/repos";); + CaseInsensitiveMap<String> filters = CaseInsensitiveMap.newHashMap(); + filters.put("param1", "value1"); + filters.put("param2", "value2"); + assertEquals("https://github.com/orgs/apache/repos";, SimpleHttp.mapURLParameters(githubSingleParam, filters)); + } + + @Test + public void testMixedCase() { + // Since SQL is case-insensitive, + HttpUrl githubSingleParam = HttpUrl.parse("https://github.com/orgs/{ORG}/{org}/repos";); + CaseInsensitiveMap<String> filters = CaseInsensitiveMap.newHashMap(); + filters.put("org", "apache"); + filters.put("ORG", "linux"); Review comment: Thanks for the case-sensitivity tests. ########## File path: contrib/storage-http/src/main/java/org/apache/drill/exec/store/http/util/SimpleHttp.java ########## @@ -1,346 +1,484 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package org.apache.drill.exec.store.http.util; - -import okhttp3.Authenticator; -import okhttp3.Cache; -import okhttp3.Credentials; -import okhttp3.FormBody; -import okhttp3.HttpUrl; -import okhttp3.Interceptor; -import okhttp3.OkHttpClient; -import okhttp3.OkHttpClient.Builder; -import okhttp3.Request; -import okhttp3.Response; -import okhttp3.Route; - -import org.apache.drill.common.exceptions.CustomErrorContext; -import org.apache.drill.common.exceptions.UserException; -import org.apache.drill.exec.store.http.HttpApiConfig; -import org.apache.drill.exec.store.http.HttpApiConfig.HttpMethod; -import org.apache.drill.exec.store.http.HttpStoragePluginConfig; -import org.apache.drill.exec.store.http.HttpSubScan; -import org.jetbrains.annotations.NotNull; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - -import java.io.File; -import java.io.IOException; -import java.io.InputStream; -import java.net.InetSocketAddress; -import java.net.Proxy; -import java.util.Map; -import java.util.Objects; -import java.util.concurrent.TimeUnit; -import java.util.regex.Pattern; - - -/** - * Performs the actual HTTP requests for the HTTP Storage Plugin. The core - * method is the getInputStream() method which accepts a url and opens an - * InputStream with that URL's contents. - */ -public class SimpleHttp { - private static final Logger logger = LoggerFactory.getLogger(SimpleHttp.class); - - private final OkHttpClient client; - private final HttpSubScan scanDefn; - private final File tempDir; - private final HttpProxyConfig proxyConfig; - private final CustomErrorContext errorContext; - private final HttpUrl url; - private String responseMessage; - private int responseCode; - private String responseProtocol; - private String responseURL; - - public SimpleHttp(HttpSubScan scanDefn, HttpUrl url, File tempDir, - HttpProxyConfig proxyConfig, CustomErrorContext errorContext) { - this.scanDefn = scanDefn; - this.url = url; - this.tempDir = tempDir; - this.proxyConfig = proxyConfig; - this.errorContext = errorContext; - this.client = setupHttpClient(); - } - - /** - * Configures the OkHTTP3 server object with configuration info from the user. - * - * @return OkHttpClient configured server - */ - private OkHttpClient setupHttpClient() { - Builder builder = new OkHttpClient.Builder(); - - // Set up the HTTP Cache. Future possibilities include making the cache size and retention configurable but - // right now it is on or off. The writer will write to the Drill temp directory if it is accessible and - // output a warning if not. - HttpStoragePluginConfig config = scanDefn.tableSpec().config(); - if (config.cacheResults()) { - setupCache(builder); - } - - // If the API uses basic authentication add the authentication code. - HttpApiConfig apiConfig = scanDefn.tableSpec().connectionConfig(); - if (apiConfig.authType().toLowerCase().equals("basic")) { - logger.debug("Adding Interceptor"); - builder.addInterceptor(new BasicAuthInterceptor(apiConfig.userName(), apiConfig.password())); - } - - // Set timeouts - int timeout = Math.max(1, config.timeout()); - builder.connectTimeout(timeout, TimeUnit.SECONDS); - builder.writeTimeout(timeout, TimeUnit.SECONDS); - builder.readTimeout(timeout, TimeUnit.SECONDS); - - // Set the proxy configuration - - Proxy.Type proxyType; - switch (proxyConfig.type) { - case SOCKS: - proxyType = Proxy.Type.SOCKS; - break; - case HTTP: - proxyType = Proxy.Type.HTTP; - break; - default: - proxyType = Proxy.Type.DIRECT; - } - if (proxyType != Proxy.Type.DIRECT) { - builder.proxy(new Proxy(proxyType, - new InetSocketAddress(proxyConfig.host, proxyConfig.port))); - if (proxyConfig.username != null) { - builder.proxyAuthenticator(new Authenticator() { - @Override public Request authenticate(Route route, Response response) { - String credential = Credentials.basic(proxyConfig.username, proxyConfig.password); - return response.request().newBuilder() - .header("Proxy-Authorization", credential) - .build(); - } - }); - } - } - - return builder.build(); - } - - public String url() { return url.toString(); } - - public InputStream getInputStream() { - - Request.Builder requestBuilder = new Request.Builder() - .url(url); - - // The configuration does not allow for any other request types other than POST and GET. - HttpApiConfig apiConfig = scanDefn.tableSpec().connectionConfig(); - if (apiConfig.getMethodType() == HttpMethod.POST) { - // Handle POST requests - FormBody.Builder formBodyBuilder = buildPostBody(apiConfig.postBody()); - requestBuilder.post(formBodyBuilder.build()); - } - - // Log the URL and method to aid in debugging user issues. - logger.info("Connection: {}, Method {}, URL: {}", - scanDefn.tableSpec().connection(), - apiConfig.getMethodType().name(), url()); - - // Add headers to request - if (apiConfig.headers() != null) { - for (Map.Entry<String, String> entry : apiConfig.headers().entrySet()) { - requestBuilder.addHeader(entry.getKey(), entry.getValue()); - } - } - - // Build the request object - Request request = requestBuilder.build(); - - try { - // Execute the request - Response response = client - .newCall(request) - .execute(); - - // Preserve the response - responseMessage = response.message(); - responseCode = response.code(); - responseProtocol = response.protocol().toString(); - responseURL = response.request().url().toString(); - - // If the request is unsuccessful, throw a UserException - if (! isSuccessful(responseCode)) { - throw UserException - .dataReadError() - .message("HTTP request failed") - .addContext("Response code", response.code()) - .addContext("Response message", response.message()) - .addContext(errorContext) - .build(logger); - } - logger.debug("HTTP Request for {} successful.", url()); - logger.debug("Response Headers: {} ", response.headers()); - - // Return the InputStream of the response - return Objects.requireNonNull(response.body()).byteStream(); - } catch (IOException e) { - throw UserException - .dataReadError(e) - .message("Failed to read the HTTP response body") - .addContext("Error message", e.getMessage()) - .addContext(errorContext) - .build(logger); - } - } - - /** - * This function is a replacement for the isSuccessful() function which comes - * with okhttp3. The issue is that in some cases, a user may not want Drill to throw - * errors on 400 response codes. This function will return true/false depending on the - * configuration for the specific connection. - * @param responseCode An int of the connection code - * @return True if the response code is 200-299 and possibly 400-499, false if other - */ - private boolean isSuccessful(int responseCode) { - if (scanDefn.tableSpec().connectionConfig().errorOn400()) { - return ((responseCode >= 200 && responseCode <=299) || - (responseCode >= 400 && responseCode <=499)); - } else { - return responseCode >= 200 && responseCode <=299; - } - } - - /** - * Gets the HTTP response code from the HTTP call. Note that this value - * is only available after the getInputStream() method has been called. - * @return int value of the HTTP response code - */ - public int getResponseCode() { - return responseCode; - } - - /** - * Gets the HTTP response code from the HTTP call. Note that this value - * is only available after the getInputStream() method has been called. - * @return int of HTTP response code - */ - public String getResponseMessage() { - return responseMessage; - } - - /** - * Gets the HTTP response code from the HTTP call. Note that this value - * is only available after the getInputStream() method has been called. - * @return The HTTP response protocol - */ - public String getResponseProtocol() { - return responseProtocol; - } - - /** - * Gets the HTTP response code from the HTTP call. Note that this value - * is only available after the getInputStream() method has been called. - * @return The HTTP response URL - */ - public String getResponseURL() { - return responseURL; - } - - /** - * Configures response caching using a provided temp directory. - * - * @param builder - * Builder the Builder object to which the caching is to be - * configured - */ - private void setupCache(Builder builder) { - int cacheSize = 10 * 1024 * 1024; // TODO Add cache size in MB to config - File cacheDirectory = new File(tempDir, "http-cache"); - if (!cacheDirectory.exists()) { - if (!cacheDirectory.mkdirs()) { - throw UserException - .dataWriteError() - .message("Could not create the HTTP cache directory") - .addContext("Path", cacheDirectory.getAbsolutePath()) - .addContext("Please check the temp directory or disable HTTP caching.") - .addContext(errorContext) - .build(logger); - } - } - - try { - Cache cache = new Cache(cacheDirectory, cacheSize); - logger.debug("Caching HTTP Query Results at: {}", cacheDirectory); - builder.cache(cache); - } catch (Exception e) { - throw UserException.dataWriteError(e) - .message("Could not create the HTTP cache") - .addContext("Path", cacheDirectory.getAbsolutePath()) - .addContext("Please check the temp directory or disable HTTP caching.") - .addContext(errorContext) - .build(logger); - } - } - - /** - * Accepts text from a post body in the format:<br> - * {@code key1=value1}<br> - * {@code key2=value2} - * <p> - * and creates the appropriate headers. - * - * @return FormBody.Builder The populated formbody builder - */ - private FormBody.Builder buildPostBody(String postBody) { - final Pattern postBodyPattern = Pattern.compile("^.+=.+$"); - - FormBody.Builder formBodyBuilder = new FormBody.Builder(); - String[] lines = postBody.split("\\r?\\n"); - for(String line : lines) { - - // If the string is in the format key=value split it, - // Otherwise ignore - if (postBodyPattern.matcher(line).find()) { - //Split into key/value - String[] parts = line.split("="); - formBodyBuilder.add(parts[0], parts[1]); - } - } - return formBodyBuilder; - } - - /** - * Intercepts requests and adds authentication headers to the request - */ - public static class BasicAuthInterceptor implements Interceptor { - private final String credentials; - - public BasicAuthInterceptor(String user, String password) { - credentials = Credentials.basic(user, password); - } - - @NotNull - @Override - public Response intercept(Chain chain) throws IOException { - // Get the existing request - Request request = chain.request(); - - // Replace with new request containing the authorization headers and previous headers - Request authenticatedRequest = request.newBuilder().header("Authorization", credentials).build(); - return chain.proceed(authenticatedRequest); - } - } -} +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.drill.exec.store.http.util; + +import okhttp3.Authenticator; +import okhttp3.Cache; +import okhttp3.Credentials; +import okhttp3.FormBody; +import okhttp3.HttpUrl; +import okhttp3.Interceptor; +import okhttp3.OkHttpClient; +import okhttp3.OkHttpClient.Builder; +import okhttp3.Request; +import okhttp3.Response; +import okhttp3.Route; + +import org.apache.drill.common.map.CaseInsensitiveMap; +import org.apache.drill.common.exceptions.CustomErrorContext; +import org.apache.drill.common.exceptions.UserException; +import org.apache.drill.exec.store.http.HttpApiConfig; +import org.apache.drill.exec.store.http.HttpApiConfig.HttpMethod; +import org.apache.drill.exec.store.http.HttpStoragePluginConfig; +import org.apache.drill.exec.store.http.HttpSubScan; +import org.apache.drill.shaded.guava.com.google.common.base.Strings; +import org.jetbrains.annotations.NotNull; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.io.File; +import java.io.IOException; +import java.io.InputStream; +import java.io.UnsupportedEncodingException; +import java.net.InetSocketAddress; +import java.net.Proxy; +import java.net.URLDecoder; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.concurrent.TimeUnit; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + + +/** + * Performs the actual HTTP requests for the HTTP Storage Plugin. The core + * method is the getInputStream() method which accepts a url and opens an + * InputStream with that URL's contents. + */ +public class SimpleHttp { + private static final Logger logger = LoggerFactory.getLogger(SimpleHttp.class); + private static final Pattern URL_PARAM_REGEX = Pattern.compile("\\{(\\w+)=?(\\w*)\\}"); Review comment: Nit: I think this wants to be `"\\{(\\w+)(?:=(\\w*))?\\}"` This makes it clear that the second word is allowed only if there is an `=`. Also, the second word uses `*`. This means that the following is valid: `{foo=}`. This is good, it means that an empty string is a valid default. This works, however, only if the code which checks for a value can tell the difference between no default and a default of a blank string. Can it? ########## File path: contrib/storage-http/src/test/java/org/apache/drill/exec/store/http/TestURLParameters.java ########## @@ -0,0 +1,121 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.drill.exec.store.http; + +import okhttp3.HttpUrl; +import org.apache.drill.common.map.CaseInsensitiveMap; +import org.apache.drill.exec.store.http.util.SimpleHttp; +import org.junit.Test; + +import static org.junit.Assert.assertEquals; + +public class TestURLParameters { + + @Test + public void testUrlParameters() { + // Http client setup + HttpUrl githubSingleParam = HttpUrl.parse("https://github.com/orgs/{org}/repos";); + CaseInsensitiveMap<String> filters = CaseInsensitiveMap.newHashMap(); + filters.put("org", "apache"); + filters.put("param1", "value1"); + filters.put("param2", "value2"); + assertEquals("https://github.com/orgs/apache/repos";, SimpleHttp.mapURLParameters(githubSingleParam, filters)); + + + HttpUrl githubMultiParam = HttpUrl.parse("https://github.com/orgs/{org}/{repos}";); + CaseInsensitiveMap<String> filters2 = CaseInsensitiveMap.newHashMap(); + filters2.put("org", "apache"); + filters2.put("param1", "value1"); + filters2.put("repos", "drill"); + assertEquals("https://github.com/orgs/apache/drill";, SimpleHttp.mapURLParameters(githubMultiParam, filters2)); + + HttpUrl githubNoParam = HttpUrl.parse("https://github.com/orgs/org/repos";); + CaseInsensitiveMap<String> filters3 = CaseInsensitiveMap.newHashMap(); + + filters3.put("org", "apache"); + filters3.put("param1", "value1"); + filters3.put("repos", "drill"); + assertEquals("https://github.com/orgs/org/repos";, SimpleHttp.mapURLParameters(githubNoParam, filters3)); + } + + @Test + public void testParamAtEnd() { + HttpUrl pokemonUrl = HttpUrl.parse("https://pokeapi.co/api/v2/pokemon/{pokemon_name}";); + CaseInsensitiveMap<String> filters = CaseInsensitiveMap.newHashMap(); + filters.put("pokemon_name", "Misty"); + filters.put("param1", "value1"); + filters.put("repos", "drill"); + assertEquals("https://pokeapi.co/api/v2/pokemon/Misty";, SimpleHttp.mapURLParameters(pokemonUrl, filters)); + } + + @Test + public void testUpperCase() { + HttpUrl githubSingleParam = HttpUrl.parse("https://github.com/orgs/{ORG}/repos";); + CaseInsensitiveMap<String> filters = CaseInsensitiveMap.newHashMap(); + filters.put("org", "apache"); + filters.put("param1", "value1"); + filters.put("param2", "value2"); + assertEquals("https://github.com/orgs/apache/repos";, SimpleHttp.mapURLParameters(githubSingleParam, filters)); + } + + @Test + public void testURLDefaultParameters() { + HttpUrl githubSingleParam = HttpUrl.parse("https://github.com/orgs/{org=apache}/repos";); + CaseInsensitiveMap<String> filters = CaseInsensitiveMap.newHashMap(); + filters.put("param1", "value1"); + filters.put("param2", "value2"); + assertEquals("https://github.com/orgs/apache/repos";, SimpleHttp.mapURLParameters(githubSingleParam, filters)); + } + + @Test + public void testMixedCase() { + // Since SQL is case-insensitive, + HttpUrl githubSingleParam = HttpUrl.parse("https://github.com/orgs/{ORG}/{org}/repos";); + CaseInsensitiveMap<String> filters = CaseInsensitiveMap.newHashMap(); + filters.put("org", "apache"); + filters.put("ORG", "linux"); + filters.put("param1", "value1"); + filters.put("param2", "value2"); + assertEquals("https://github.com/orgs/linux/linux/repos";, SimpleHttp.mapURLParameters(githubSingleParam, filters)); + } + + @Test + public void testDuplicateParameters() { + HttpUrl pokemonUrl = HttpUrl.parse("https://pokeapi.co/api/{pokemon_name}/pokemon/{pokemon_name}";); + CaseInsensitiveMap<String> filters = CaseInsensitiveMap.newHashMap(); + filters.put("pokemon_name", "Misty"); + filters.put("param1", "value1"); + filters.put("repos", "drill"); + assertEquals("https://pokeapi.co/api/Misty/pokemon/Misty";, SimpleHttp.mapURLParameters(pokemonUrl, filters)); + } + + @Test + public void testDefaultParametersWithDifferentDatatypes() { + HttpUrl pokemonUrl = HttpUrl.parse("https://pokeapi.co/api/{boolean=true}/{int=1234}";); + CaseInsensitiveMap<String> filters = CaseInsensitiveMap.newHashMap(); + assertEquals("https://pokeapi.co/api/true/1234";, SimpleHttp.mapURLParameters(pokemonUrl,filters)); + } + + @Test + public void testDefaultParameterExtractor() { + HttpUrl pokemonUrl = HttpUrl.parse("https://pokeapi.co/api/{pokemon_name=Misty}";); Review comment: Would be great to test the `{foo=}` case as well to define the expected behavior. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
