[DT-3882] Standardize service URL construction

[kevinmarete]

Addresses

https://broadworkbench.atlassian.net/browse/DT-3882

Summary

• Normalize Local, Sam, and ECM base URLs to use one trailing slash.
• Standardize endpoint construction and remove one-off URL handling.
• Add coverage for zero, one, and multiple trailing slashes.
• Update health-check and integration tests to verify canonical URLs.

---

Have you read CONTRIBUTING.md lately? If not, do that first.

• Label PR with a Jira ticket number and include a link to the ticket
• Label PR with a security risk modifier [no, low, medium, high]
• PR describes scope of changes
• Get a minimum of one thumbs worth of review, preferably two if enough team members are available
• Get PO sign-off for all non-trivial UI or workflow changes
• Verify all tests go green
• Test this change deployed correctly and works on dev environment after deployment

[Copilot]

🟡 Not ready to approve

normalizeBaseUrl currently permits blank base URLs (only @NotNull), which can silently produce malformed downstream URLs instead of failing fast on misconfiguration.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

This PR standardizes how service base URLs are handled in ServicesConfiguration so downstream URL construction (User location headers, ECM/Sam endpoints, health checks) is consistent and avoids double-slash path issues.

Changes:

• Normalize localURL, samUrl, and ecmUrl at set-time to always end with a single trailing /, and remove leading / from path constants.
• Introduce getSamStatusUrl() and update SamHealthCheck (and tests) to use it.
• Update tests (unit + integration) to align with the standardized URL construction and expected Location header.

File summaries

File	Description
src/test/java/org/broadinstitute/consent/integration/user/UserTests.java	Updates WireMock stubbing to match the standardized ECM RAS path (no double-slash matching).
src/test/java/org/broadinstitute/consent/http/resources/UserResourceTest.java	Adjusts localURL stubbing to include trailing / and asserts the created Location URI.
src/test/java/org/broadinstitute/consent/http/health/SamHealthCheckTest.java	Updates tests to stub getSamStatusUrl() instead of building .../status from getSamUrl().
src/test/java/org/broadinstitute/consent/http/configurations/ServicesConfigurationTest.java	Adds a broader normalization-focused test validating many derived URLs from normalized bases.
src/main/java/org/broadinstitute/consent/http/resources/UserResource.java	Simplifies Location URI creation to rely on normalized getLocalURL().
src/main/java/org/broadinstitute/consent/http/health/SamHealthCheck.java	Switches health check target URL construction to configuration.getSamStatusUrl().
src/main/java/org/broadinstitute/consent/http/configurations/ServicesConfiguration.java	Implements base URL normalization in setters, adds getSamStatusUrl(), and standardizes path constants for safe concatenation.

Review details

• Files reviewed: 7/7 changed files
• Comments generated: 1
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Not ready to approve

ServicesConfiguration’s base URL normalization throws a NullPointerException for null inputs; this should be a consistent, explicit IllegalArgumentException to improve configuration failure clarity.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (1)

src/main/java/org/broadinstitute/consent/http/configurations/ServicesConfiguration.java:182

• normalizeBaseUrl currently uses Objects.requireNonNull(...), which throws a NullPointerException for null inputs. For configuration validation (and consistency with the blank check), it’s better to throw an IllegalArgumentException with a clear message instead of an NPE.

  private static String normalizeBaseUrl(String baseUrl) {
    String normalizedBaseUrl =
        Objects.requireNonNull(baseUrl, "Service base URL must not be null").strip();
    if (normalizedBaseUrl.isEmpty()) {
      throw new IllegalArgumentException("Service base URL must not be blank");
    }

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
96.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[Copilot]

🟢 Ready to approve

The URL normalization and centralized endpoint construction are consistent across the codebase and are backed by targeted unit/integration test updates covering the key edge cases.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.