Skip to content

Remove feign#55

Merged
DanVanAtta merged 5 commits intomainfrom
remove-feign
Apr 26, 2026
Merged

Remove feign#55
DanVanAtta merged 5 commits intomainfrom
remove-feign

Conversation

@DanVanAtta
Copy link
Copy Markdown
Member

No description provided.

Copilot AI review requested due to automatic review settings April 26, 2026 04:55
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR removes the Feign-based GitHub HTTP client usage by reimplementing GithubApiClient on Java’s built-in java.net.http.HttpClient with Gson JSON parsing, and adds an integration characterization test + production-sample fixture to ensure the maps listing endpoint output remains stable.

Changes:

  • Replace Feign usage in GithubApiClient with java.net.http.HttpClient + Gson, and update the GithubClient.build() factory accordingly.
  • Update Gradle dependencies to drop Feign artifacts and add Gson; set test JVM timezone to UTC for deterministic timestamp behavior.
  • Add a characterization integration test and a production-sample DB fixture for the maps listing endpoint.

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/main/java/org/triplea/http/client/github/GithubClient.java Switch factory construction away from the Feign builder to direct client instantiation.
src/main/java/org/triplea/http/client/github/GithubApiClient.java Core change: remove Feign calls and implement GitHub REST calls via java.net.http + Gson.
build.gradle.kts Remove Feign dependencies, add Gson, and set test timezone to UTC.
src/testInteg/java/org/triplea/maps/listing/MapListingCharacterizationTest.java New integration characterization test asserting the maps listing response matches a production-derived baseline.
src/testInteg/resources/datasets/map_index_prod_sample.yml New DBRider dataset representing a production sample of the first 20 maps.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

}

private String sendGet(String path) {
HttpRequest.Builder request = HttpRequest.newBuilder().uri(URI.create(baseUri + path)).GET();
Copy link

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

URI.create(baseUri + path) relies on string concatenation and is brittle if baseUri ever includes a trailing slash or a path segment (it can produce double slashes or missing separators). Using baseUri.resolve(path) (or URIBuilder-style joining) will construct the URL more reliably.

Suggested change
HttpRequest.Builder request = HttpRequest.newBuilder().uri(URI.create(baseUri + path)).GET();
final URI normalizedBaseUri =
baseUri.toString().endsWith("/") ? baseUri : URI.create(baseUri.toString() + "/");
final URI requestUri = normalizedBaseUri.resolve(path.startsWith("/") ? path.substring(1) : path);
HttpRequest.Builder request = HttpRequest.newBuilder().uri(requestUri).GET();

Copilot uses AI. Check for mistakes.
Comment on lines +71 to +74
@Override
public Instant getLatestCommitDate(String repoName, String branchName) {
return githubApiFeignClient.getBranchInfo(org, repoName, branchName).getLastCommitDate();
String body = sendGet(String.format(BRANCHES_PATH, org, repoName, branchName));
return GSON.fromJson(body, BranchInfoResponse.class).getLastCommitDate();
Copy link

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

repoName and especially branchName are interpolated directly into the URL path. Git branch names can legally contain characters like / and spaces, which must be URL-encoded for this endpoint; otherwise requests can hit the wrong path or 404. Encode path segments (or use URI.resolve with proper encoding) before sending the request.

Copilot uses AI. Check for mistakes.
Comment on lines +18 to +23
* production data (first 20 maps alphabetically as of April 2026), giving us a stable reference
* point. If the server's response ever diverges from this baseline, the test will catch it.
*
* <p>The fixture {@code map_index_prod_sample.yml} was hand-crafted from
* https://prod.triplea-game.org/support/maps/listing. When the fixture needs to be refreshed, fetch
* the endpoint again, pick the first 20 maps, and update both the YAML and the expected list below.
Copy link

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Javadoc says the fixture uses the "first 20 maps alphabetically", but the assertion uses containsExactlyInAnyOrder, so ordering changes won’t be detected. Either clarify that order is intentionally not asserted, or change the assertion (and expected list) to validate ordering if alphabetical order is part of the endpoint contract.

Suggested change
* production data (first 20 maps alphabetically as of April 2026), giving us a stable reference
* point. If the server's response ever diverges from this baseline, the test will catch it.
*
* <p>The fixture {@code map_index_prod_sample.yml} was hand-crafted from
* https://prod.triplea-game.org/support/maps/listing. When the fixture needs to be refreshed, fetch
* the endpoint again, pick the first 20 maps, and update both the YAML and the expected list below.
* production data, giving us a stable reference point. If the server's response ever diverges from
* this baseline, the test will catch it.
*
* <p>The fixture {@code map_index_prod_sample.yml} was hand-crafted from
* https://prod.triplea-game.org/support/maps/listing using 20 production entries selected from the
* alphabetically sorted endpoint as of April 2026. This test intentionally validates the returned
* set of map entries, not their order. When the fixture needs to be refreshed, fetch the endpoint
* again, select 20 representative entries, and update both the YAML and the expected list below.

Copilot uses AI. Check for mistakes.
request.header("Authorization", "token " + authToken);
}
try {
return httpClient.send(request.build(), HttpResponse.BodyHandlers.ofString()).body();
Copy link

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sendGet ignores the HTTP status code and always returns the response body. If GitHub returns 4xx/5xx (rate limit, auth error, missing repo/branch), the subsequent JSON parsing will likely fail with a confusing error and hide the real cause. Consider checking response.statusCode() and throwing an exception that includes the status (and ideally the response body) when it’s not 2xx.

Suggested change
return httpClient.send(request.build(), HttpResponse.BodyHandlers.ofString()).body();
HttpResponse<String> response =
httpClient.send(request.build(), HttpResponse.BodyHandlers.ofString());
int statusCode = response.statusCode();
if (statusCode < 200 || statusCode >= 300) {
throw new RuntimeException(
"GitHub API request failed: "
+ path
+ ", status: "
+ statusCode
+ ", response body: "
+ response.body());
}
return response.body();

Copilot uses AI. Check for mistakes.
@DanVanAtta DanVanAtta merged commit 1d421d3 into main Apr 26, 2026
1 check passed
@DanVanAtta DanVanAtta deleted the remove-feign branch April 26, 2026 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants