-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[CALCITE-5052] Allow resources created with a JAR protocol (Bazel) #2750
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,7 @@ | |
* limitations under the License. | ||
*/ | ||
package org.apache.calcite.util; | ||
|
||
import com.google.common.io.CharSource; | ||
|
||
import org.junit.jupiter.api.Disabled; | ||
|
@@ -28,6 +29,7 @@ | |
import java.io.IOException; | ||
import java.io.InputStreamReader; | ||
import java.io.Reader; | ||
import java.net.MalformedURLException; | ||
import java.net.URISyntaxException; | ||
import java.net.URL; | ||
import java.nio.charset.StandardCharsets; | ||
|
@@ -40,7 +42,7 @@ | |
|
||
import static org.hamcrest.CoreMatchers.is; | ||
import static org.hamcrest.MatcherAssert.assertThat; | ||
import static org.hamcrest.Matchers.hasToString; | ||
import static org.hamcrest.Matchers.*; | ||
Check failure on line 45 in core/src/test/java/org/apache/calcite/util/SourceTest.java
|
||
import static org.junit.jupiter.api.Assertions.assertEquals; | ||
import static org.junit.jupiter.api.Assertions.assertNotNull; | ||
import static org.junit.jupiter.api.Assertions.assertNull; | ||
|
@@ -118,6 +120,22 @@ | |
() -> "Sources.of(Sources.of(file(" + path + ").absolutePath).url()).file().getPath()"); | ||
} | ||
|
||
/** | ||
* [CALCITE-5052] Resources created with the JAR protocol should also work. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if youre going to link a jira case you can format it like this example: https://github.com/apache/calcite/blob/main/core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java#L277 |
||
* This will enable the Fixture test to work under Bazel. | ||
*/ | ||
@Test void testJarFileUrl() throws MalformedURLException { | ||
// mock jar file | ||
final String jarPath = "jar:file:sources!/abcdef.txt"; | ||
final URL url = new URL(jarPath); | ||
final Source source = of(url); | ||
assertThat("No file retrieved for Sources.of(file " + jarPath + ")", | ||
source.file(), notNullValue()); | ||
assertThat("Sources.of(file " + jarPath + ").url()).file().getPath()", | ||
slashify(source.file().getPath()), | ||
is("sources!/abcdef.txt")); | ||
} | ||
|
||
@Test void testAppendWithSpaces() { | ||
String fooRelative = "fo o+"; | ||
String fooAbsolute = ROOT_PREFIX + "fo o+"; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,6 +23,8 @@ | |
import org.apache.calcite.util.Util; | ||
import org.apache.calcite.util.XmlOutput; | ||
|
||
import org.apache.commons.lang3.StringUtils; | ||
|
||
import com.google.common.cache.CacheBuilder; | ||
import com.google.common.cache.CacheLoader; | ||
import com.google.common.cache.LoadingCache; | ||
|
@@ -53,6 +55,7 @@ | |
import java.util.Objects; | ||
import java.util.SortedMap; | ||
import java.util.TreeMap; | ||
import java.util.regex.Pattern; | ||
import javax.xml.XMLConstants; | ||
import javax.xml.parsers.DocumentBuilder; | ||
import javax.xml.parsers.DocumentBuilderFactory; | ||
|
@@ -170,7 +173,7 @@ public class DiffRepository { | |
* the same class to share the same diff-repository: if the repository gets | ||
* loaded once per test case, then only one diff is recorded. | ||
*/ | ||
private static final LoadingCache<Key, DiffRepository> REPOSITORY_CACHE = | ||
static final LoadingCache<Key, DiffRepository> REPOSITORY_CACHE = | ||
CacheBuilder.newBuilder().build(CacheLoader.from(Key::toRepo)); | ||
|
||
private static final ThreadLocal<@Nullable DocumentBuilderFactory> DOCUMENT_BUILDER_FACTORY = | ||
|
@@ -197,7 +200,7 @@ public class DiffRepository { | |
private Document doc; | ||
private final Element root; | ||
private final URL refFile; | ||
private final File logFile; | ||
final File logFile; | ||
private final Filter filter; | ||
private int modCount; | ||
private int modCountAtLastWrite; | ||
|
@@ -211,7 +214,7 @@ public class DiffRepository { | |
* @param filter Filter or null | ||
* @param indent Indentation of XML file | ||
*/ | ||
private DiffRepository(URL refFile, File logFile, | ||
protected DiffRepository(URL refFile, File logFile, | ||
@Nullable DiffRepository baseRepository, Filter filter, int indent) { | ||
this.baseRepository = baseRepository; | ||
this.filter = filter; | ||
|
@@ -877,7 +880,7 @@ public static DiffRepository lookup(Class<?> clazz, | |
return REPOSITORY_CACHE.getUnchecked(key); | ||
} | ||
|
||
/** | ||
/** | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. mistake? |
||
* Callback to filter strings before returning them. | ||
*/ | ||
public interface Filter { | ||
|
@@ -900,18 +903,21 @@ String filter( | |
} | ||
|
||
/** Cache key. */ | ||
private static class Key { | ||
protected static class Key { | ||
private final Class<?> clazz; | ||
private final DiffRepository baseRepository; | ||
private final Filter filter; | ||
private final int indent; | ||
|
||
private URL refFile; | ||
String refFilePath; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be honest I like the newline after all the variables and before the |
||
Key(Class<?> clazz, DiffRepository baseRepository, Filter filter, | ||
int indent) { | ||
this.clazz = requireNonNull(clazz, "clazz"); | ||
this.baseRepository = baseRepository; | ||
this.filter = filter; | ||
this.indent = indent; | ||
this.refFile = findFile(clazz, ".xml"); | ||
this.refFilePath = Sources.of(refFile).file().getAbsolutePath(); | ||
} | ||
|
||
@Override public int hashCode() { | ||
|
@@ -927,11 +933,19 @@ private static class Key { | |
} | ||
|
||
DiffRepository toRepo() { | ||
final URL refFile = findFile(clazz, ".xml"); | ||
final String refFilePath = Sources.of(refFile).file().getAbsolutePath(); | ||
final String logFilePath = refFilePath | ||
final String logFilePath; | ||
if (StringUtils.containsIgnoreCase(refFilePath, ".jar!")) { | ||
mkou marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// If the file is located in a JAR, we cannot write the file in place | ||
// so we add it to the /tmp directory | ||
// the expected output is /tmp/[jarname]/[path-to-file-in-jar/filename]_actual.xml | ||
logFilePath = Pattern.compile(".*\\/(.*)\\.jar\\!(.*)\\.xml"). | ||
matcher(refFilePath). | ||
replaceAll("/tmp/$1$2_actual.xml"); | ||
} else { | ||
logFilePath = refFilePath | ||
.replace("resources", "diffrepo") | ||
.replace(".xml", "_actual.xml"); | ||
} | ||
final File logFile = new File(logFilePath); | ||
assert !refFilePath.equals(logFile.getAbsolutePath()); | ||
return new DiffRepository(refFile, logFile, baseRepository, filter, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,77 @@ | ||
/* | ||
* 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.calcite.test; | ||
|
||
import com.google.common.cache.CacheBuilder; | ||
import com.google.common.cache.CacheLoader; | ||
import com.google.common.cache.LoadingCache; | ||
|
||
import org.checkerframework.checker.nullness.qual.Nullable; | ||
import org.junit.jupiter.api.Test; | ||
|
||
import java.io.File; | ||
import java.net.URL; | ||
|
||
import static org.junit.jupiter.api.Assertions.assertEquals; | ||
|
||
/** Tests DiffRepository for fixtures. | ||
* | ||
* @see DiffRepository */ | ||
|
||
/** | ||
* Stub of DiffRepository to create a fake source JAR file. | ||
*/ | ||
class JarStubDiffRepository extends DiffRepository { | ||
/** | ||
* Creates a DiffRepository. | ||
* | ||
* @param refFile Reference file | ||
* @param logFile Log file | ||
* @param baseRepository Parent repository or null | ||
* @param filter Filter or null | ||
* @param indent Indentation of XML file | ||
*/ | ||
private JarStubDiffRepository(URL refFile, File logFile, | ||
@Nullable DiffRepository baseRepository, Filter filter, int indent) { | ||
super(refFile, logFile, baseRepository, filter, indent); | ||
} | ||
|
||
public static DiffRepository lookup(Class<?> clazz) { | ||
final JarKey key = new JarKey(clazz, null, null, 2); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should these have comments // be spaced out ? I think the actual logic looks fine ! |
||
return REPOSITORY_CACHE.getUnchecked(key); | ||
} | ||
static final LoadingCache<JarKey, DiffRepository> REPOSITORY_CACHE = | ||
CacheBuilder.newBuilder().build(CacheLoader.from(JarKey::toRepo)); | ||
private static class JarKey extends Key { | ||
JarKey(Class<?> clazz, DiffRepository baseRepository, Filter filter, int indent) { | ||
super(clazz, baseRepository, filter, indent); | ||
// STUBS THE REF FILE TO PRETEND IT LIVES IN A JAR | ||
this.refFilePath = "mypath/calcite-stub.jar!/randompath/abcdef.xml";; | ||
} | ||
} | ||
} | ||
public class DiffRepositoryTest { | ||
|
||
/** | ||
* Test that a DiffRepository from a file in a JAR | ||
* would create a log file outside of the JAR. | ||
**/ | ||
@Test void testLogFilePathInJar() { | ||
final DiffRepository diffRepo = JarStubDiffRepository.lookup(FixtureTest.class); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks so much better now |
||
assertEquals("/tmp/calcite-stub/randompath/abcdef_actual.xml", diffRepo.logFile.getPath()); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Calcite uses explicit imports rather than .*; this is configurable in intellij settings
https://www.jetbrains.com/help/idea/creating-and-optimizing-imports.html#disable-wildcard-imports