Skip to content

Commit d8c2328

Browse files
lukekimlukekim
committed
chore: Add CI quality checks, address PR review comments
- Add SpotBugs, OWASP dependency-check, JaCoCo, Enforcer, Checkstyle plugins - Add 'quality' CI job running static analysis and dependency scanning - Add JaCoCo coverage reporting to build_multi_os CI job - Make 'closed' field volatile for thread-safety - Synchronize close() to prevent race with reset()/buildFlightClient() - Add channel cleanup (shutdownNow) on buildFlightClient() failure - Remove unused VectorSchemaRoot import in ResetTest - Fix stale/misleading comments in ResetTest
1 parent beea47a commit d8c2328

4 files changed

Lines changed: 163 additions & 34 deletions

File tree

‎.github/workflows/build.yaml‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,14 @@ jobs:
9797
path: target/surefire-reports/
9898
retention-days: 7
9999

100+
- name: Upload coverage report
101+
if: always()
102+
uses: actions/upload-artifact@v4
103+
with:
104+
name: coverage-report-${{ matrix.os }}
105+
path: target/site/jacoco/
106+
retention-days: 7
107+
100108
build:
101109
runs-on: ubuntu-latest
102110
timeout-minutes: 20
@@ -178,3 +186,40 @@ jobs:
178186
name: test-results-jdk${{ matrix.java.version }}-${{ matrix.java.distribution }}
179187
path: target/surefire-reports/
180188
retention-days: 7
189+
190+
quality:
191+
name: Code quality checks
192+
runs-on: ubuntu-latest
193+
timeout-minutes: 30
194+
steps:
195+
- uses: actions/checkout@v4
196+
197+
- name: Set up JDK 17 (Oracle)
198+
uses: actions/setup-java@v4
199+
with:
200+
java-version: 17
201+
distribution: oracle
202+
cache: maven
203+
204+
- name: Build
205+
run: mvn install -DskipTests=true -Dgpg.skip -B -V
206+
207+
- name: Maven Enforcer
208+
run: mvn validate -B
209+
210+
- name: SpotBugs
211+
run: mvn spotbugs:check -B
212+
213+
- name: Checkstyle
214+
run: mvn checkstyle:check -B
215+
216+
- name: OWASP Dependency-Check
217+
run: mvn dependency-check:check -B
218+
219+
- name: Upload dependency-check report
220+
if: always()
221+
uses: actions/upload-artifact@v4
222+
with:
223+
name: dependency-check-report
224+
path: target/dependency-check-report.html
225+
retention-days: 30

‎pom.xml‎

Lines changed: 75 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,8 @@
8989
<artifactId>maven-surefire-plugin</artifactId>
9090
<version>3.5.4</version>
9191
<configuration>
92-
<argLine>--add-opens=java.base/java.nio=ALL-UNNAMED</argLine>
92+
<!-- @{argLine} is set by JaCoCo's prepare-agent goal -->
93+
<argLine>@{argLine} --add-opens=java.base/java.nio=ALL-UNNAMED</argLine>
9394
</configuration>
9495
</plugin>
9596
<plugin>
@@ -148,6 +149,79 @@
148149
<waitUntil>published</waitUntil> -->
149150
</configuration>
150151
</plugin>
152+
<!-- P0: Static bug detection — null derefs, concurrency, resource leaks -->
153+
<plugin>
154+
<groupId>com.github.spotbugs</groupId>
155+
<artifactId>spotbugs-maven-plugin</artifactId>
156+
<version>4.9.3.0</version>
157+
</plugin>
158+
<!-- P0: CVE scanning for transitive dependencies (Netty, gRPC, Arrow) -->
159+
<plugin>
160+
<groupId>org.owasp</groupId>
161+
<artifactId>dependency-check-maven</artifactId>
162+
<version>12.1.1</version>
163+
<configuration>
164+
<failBuildOnCVSS>7</failBuildOnCVSS>
165+
</configuration>
166+
</plugin>
167+
<!-- P1: Test coverage reporting -->
168+
<plugin>
169+
<groupId>org.jacoco</groupId>
170+
<artifactId>jacoco-maven-plugin</artifactId>
171+
<version>0.8.13</version>
172+
<executions>
173+
<execution>
174+
<id>prepare-agent</id>
175+
<goals>
176+
<goal>prepare-agent</goal>
177+
</goals>
178+
</execution>
179+
<execution>
180+
<id>report</id>
181+
<phase>test</phase>
182+
<goals>
183+
<goal>report</goal>
184+
</goals>
185+
</execution>
186+
</executions>
187+
</plugin>
188+
<!-- P2: Build hygiene — enforce Maven/JDK versions, dependency convergence -->
189+
<plugin>
190+
<groupId>org.apache.maven.plugins</groupId>
191+
<artifactId>maven-enforcer-plugin</artifactId>
192+
<version>3.5.0</version>
193+
<executions>
194+
<execution>
195+
<id>enforce</id>
196+
<goals>
197+
<goal>enforce</goal>
198+
</goals>
199+
<configuration>
200+
<rules>
201+
<requireMavenVersion>
202+
<version>[3.6.0,)</version>
203+
</requireMavenVersion>
204+
<requireJavaVersion>
205+
<version>[11,)</version>
206+
</requireJavaVersion>
207+
<banDuplicatePomDependencyVersions/>
208+
</rules>
209+
</configuration>
210+
</execution>
211+
</executions>
212+
</plugin>
213+
<!-- P3: Code style consistency (Google Java Style) -->
214+
<plugin>
215+
<groupId>org.apache.maven.plugins</groupId>
216+
<artifactId>maven-checkstyle-plugin</artifactId>
217+
<version>3.6.0</version>
218+
<configuration>
219+
<configLocation>google_checks.xml</configLocation>
220+
<consoleOutput>true</consoleOutput>
221+
<violationSeverity>warning</violationSeverity>
222+
<failOnViolation>false</failOnViolation>
223+
</configuration>
224+
</plugin>
151225
</plugins>
152226
</build>
153227
</project>

‎src/main/java/ai/spice/SpiceClient.java‎

Lines changed: 40 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -153,7 +153,7 @@ public class SpiceClient implements AutoCloseable {
153153
private FlightSqlClient flightClient;
154154
private CredentialCallOption authCallOptions = null;
155155
private BufferAllocator allocator;
156-
private boolean closed = false;
156+
private volatile boolean closed = false;
157157

158158
// Cached retryers (immutable, thread-safe)
159159
private Retryer<ArrowReader> adbcRetryer;
@@ -275,39 +275,50 @@ private synchronized void buildFlightClient() {
275275
.maxInboundMetadataSize(Integer.MAX_VALUE);
276276
ManagedChannel channel = channelBuilder.build();
277277

278-
if (Strings.isNullOrEmpty(apiKey)) {
279-
FlightClient client = FlightGrpcUtils.createFlightClient(allocator, channel);
280-
this.flightClient = new FlightSqlClient(client);
281-
logger.debug("Flight client built (unauthenticated) - target={}", target);
282-
return;
283-
}
278+
try {
279+
if (Strings.isNullOrEmpty(apiKey)) {
280+
FlightClient client = FlightGrpcUtils.createFlightClient(allocator, channel);
281+
this.flightClient = new FlightSqlClient(client);
282+
logger.debug("Flight client built (unauthenticated) - target={}", target);
283+
return;
284+
}
284285

285-
// prepare additional headers to insert into Flight requests
286-
Map<String, String> headers = new HashMap<>();
287-
String uaString;
288-
if (Strings.isNullOrEmpty(userAgent)) {
289-
uaString = Config.getUserAgent();
290-
} else {
291-
// Prepend the user-supplied user agent string with the Spice.ai user agent
292-
uaString = userAgent + " " + Config.getUserAgent();
293-
}
294-
headers.put("User-Agent", uaString);
286+
// prepare additional headers to insert into Flight requests
287+
Map<String, String> headers = new HashMap<>();
288+
String uaString;
289+
if (Strings.isNullOrEmpty(userAgent)) {
290+
uaString = Config.getUserAgent();
291+
} else {
292+
// Prepend the user-supplied user agent string with the Spice.ai user agent
293+
uaString = userAgent + " " + Config.getUserAgent();
294+
}
295+
headers.put("User-Agent", uaString);
295296

296-
final ClientIncomingAuthHeaderMiddleware.Factory authFactory = new ClientIncomingAuthHeaderMiddleware.Factory(
297-
new ClientBearerHeaderHandler());
297+
final ClientIncomingAuthHeaderMiddleware.Factory authFactory = new ClientIncomingAuthHeaderMiddleware.Factory(
298+
new ClientBearerHeaderHandler());
298299

299-
// Combine auth and custom header middleware into a single factory
300-
final HeaderAuthMiddlewareFactory combinedFactory = new HeaderAuthMiddlewareFactory(authFactory, headers);
300+
// Combine auth and custom header middleware into a single factory
301+
final HeaderAuthMiddlewareFactory combinedFactory = new HeaderAuthMiddlewareFactory(authFactory, headers);
301302

302-
List<FlightClientMiddleware.Factory> middleware = new ArrayList<>();
303-
middleware.add(combinedFactory);
303+
List<FlightClientMiddleware.Factory> middleware = new ArrayList<>();
304+
middleware.add(combinedFactory);
304305

305-
final FlightClient client = FlightGrpcUtils.createFlightClient(allocator, channel, middleware);
306-
client.handshake(new CredentialCallOption(new BasicAuthCredentialWriter(this.appId, this.apiKey)));
307-
this.authCallOptions = authFactory.getCredentialCallOption();
308-
this.flightClient = new FlightSqlClient(client);
306+
final FlightClient client = FlightGrpcUtils.createFlightClient(allocator, channel, middleware);
307+
client.handshake(new CredentialCallOption(new BasicAuthCredentialWriter(this.appId, this.apiKey)));
308+
this.authCallOptions = authFactory.getCredentialCallOption();
309+
this.flightClient = new FlightSqlClient(client);
309310

310-
logger.debug("Flight client built (authenticated) - target={}, appId={}", target, this.appId);
311+
logger.debug("Flight client built (authenticated) - target={}, appId={}", target, this.appId);
312+
} catch (Exception e) {
313+
// Ensure the channel is shut down if client creation or handshake fails
314+
// to avoid leaking threads and file descriptors on repeated rebuild attempts.
315+
try {
316+
channel.shutdownNow();
317+
} catch (Exception suppressed) {
318+
e.addSuppressed(suppressed);
319+
}
320+
throw e;
321+
}
311322
}
312323

313324
/**
@@ -1013,7 +1024,7 @@ private boolean shouldRetry(CallStatus status) {
10131024
}
10141025

10151026
@Override
1016-
public void close() throws Exception {
1027+
public synchronized void close() throws Exception {
10171028
if (closed) {
10181029
return;
10191030
}

‎src/test/java/ai/spice/ResetTest.java‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@ of this software and associated documentation files (the "Software"), to deal
2929
import java.util.concurrent.atomic.AtomicInteger;
3030

3131
import org.apache.arrow.flight.FlightStream;
32-
import org.apache.arrow.vector.VectorSchemaRoot;
3332
import org.apache.arrow.vector.ipc.ArrowReader;
3433

3534
import junit.framework.TestCase;
@@ -74,8 +73,8 @@ public void testResetOnHttpsClient() throws Exception {
7473
}
7574

7675
/**
77-
* After reset(), close() should complete without errors
78-
* (the Flight client is already nulled out; close handles null gracefully).
76+
* After reset(), close() should complete without errors.
77+
* reset() eagerly rebuilds the Flight client, so close() tears down the new connection.
7978
*/
8079
public void testCloseAfterReset() throws Exception {
8180
SpiceClient client = SpiceClient.builder().build();
@@ -199,7 +198,7 @@ public void testTryWithResourcesAndReset() throws Exception {
199198

200199
/**
201200
* Concurrent calls to reset() from multiple threads should not throw
202-
* or corrupt state (all methods are synchronized).
201+
* or corrupt the client's internal state (reset() is expected to be thread-safe).
203202
*/
204203
public void testConcurrentResetDoesNotThrow() throws Exception {
205204
final SpiceClient client = SpiceClient.builder().build();

0 commit comments

Comments
 (0)