Skip to content

Commit f3c1f38

Browse files
ludochgae-java-bot
authored andcommitted
Fix shading of DatastoreV3Pb inner classes in remoteapi, fixing #475 by stopping trying to rename classes in shading as it is not reliable.
PiperOrigin-RevId: 869017057 Change-Id: Ie591308e95928ef8ccd931ff4175f04f9b63a6ce
1 parent 29b61fd commit f3c1f38

5 files changed

Lines changed: 131 additions & 20 deletions

File tree

appengine-api-1.0-sdk/pom.xml

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -57,10 +57,6 @@
5757
<configuration>
5858
<createSourcesJar>true</createSourcesJar>
5959
<relocations>
60-
<relocation>
61-
<pattern>com.google.apphosting.datastore_bytes.proto2api.DatastoreV3Pb</pattern>
62-
<shadedPattern>com.google.apphosting.api_bytes.proto2api.DatastorePb</shadedPattern>
63-
</relocation>
6460
<relocation>
6561
<!-- Apache commons codec is repackaged via one of the 'android' dependencies into com.google.api.client.repackaged. -->
6662
<pattern>com.google.api.client.repackaged.org.apache.commons.codec</pattern>

appengine-api-stubs/pom.xml

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -72,10 +72,6 @@
7272
<createSourcesJar>true</createSourcesJar>
7373
<shadeSourcesContent>true</shadeSourcesContent>
7474
<relocations>
75-
<relocation>
76-
<pattern>com.google.apphosting.datastore_bytes.proto2api.DatastoreV3Pb</pattern>
77-
<shadedPattern>com.google.apphosting.api_bytes.proto2api.DatastorePb</shadedPattern>
78-
</relocation>
7975
<relocation>
8076
<!-- Apache commons codec is repackaged via one of the 'android' dependencies
8177
into com.google.api.client.repackaged. -->

appengine_testing/pom.xml

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -88,14 +88,6 @@
8888
<relocation>
8989
<pattern>org.antlr.runtime</pattern>
9090
<shadedPattern>com.google.appengine.repackaged.org.antlr.runtime</shadedPattern>
91-
</relocation>
92-
<relocation>
93-
<pattern>com.google.apphosting.datastore_bytes.proto2api.DatastoreV3Pb</pattern>
94-
<shadedPattern>com.google.apphosting.api_bytes.proto2api.DatastorePb</shadedPattern>
95-
</relocation>
96-
<relocation>
97-
<pattern>com.google.apphosting.datastore.DatastoreV3Pb</pattern>
98-
<shadedPattern>com.google.apphosting.api.DatastorePb</shadedPattern>
9991
</relocation>
10092
<relocation>
10193
<!-- Apache commons codec is repackaged via one of the 'android' dependencies

remoteapi/pom.xml

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,18 @@
9090
</dependencies>
9191
<build>
9292
<plugins>
93+
<plugin>
94+
<groupId>org.apache.maven.plugins</groupId>
95+
<artifactId>maven-failsafe-plugin</artifactId>
96+
<executions>
97+
<execution>
98+
<goals>
99+
<goal>integration-test</goal>
100+
<goal>verify</goal>
101+
</goals>
102+
</execution>
103+
</executions>
104+
</plugin>
93105
<plugin>
94106
<artifactId>maven-shade-plugin</artifactId>
95107
<executions>
@@ -135,10 +147,6 @@
135147
<relocation>
136148
<pattern>org.codehaus.jackson</pattern>
137149
<shadedPattern>com.google.appengine.repackaged.org.codehaus.jackson</shadedPattern>
138-
</relocation>
139-
<relocation>
140-
<pattern>com.google.apphosting.datastore_bytes.proto2api.DatastoreV3Pb</pattern>
141-
<shadedPattern>com.google.apphosting.api_bytes.proto2api.DatastorePb</shadedPattern>
142150
</relocation>
143151
<relocation>
144152
<pattern>com.google.storage.onestore.v3_bytes.proto2api</pattern>
Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,119 @@
1+
/*
2+
* Copyright 2021 Google LLC
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* https://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package com.google.appengine.tools.remoteapi;
18+
19+
import static org.junit.Assert.assertNotNull;
20+
21+
import java.io.File;
22+
import java.lang.reflect.Constructor;
23+
import java.lang.reflect.Method;
24+
import java.net.URL;
25+
import java.net.URLClassLoader;
26+
import java.util.ArrayList;
27+
import java.util.List;
28+
import org.junit.Test;
29+
30+
public class ShadingIT {
31+
32+
@Test
33+
public void testTransactionBuilderRelocation() throws Exception {
34+
File targetDir = new File("target");
35+
if (!targetDir.exists()) {
36+
System.err.println("Target directory not found, skipping shading test.");
37+
return;
38+
}
39+
40+
File[] files =
41+
targetDir.listFiles(
42+
(dir, name) ->
43+
name.startsWith("appengine-remote-api-")
44+
&& name.endsWith(".jar")
45+
&& !name.contains("original")
46+
&& !name.contains("sources")
47+
&& !name.contains("javadoc"));
48+
49+
if (files == null || files.length == 0) {
50+
System.err.println("No shaded jar found in target, skipping shading test.");
51+
return;
52+
}
53+
54+
File shadedJar = files[0];
55+
System.out.println("Testing shaded jar: " + shadedJar.getAbsolutePath());
56+
57+
// Construct a classpath that includes the shaded jar and all dependencies,
58+
// but EXCLUDES the original classes (target/classes) to ensure we test the shaded ones.
59+
List<URL> urls = new ArrayList<>();
60+
urls.add(shadedJar.toURI().toURL());
61+
62+
String classpath = System.getProperty("java.class.path");
63+
String[] paths = classpath.split(File.pathSeparator);
64+
65+
for (String path : paths) {
66+
if (path.contains("target" + File.separator + "classes") || path.contains("target/classes")) {
67+
continue; // Skip original classes
68+
}
69+
if (path.contains("target" + File.separator + "test-classes")
70+
|| path.contains("target/test-classes")) {
71+
continue; // Skip test classes
72+
}
73+
urls.add(new File(path).toURI().toURL());
74+
}
75+
76+
// Use platform classloader as parent to get JDK classes, but not system classpath
77+
ClassLoader parent = ClassLoader.getPlatformClassLoader();
78+
79+
try (URLClassLoader loader = new URLClassLoader(urls.toArray(new URL[0]), parent)) {
80+
81+
// Verify that we can load the TransactionBuilder class
82+
Class<?> tbClass =
83+
loader.loadClass("com.google.appengine.tools.remoteapi.TransactionBuilder");
84+
85+
// Ensure we loaded it from the shaded jar
86+
URL tbUrl = tbClass.getProtectionDomain().getCodeSource().getLocation();
87+
System.out.println("Loaded TransactionBuilder from: " + tbUrl);
88+
if (!tbUrl.toString().contains(shadedJar.getName())) {
89+
System.err.println("WARNING: TransactionBuilder loaded from unexpected location: " + tbUrl);
90+
// We might fail here if we want to be strict, but for now let's proceed and check behavior.
91+
}
92+
93+
Constructor<?> ctor = tbClass.getDeclaredConstructor(boolean.class);
94+
ctor.setAccessible(true);
95+
Object instance = ctor.newInstance(false);
96+
97+
// We invoke the method to ensure that the bytecode within the method
98+
// (which references the relocated inner class) is verified and linked.
99+
// Merely loading the TransactionBuilder class might not trigger the
100+
// resolution of the inner class if it's only used inside a method.
101+
// If the relocation of inner classes failed, this will throw NoClassDefFoundError
102+
// (caused by ClassNotFoundException: ...DatastoreV3Pb$PutRequestOrBuilder)
103+
Method method = tbClass.getDeclaredMethod("makeCommitRequest");
104+
method.setAccessible(true);
105+
106+
try {
107+
Object result = method.invoke(instance);
108+
assertNotNull(result);
109+
} catch (Throwable e) {
110+
// Unwrap reflection exception if possible
111+
Throwable cause = e.getCause();
112+
if (cause != null) {
113+
throw new RuntimeException("Invocation failed: " + cause.toString(), cause);
114+
}
115+
throw e;
116+
}
117+
}
118+
}
119+
}

0 commit comments

Comments
 (0)