nastra commented on a change in pull request #4244: URL: https://github.com/apache/iceberg/pull/4244#discussion_r819376635
########## File path: core/src/test/java/org/apache/iceberg/rest/TestRESTUtil.java ########## @@ -0,0 +1,106 @@ +/* + * 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.iceberg.rest; + +import java.util.List; +import java.util.Map; +import org.apache.iceberg.catalog.Namespace; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; +import org.assertj.core.api.Assertions; +import org.junit.Test; + +public class TestRESTUtil { + + static class LevelsAndURLEncodedString { + public String[] levels; + public String parameterEncodedForUrlUsage; + + LevelsAndURLEncodedString(String[] levels, String parameterEncodedForUrlUsage) { + this.levels = levels; + this.parameterEncodedForUrlUsage = parameterEncodedForUrlUsage; + } + } + + @Test + public void testFilterAndRemovePrefix() { Review comment: nit: you could maybe use JUnit5's feature to parameterize test cases (https://junit.org/junit5/docs/current/user-guide/#writing-tests-parameterized-tests), since this would simplify the test and clearly state the input and the expected result. As a quick reference I'm attaching https://github.com/projectnessie/nessie/blob/39f40ced48531001eac11c9233bba1951c01c56d/model/src/test/java/org/projectnessie/model/TestTableReference.java#L52-L59 that shows a quick example how this could be done ########## File path: core/src/main/java/org/apache/iceberg/rest/RESTUtil.java ########## @@ -0,0 +1,95 @@ +/* + * 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.iceberg.rest; + +import java.io.UncheckedIOException; +import java.io.UnsupportedEncodingException; +import java.net.URLDecoder; +import java.net.URLEncoder; +import java.nio.charset.StandardCharsets; +import java.util.Map; +import org.apache.iceberg.catalog.Namespace; +import org.apache.iceberg.relocated.com.google.common.base.Joiner; +import org.apache.iceberg.relocated.com.google.common.base.Preconditions; +import org.apache.iceberg.relocated.com.google.common.base.Splitter; +import org.apache.iceberg.relocated.com.google.common.collect.Maps; +import org.apache.iceberg.relocated.com.google.common.collect.Streams; + +class RESTUtil { + private static final Joiner NULL_JOINER = Joiner.on('\u0000'); + private static final Splitter NULL_SPLITTER = Splitter.on('\u0000'); + + private RESTUtil() { + } + + public static String stripTrailingSlash(String path) { + if (path == null) { + return null; + } + + return path.endsWith("/") ? path.substring(0, path.length() - 1) : path; + } + + public static Map<String, String> filterByPrefix(Map<String, String> properties, String prefix) { + Preconditions.checkNotNull(properties, "Invalid properties map: null"); + Map<String, String> result = Maps.newHashMap(); + properties.forEach((key, value) -> { + if (key.startsWith(prefix)) { + result.put(key.substring(prefix.length()), value); + } + }); + + return result; + } + + public static String asURLVariable(Namespace ns) { Review comment: nit: maybe just `urlEncode(Namespace ns)` and `urlDecode(String encodedNs)`, since `Variable` feels kind of misleading ########## File path: core/src/main/java/org/apache/iceberg/rest/RESTUtil.java ########## @@ -0,0 +1,95 @@ +/* + * 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.iceberg.rest; + +import java.io.UncheckedIOException; +import java.io.UnsupportedEncodingException; +import java.net.URLDecoder; +import java.net.URLEncoder; +import java.nio.charset.StandardCharsets; +import java.util.Map; +import org.apache.iceberg.catalog.Namespace; +import org.apache.iceberg.relocated.com.google.common.base.Joiner; +import org.apache.iceberg.relocated.com.google.common.base.Preconditions; +import org.apache.iceberg.relocated.com.google.common.base.Splitter; +import org.apache.iceberg.relocated.com.google.common.collect.Maps; +import org.apache.iceberg.relocated.com.google.common.collect.Streams; + +class RESTUtil { + private static final Joiner NULL_JOINER = Joiner.on('\u0000'); + private static final Splitter NULL_SPLITTER = Splitter.on('\u0000'); + + private RESTUtil() { + } + + public static String stripTrailingSlash(String path) { + if (path == null) { + return null; + } + + return path.endsWith("/") ? path.substring(0, path.length() - 1) : path; + } + + public static Map<String, String> filterByPrefix(Map<String, String> properties, String prefix) { + Preconditions.checkNotNull(properties, "Invalid properties map: null"); + Map<String, String> result = Maps.newHashMap(); + properties.forEach((key, value) -> { + if (key.startsWith(prefix)) { + result.put(key.substring(prefix.length()), value); + } + }); + + return result; + } + + public static String asURLVariable(Namespace ns) { + String[] levels = ns.levels(); + String[] encodedLevels = new String[levels.length]; + + for (int i = 0; i < levels.length; i++) { + try { + encodedLevels[i] = URLEncoder.encode(levels[i], StandardCharsets.UTF_8.name()); + } catch (UnsupportedEncodingException e) { + throw new UncheckedIOException( + String.format("Failed to URL encode namespace as UTF-8 encoding is not supported: %s", ns), e); + } + } + + return NULL_JOINER.join(encodedLevels); + } + + public static Namespace fromURLVariable(String encodedNs) { + Iterable<String> encodedLevels = NULL_SPLITTER.split(encodedNs); Review comment: nit: maybe add a `null` check for the `encodedNs` ########## File path: core/src/main/java/org/apache/iceberg/rest/RESTUtil.java ########## @@ -0,0 +1,95 @@ +/* + * 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.iceberg.rest; + +import java.io.UncheckedIOException; +import java.io.UnsupportedEncodingException; +import java.net.URLDecoder; +import java.net.URLEncoder; +import java.nio.charset.StandardCharsets; +import java.util.Map; +import org.apache.iceberg.catalog.Namespace; +import org.apache.iceberg.relocated.com.google.common.base.Joiner; +import org.apache.iceberg.relocated.com.google.common.base.Preconditions; +import org.apache.iceberg.relocated.com.google.common.base.Splitter; +import org.apache.iceberg.relocated.com.google.common.collect.Maps; +import org.apache.iceberg.relocated.com.google.common.collect.Streams; + +class RESTUtil { + private static final Joiner NULL_JOINER = Joiner.on('\u0000'); + private static final Splitter NULL_SPLITTER = Splitter.on('\u0000'); + + private RESTUtil() { + } + + public static String stripTrailingSlash(String path) { + if (path == null) { + return null; + } + + return path.endsWith("/") ? path.substring(0, path.length() - 1) : path; + } + + public static Map<String, String> filterByPrefix(Map<String, String> properties, String prefix) { Review comment: nit: maybe add some javadoc to make it clear what the purpose of this is ########## File path: core/src/main/java/org/apache/iceberg/rest/RESTUtil.java ########## @@ -0,0 +1,95 @@ +/* + * 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.iceberg.rest; + +import java.io.UncheckedIOException; +import java.io.UnsupportedEncodingException; +import java.net.URLDecoder; +import java.net.URLEncoder; +import java.nio.charset.StandardCharsets; +import java.util.Map; +import org.apache.iceberg.catalog.Namespace; +import org.apache.iceberg.relocated.com.google.common.base.Joiner; +import org.apache.iceberg.relocated.com.google.common.base.Preconditions; +import org.apache.iceberg.relocated.com.google.common.base.Splitter; +import org.apache.iceberg.relocated.com.google.common.collect.Maps; +import org.apache.iceberg.relocated.com.google.common.collect.Streams; + +class RESTUtil { + private static final Joiner NULL_JOINER = Joiner.on('\u0000'); + private static final Splitter NULL_SPLITTER = Splitter.on('\u0000'); + + private RESTUtil() { + } + + public static String stripTrailingSlash(String path) { + if (path == null) { + return null; + } + + return path.endsWith("/") ? path.substring(0, path.length() - 1) : path; + } + + public static Map<String, String> filterByPrefix(Map<String, String> properties, String prefix) { + Preconditions.checkNotNull(properties, "Invalid properties map: null"); + Map<String, String> result = Maps.newHashMap(); + properties.forEach((key, value) -> { + if (key.startsWith(prefix)) { + result.put(key.substring(prefix.length()), value); + } + }); + + return result; + } + + public static String asURLVariable(Namespace ns) { Review comment: also I think it would make sense to add a `null` check for the `ns` ########## File path: core/src/main/java/org/apache/iceberg/rest/RESTUtil.java ########## @@ -0,0 +1,95 @@ +/* + * 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.iceberg.rest; + +import java.io.UncheckedIOException; +import java.io.UnsupportedEncodingException; +import java.net.URLDecoder; +import java.net.URLEncoder; +import java.nio.charset.StandardCharsets; +import java.util.Map; +import org.apache.iceberg.catalog.Namespace; +import org.apache.iceberg.relocated.com.google.common.base.Joiner; +import org.apache.iceberg.relocated.com.google.common.base.Preconditions; +import org.apache.iceberg.relocated.com.google.common.base.Splitter; +import org.apache.iceberg.relocated.com.google.common.collect.Maps; +import org.apache.iceberg.relocated.com.google.common.collect.Streams; + +class RESTUtil { + private static final Joiner NULL_JOINER = Joiner.on('\u0000'); + private static final Splitter NULL_SPLITTER = Splitter.on('\u0000'); + + private RESTUtil() { + } + + public static String stripTrailingSlash(String path) { + if (path == null) { + return null; + } + + return path.endsWith("/") ? path.substring(0, path.length() - 1) : path; + } + + public static Map<String, String> filterByPrefix(Map<String, String> properties, String prefix) { + Preconditions.checkNotNull(properties, "Invalid properties map: null"); + Map<String, String> result = Maps.newHashMap(); + properties.forEach((key, value) -> { + if (key.startsWith(prefix)) { + result.put(key.substring(prefix.length()), value); + } + }); + + return result; + } + + public static String asURLVariable(Namespace ns) { + String[] levels = ns.levels(); + String[] encodedLevels = new String[levels.length]; + + for (int i = 0; i < levels.length; i++) { + try { + encodedLevels[i] = URLEncoder.encode(levels[i], StandardCharsets.UTF_8.name()); + } catch (UnsupportedEncodingException e) { + throw new UncheckedIOException( + String.format("Failed to URL encode namespace as UTF-8 encoding is not supported: %s", ns), e); Review comment: nit: wdyt about adding the actual namespace name right in the middle? `Failed to URL encode namespace '%s'as UTF-8 encoding is not supported` I think this reads a bit more fluent to users running into this error msg, wdyt? Same for the decoding message below maybe -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
