[
https://issues.apache.org/jira/browse/BEAM-2230?focusedWorklogId=140692&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-140692
]
ASF GitHub Bot logged work on BEAM-2230:
----------------------------------------
Author: ASF GitHub Bot
Created on: 03/Sep/18 18:14
Start Date: 03/Sep/18 18:14
Worklog Time Spent: 10m
Work Description: stale[bot] closed pull request #3533: [BEAM-2230]
ApiSurface Refactoring
URL: https://github.com/apache/beam/pull/3533
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
diff --git
a/runners/direct-java/src/test/java/org/apache/beam/runners/direct/DirectRunnerApiSurfaceTest.java
b/runners/direct-java/src/test/java/org/apache/beam/runners/direct/DirectRunnerApiSurfaceTest.java
index 631349fc251..7b49873f488 100644
---
a/runners/direct-java/src/test/java/org/apache/beam/runners/direct/DirectRunnerApiSurfaceTest.java
+++
b/runners/direct-java/src/test/java/org/apache/beam/runners/direct/DirectRunnerApiSurfaceTest.java
@@ -22,6 +22,8 @@
import static org.junit.Assert.assertThat;
import com.google.common.collect.ImmutableSet;
+
+import java.io.IOException;
import java.util.Set;
import org.apache.beam.sdk.Pipeline;
import org.apache.beam.sdk.PipelineRunner;
@@ -37,6 +39,32 @@
/** API surface verification for {@link org.apache.beam.runners.direct}. */
@RunWith(JUnit4.class)
public class DirectRunnerApiSurfaceTest {
+ /**
+ * All classes transitively reachable via only public method signatures of
the Direct Runner.
+ *
+ * <p>Note that our idea of "public" does not include various
internal-only APIs.
+ */
+ private static ApiSurface directRunnerApiSurface(
+ final Package directRunnerPackage, final ClassLoader classLoader)
throws IOException {
+ ApiSurface apiSurface =
+ ApiSurface.ofPackage(directRunnerPackage, classLoader)
+ // Do not include dependencies that are required based on
the known exposures.
+ // This could alternatively prune everything exposed by
the public parts of
+ // the Core SDK
+ .pruningClass(Pipeline.class)
+ .pruningClass(PipelineRunner.class)
+ .pruningClass(PipelineOptions.class)
+ .pruningClass(PipelineOptionsRegistrar.class)
+ .pruningClass(PipelineOptions.DirectRunner.class)
+ .pruningClass(DisplayData.Builder.class)
+ .pruningClass(MetricResults.class)
+ .pruningPattern("org[.]apache[.]beam[.].*Test.*")
+ .pruningPattern("org[.]apache[.]beam[.].*IT")
+ .pruningPattern("java[.]io.*")
+ .pruningPattern("java[.]lang.*")
+ .pruningPattern("java[.]util.*");
+ return apiSurface;
+ }
@Test
public void testDirectRunnerApiSurface() throws Exception {
// The DirectRunner can expose the Core SDK, anything exposed by the Core
SDK, and itself
@@ -46,23 +74,6 @@ public void testDirectRunnerApiSurface() throws Exception {
final Package thisPackage = getClass().getPackage();
final ClassLoader thisClassLoader = getClass().getClassLoader();
- ApiSurface apiSurface =
- ApiSurface.ofPackage(thisPackage, thisClassLoader)
- // Do not include dependencies that are required based on the
known exposures. This
- // could alternatively prune everything exposed by the public
parts of the Core SDK
- .pruningClass(Pipeline.class)
- .pruningClass(PipelineRunner.class)
- .pruningClass(PipelineOptions.class)
- .pruningClass(PipelineOptionsRegistrar.class)
- .pruningClass(PipelineOptions.DirectRunner.class)
- .pruningClass(DisplayData.Builder.class)
- .pruningClass(MetricResults.class)
- .pruningPattern("org[.]apache[.]beam[.].*Test.*")
- .pruningPattern("org[.]apache[.]beam[.].*IT")
- .pruningPattern("java[.]io.*")
- .pruningPattern("java[.]lang.*")
- .pruningPattern("java[.]util.*");
-
- assertThat(apiSurface, containsOnlyPackages(allowed));
+ assertThat(directRunnerApiSurface(thisPackage, thisClassLoader),
containsOnlyPackages(allowed));
}
}
diff --git
a/sdks/java/core/src/main/java/org/apache/beam/sdk/util/ApiSurface.java
b/sdks/java/core/src/main/java/org/apache/beam/sdk/util/ApiSurface.java
index 735190b4131..9b04928c65b 100644
--- a/sdks/java/core/src/main/java/org/apache/beam/sdk/util/ApiSurface.java
+++ b/sdks/java/core/src/main/java/org/apache/beam/sdk/util/ApiSurface.java
@@ -822,18 +822,4 @@ private boolean exposed(int modifiers) {
return 0 != (modifiers & (Modifier.PUBLIC | Modifier.PROTECTED));
}
- ////////////////////////////////////////////////////////////////////////////
-
- /**
- * All classes transitively reachable via only public method signatures of
the SDK.
- *
- * <p>Note that our idea of "public" does not include various internal-only
APIs.
- */
- public static ApiSurface getSdkApiSurface(final ClassLoader classLoader)
throws IOException {
- return ApiSurface.ofPackage("org.apache.beam", classLoader)
- .pruningPattern("org[.]apache[.]beam[.].*Test")
- // Exposes Guava, but not intended for users
- .pruningClassName("org.apache.beam.sdk.util.common.ReflectHelpers")
- .pruningPrefix("java");
- }
}
diff --git
a/sdks/java/core/src/test/java/org/apache/beam/SdkCoreApiSurfaceTest.java
b/sdks/java/core/src/test/java/org/apache/beam/SdkCoreApiSurfaceTest.java
index 080f34aa5e6..07206264252 100644
--- a/sdks/java/core/src/test/java/org/apache/beam/SdkCoreApiSurfaceTest.java
+++ b/sdks/java/core/src/test/java/org/apache/beam/SdkCoreApiSurfaceTest.java
@@ -21,6 +21,8 @@
import static org.hamcrest.MatcherAssert.assertThat;
import com.google.common.collect.ImmutableSet;
+
+import java.io.IOException;
import java.util.Set;
import org.apache.beam.sdk.util.ApiSurface;
import org.junit.Test;
@@ -30,6 +32,18 @@
/** API surface verification for {@link org.apache.beam}. */
@RunWith(JUnit4.class)
public class SdkCoreApiSurfaceTest {
+ /**
+ * All classes transitively reachable via only public method signatures of
the SDK.
+ *
+ * <p>Note that our idea of "public" does not include various
internal-only APIs.
+ */
+ public static ApiSurface getSdkApiSurface(final ClassLoader classLoader)
throws IOException {
+ return ApiSurface.ofPackage("org.apache.beam", classLoader)
+ .pruningPattern("org[.]apache[.]beam[.].*Test")
+ // Exposes Guava, but not intended for users
+
.pruningClassName("org.apache.beam.sdk.util.common.ReflectHelpers")
+ .pruningPrefix("java");
+ }
@Test
public void testSdkApiSurface() throws Exception {
@@ -50,6 +64,6 @@ public void testSdkApiSurface() throws Exception {
"org.junit");
assertThat(
- ApiSurface.getSdkApiSurface(getClass().getClassLoader()),
containsOnlyPackages(allowed));
+ getSdkApiSurface(getClass().getClassLoader()),
containsOnlyPackages(allowed));
}
}
diff --git
a/sdks/java/extensions/google-cloud-platform-core/src/test/java/org/apache/beam/sdk/extensions/gcp/GcpCoreApiSurfaceTest.java
b/sdks/java/extensions/google-cloud-platform-core/src/test/java/org/apache/beam/sdk/extensions/gcp/GcpCoreApiSurfaceTest.java
index a0d9e4b6688..95ce7b4603e 100644
---
a/sdks/java/extensions/google-cloud-platform-core/src/test/java/org/apache/beam/sdk/extensions/gcp/GcpCoreApiSurfaceTest.java
+++
b/sdks/java/extensions/google-cloud-platform-core/src/test/java/org/apache/beam/sdk/extensions/gcp/GcpCoreApiSurfaceTest.java
@@ -22,44 +22,58 @@
import static org.hamcrest.MatcherAssert.assertThat;
import com.google.common.collect.ImmutableSet;
+
+import java.io.IOException;
import java.util.Set;
+
import org.apache.beam.sdk.util.ApiSurface;
import org.hamcrest.Matcher;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
-/** API surface verification for Google Cloud Platform core components. */
+/**
+ * API surface verification for Google Cloud Platform core components.
+ */
@RunWith(JUnit4.class)
public class GcpCoreApiSurfaceTest {
+ /**
+ * All classes transitively reachable via only public method signatures of
the GCPCore.
+ *
+ * <p>Note that our idea of "public" does not include various
internal-only APIs.
+ */
+ private static ApiSurface gcpCoreApiSurface(
+ final Package gcpCorePackage, final ClassLoader classLoader)
throws IOException {
+ final ApiSurface apiSurface =
+ ApiSurface.ofPackage(gcpCorePackage, classLoader)
+ .pruningPattern("org[.]apache[.]beam[.].*Test.*")
+ .pruningPattern("org[.]apache[.]beam[.].*IT")
+ .pruningPattern("java[.]lang.*")
+ .pruningPattern("java[.]util.*");
+ return apiSurface;
+ }
- @Test
- public void testGcpCoreApiSurface() throws Exception {
- final Package thisPackage = getClass().getPackage();
- final ClassLoader thisClassLoader = getClass().getClassLoader();
- final ApiSurface apiSurface =
- ApiSurface.ofPackage(thisPackage, thisClassLoader)
- .pruningPattern("org[.]apache[.]beam[.].*Test.*")
- .pruningPattern("org[.]apache[.]beam[.].*IT")
- .pruningPattern("java[.]lang.*")
- .pruningPattern("java[.]util.*");
+ @Test
+ public void testGcpCoreApiSurface() throws Exception {
+ final Package thisPackage = getClass().getPackage();
+ final ClassLoader thisClassLoader = getClass().getClassLoader();
- @SuppressWarnings("unchecked")
- final Set<Matcher<Class<?>>> allowedClasses =
- ImmutableSet.of(
- classesInPackage("com.google.api.client.googleapis"),
- classesInPackage("com.google.api.client.http"),
- classesInPackage("com.google.api.client.json"),
- classesInPackage("com.google.api.client.util"),
- classesInPackage("com.google.api.services.storage"),
- classesInPackage("com.google.auth"),
- classesInPackage("com.fasterxml.jackson.annotation"),
- classesInPackage("java"),
- classesInPackage("javax"),
- classesInPackage("org.apache.beam.sdk"),
- classesInPackage("org.joda.time")
- );
- assertThat(apiSurface, containsOnlyClassesMatching(allowedClasses));
- }
+ @SuppressWarnings("unchecked") final Set<Matcher<Class<?>>>
allowedClasses =
+ ImmutableSet.of(
+ classesInPackage("com.google.api.client.googleapis"),
+ classesInPackage("com.google.api.client.http"),
+ classesInPackage("com.google.api.client.json"),
+ classesInPackage("com.google.api.client.util"),
+ classesInPackage("com.google.api.services.storage"),
+ classesInPackage("com.google.auth"),
+ classesInPackage("com.fasterxml.jackson.annotation"),
+ classesInPackage("java"),
+ classesInPackage("javax"),
+ classesInPackage("org.apache.beam.sdk"),
+ classesInPackage("org.joda.time")
+ );
+ assertThat(gcpCoreApiSurface(thisPackage, thisClassLoader),
+ containsOnlyClassesMatching(allowedClasses));
+ }
}
diff --git
a/sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/GcpApiSurfaceTest.java
b/sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/GcpApiSurfaceTest.java
index 8aac417f581..01c09b4226e 100644
---
a/sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/GcpApiSurfaceTest.java
+++
b/sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/GcpApiSurfaceTest.java
@@ -22,7 +22,10 @@
import static org.hamcrest.MatcherAssert.assertThat;
import com.google.common.collect.ImmutableSet;
+
+import java.io.IOException;
import java.util.Set;
+
import org.apache.beam.sdk.io.gcp.testing.BigqueryMatcher;
import org.apache.beam.sdk.util.ApiSurface;
import org.hamcrest.Matcher;
@@ -34,21 +37,27 @@
/** API surface verification for {@link org.apache.beam.sdk.io.gcp}. */
@RunWith(JUnit4.class)
public class GcpApiSurfaceTest {
+ /**
+ * All classes transitively reachable via only public method signatures of
GCP.
+ *
+ * <p>Note that our idea of "public" does not include various
internal-only APIs.
+ */
+ private static ApiSurface gcpApiSurface(final Package gcpPackage ,
+ final ClassLoader classLoader)
throws IOException {
+ final ApiSurface apiSurface =
+ ApiSurface.ofPackage(gcpPackage, classLoader)
+ .pruningPattern(BigqueryMatcher.class.getName())
+ .pruningPattern("org[.]apache[.]beam[.].*Test.*")
+ .pruningPattern("org[.]apache[.]beam[.].*IT")
+ .pruningPattern("java[.]lang.*")
+ .pruningPattern("java[.]util.*");
+ return apiSurface;
+ }
@Test
public void testGcpApiSurface() throws Exception {
-
final Package thisPackage = getClass().getPackage();
final ClassLoader thisClassLoader = getClass().getClassLoader();
-
- final ApiSurface apiSurface =
- ApiSurface.ofPackage(thisPackage, thisClassLoader)
- .pruningPattern(BigqueryMatcher.class.getName())
- .pruningPattern("org[.]apache[.]beam[.].*Test.*")
- .pruningPattern("org[.]apache[.]beam[.].*IT")
- .pruningPattern("java[.]lang.*")
- .pruningPattern("java[.]util.*");
-
@SuppressWarnings("unchecked")
final Set<Matcher<Class<?>>> allowedClasses =
ImmutableSet.of(
@@ -90,6 +99,7 @@ public void testGcpApiSurface() throws Exception {
classesInPackage("org.apache.commons.logging"),
classesInPackage("org.joda.time"));
- assertThat(apiSurface, containsOnlyClassesMatching(allowedClasses));
+ assertThat(gcpApiSurface(thisPackage, thisClassLoader),
+ containsOnlyClassesMatching(allowedClasses));
}
}
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
Issue Time Tracking
-------------------
Worklog Id: (was: 140692)
Time Spent: 1h 10m (was: 1h)
> Core SDK ApiSurface should be only org.apache.beam.sdk and should be defined
> outside of the general ApiSurface class
> --------------------------------------------------------------------------------------------------------------------
>
> Key: BEAM-2230
> URL: https://issues.apache.org/jira/browse/BEAM-2230
> Project: Beam
> Issue Type: Bug
> Components: sdk-java-core
> Reporter: Kenneth Knowles
> Priority: Major
> Time Spent: 1h 10m
> Remaining Estimate: 0h
>
> Currenlty, ApiSurface.getSdkApiSurface() is highly specialized and also not
> correct.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
