Conversation
Review Summary by QodoIntroduce Url value object with query parameter validation
WalkthroughsDescription• Introduce Url value object for validated URL handling - Encapsulates URL validation logic with Jackson serialization support - Adds validation to reject URLs containing query parameters (question marks) • Refactor CounterpartConfiguration to use Url type - Replace String url field with Url url for type safety - Move validation logic from static method to Url class • Update all URL usages across codebase - Call .getValue() to extract string from Url objects - Use Url.ofTrusted() for internally-generated URLs - Use Url.validate() for external URL validation • Improve null/blank checking with isBlank() method Diagramflowchart LR
A["String url field"] -->|"Refactored to"| B["Url value object"]
B -->|"Encapsulates"| C["URL validation logic"]
C -->|"Adds check for"| D["Query parameters"]
B -->|"Provides"| E["getValue method"]
E -->|"Used in"| F["ConformanceParty & ConformanceOrchestrator"]
File Changes1. core/src/main/java/org/dcsa/conformance/core/toolkit/Url.java
|
Code Review by Qodo
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| private String _getCounterpartUrl( | ||
| CounterpartConfiguration counterpartConfiguration, String method, String path) { | ||
| var url = counterpartConfiguration.getUrl() + path; | ||
| var url = counterpartConfiguration.getUrl().getValue() + path; |
There was a problem hiding this comment.
1. Null url npe risk 🐞 Bug ☼ Reliability
ConformanceParty._getCounterpartUrl dereferences counterpartConfiguration.getUrl().getValue() without checking for null, but other code paths treat the URL as nullable, so a missing/omitted URL can now crash with a NullPointerException. The same dereference pattern exists in sandbox components when composing notification/config URLs.
Agent Prompt
### Issue description
`CounterpartConfiguration.url` is nullable, but several code paths dereference it via `getUrl().getValue()` / `getUrl().isBlank()` without null checks. This can crash the orchestrator/party logic with `NullPointerException` when URL is omitted/missing.
### Issue Context
The code already treats URL as optional in at least one place (`asyncCounterpartNotification` checks `counterpartUrl != null`). After the refactor to a `Url` wrapper, dereferencing `getValue()` becomes unsafe if `url` is null.
### Fix Focus Areas
- core/src/main/java/org/dcsa/conformance/core/party/CounterpartConfiguration.java[11-19]
- core/src/main/java/org/dcsa/conformance/core/party/ConformanceParty.java[300-303]
- sandbox/src/main/java/org/dcsa/conformance/sandbox/ConformanceOrchestrator.java[192-216]
- sandbox/src/main/java/org/dcsa/conformance/sandbox/ConformanceWebuiHandler.java[211-220]
### What to change
- Ensure `CounterpartConfiguration.url` is never null (e.g., initialize to `Url.ofTrusted("")`, enforce non-null in setters/constructors, and/or add Jackson defaulting for missing fields).
- Add defensive handling where URLs are used (e.g., throw a `UserFacingException` with a clear message when URL is required but missing, rather than NPE).
- Prefer a single helper like `String baseUrl = Optional.ofNullable(getUrl()).map(Url::getValue).orElse("");` where empty is acceptable.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
Pull request overview
This PR updates URL handling across the conformance sandbox by replacing raw String URLs with a validated Url value object, and by centralizing URL validation logic in core.
Changes:
- Introduces
core.toolkit.Url(Jackson-serializable) and moves URL validation into it. - Updates
CounterpartConfiguration.urlfromStringtoUrland adjusts URL usages (getValue(),isBlank()) accordingly. - Updates sandbox UI handler and orchestrator HTTP request construction to use the new
Urltype.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| sandbox/src/main/java/org/dcsa/conformance/sandbox/ConformanceWebuiHandler.java | Uses Url for counterpart URLs, validates external party URL via Url.validate, and returns URL strings via getValue(). |
| sandbox/src/main/java/org/dcsa/conformance/sandbox/ConformanceOrchestrator.java | Builds notification URL using counterpartConfiguration.getUrl().getValue(). |
| core/src/main/java/org/dcsa/conformance/core/toolkit/Url.java | New URL value object with validation and Jackson string serialization. |
| core/src/main/java/org/dcsa/conformance/core/party/CounterpartConfiguration.java | Changes url field type to Url and removes legacy validation method. |
| core/src/main/java/org/dcsa/conformance/core/party/ConformanceParty.java | Updates URL checks/concatenation to use Url (isBlank, getValue). |
Comments suppressed due to low confidence (1)
sandbox/src/main/java/org/dcsa/conformance/sandbox/ConformanceWebuiHandler.java:408
- When the orchestrator is inactive, orchestratorUrl is derived by trimming "/party//api" from externalPartyCounterpartConfig.getUrl(). If externalPartyUrl is allowed to be empty for the role (allowEmptyUrl=true) or the provided URL does not match the expected suffix, this substring calculation can throw StringIndexOutOfBoundsException or produce an incorrect orchestratorUrl. Consider skipping this derivation when the URL is blank, and/or verifying the expected suffix before trimming (and failing with a user-facing error if it’s not present).
.setOrchestratorUrl(
externalPartyCounterpartConfig
.getUrl()
.getValue()
.substring(
0,
externalPartyCounterpartConfig.getUrl().length()
- "/party/%s/api"
.formatted(externalPartyCounterpartConfig.getName())
.length()));
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| CounterpartConfiguration externalPartyCounterpartConfig = | ||
| sandboxConfiguration.getExternalPartyCounterpartConfiguration(); | ||
|
|
||
| ObjectNode jsonSandboxConfig = OBJECT_MAPPER | ||
| .createObjectNode() | ||
| .put(SANDBOX_ID, sandboxConfiguration.getId()) | ||
| .put("sandboxName", sandboxConfiguration.getName()) | ||
| .put("sandboxUrl", sandboxPartyCounterpartConfig.getUrl()) | ||
| .put("sandboxUrl", sandboxPartyCounterpartConfig.getUrl().getValue()) | ||
| .put("sandboxAuthHeaderName", sandboxConfiguration.getAuthHeaderName()) | ||
| .put("sandboxAuthHeaderValue", sandboxConfiguration.getAuthHeaderValue()) | ||
| .put("externalPartyUrl", externalPartyCounterpartConfig.getUrl()) | ||
| .put("externalPartyUrl", externalPartyCounterpartConfig.getUrl().getValue()) | ||
| .put("externalPartyAuthHeaderName", externalPartyCounterpartConfig.getAuthHeaderName()) |
There was a problem hiding this comment.
_getSandboxConfig() dereferences externalPartyCounterpartConfig.getUrl() without checking if getExternalPartyCounterpartConfiguration() returned null. For all-in-one sandboxes (both parties in the sandbox), getExternalPartyCounterpartConfiguration() can be null, which will cause a NullPointerException when building the JSON response. Consider guarding for null and returning empty strings (or omitting the external-party fields) in that case.
| waitingForBiConsumer.accept( | ||
| partyName, "perform action '%s'".formatted(nextAction.getActionTitle())); | ||
|
|
||
| asyncWebClient.accept( | ||
| new ConformanceWebRequest( | ||
| "GET", | ||
| counterpartConfiguration.getUrl() + "/conformance/notification", | ||
| counterpartConfiguration.getUrl().getValue() + "/conformance/notification", | ||
| Collections.emptyMap(), | ||
| counterpartConfiguration.getAuthHeaderName().isBlank() |
There was a problem hiding this comment.
notifyNextActionParty() now concatenates counterpartConfiguration.getUrl().getValue() without checking for null/blank URLs. Some standards/roles allow an empty external-party URL (no notifications), and Url can also be null if missing from persisted config; either case will lead to an invalid request or NullPointerException here. Consider skipping the outbound notification when the URL is null/blank (similar to other no-notification paths) or treating that counterpart as manual mode for notification purposes.
| private String _getCounterpartUrl( | ||
| CounterpartConfiguration counterpartConfiguration, String method, String path) { | ||
| var url = counterpartConfiguration.getUrl() + path; | ||
| var url = counterpartConfiguration.getUrl().getValue() + path; |
There was a problem hiding this comment.
_getCounterpartUrl() assumes counterpartConfiguration.getUrl() is non-null and calls getValue() unconditionally. Elsewhere (e.g., asyncCounterpartNotification) the code explicitly allows a null/blank URL, so this can still throw NullPointerException if a request is attempted with an unconfigured counterpart. Consider failing fast with a clear exception when url is null/blank, or ensuring url is always initialized (e.g., Url.ofTrusted("") at deserialization time).
| var url = counterpartConfiguration.getUrl().getValue() + path; | |
| var counterpartUrl = counterpartConfiguration.getUrl(); | |
| var counterpartUrlValue = counterpartUrl == null ? null : counterpartUrl.getValue(); | |
| if (counterpartUrlValue == null || counterpartUrlValue.isBlank()) { | |
| throw new IllegalStateException( | |
| "Counterpart URL is not configured for counterpart '" | |
| + counterpartConfiguration.getName() | |
| + "' when creating request " | |
| + method | |
| + " " | |
| + path); | |
| } | |
| var url = counterpartUrlValue + path; |
| public static void validate(String url, boolean allowHttpLocalhost, boolean allowEmpty) throws UserFacingException { | ||
| if (url.isEmpty()) { | ||
| if (allowEmpty) { | ||
| return; | ||
| } | ||
| throw new UserFacingException("The application base URL must not be empty: connecting to your application is not optional for this standard and role."); | ||
| } |
There was a problem hiding this comment.
Url.validate() calls url.isEmpty() without a null check. Since Url.of(...) and validate(...) are public APIs, passing a null url will currently throw a NullPointerException rather than a UserFacingException. Consider explicitly handling null (treat as empty when allowEmpty=true, otherwise throw a UserFacingException with a clear message).
| public static Url of(String url, boolean allowHttpLocalhost, boolean allowEmpty) throws UserFacingException { | ||
| validate(url, allowHttpLocalhost, allowEmpty); | ||
| return new Url(url); | ||
| } | ||
|
|
||
| /** | ||
| * Creates a URL without validation. Use only when the URL comes from a trusted source | ||
| * or when deserializing from JSON (validation should happen at the application boundary). | ||
| * | ||
| * @param url the URL string | ||
| * @return a Url instance | ||
| */ | ||
| @JsonCreator | ||
| public static Url ofTrusted(String url) { | ||
| return new Url(url != null ? url : ""); | ||
| } | ||
|
|
||
| /** | ||
| * Validates a URL string without creating a Url object. | ||
| * | ||
| * @param url the URL string to validate | ||
| * @param allowHttpLocalhost whether to allow HTTP and localhost URLs | ||
| * @param allowEmpty whether to allow empty URLs | ||
| * @throws UserFacingException if the URL is invalid | ||
| */ | ||
| public static void validate(String url, boolean allowHttpLocalhost, boolean allowEmpty) throws UserFacingException { | ||
| if (url.isEmpty()) { | ||
| if (allowEmpty) { | ||
| return; | ||
| } | ||
| throw new UserFacingException("The application base URL must not be empty: connecting to your application is not optional for this standard and role."); | ||
| } | ||
| try { | ||
| new URI(url); | ||
| } catch (URISyntaxException e) { | ||
| throw new UserFacingException("The URL format is not correct", e); | ||
| } | ||
| if (url.contains("?")) { | ||
| throw new UserFacingException("The URL must not contain query parameters (question marks)"); | ||
| } | ||
| if (!allowHttpLocalhost) { | ||
| String lowerCaseUrl = url.toLowerCase(); | ||
| if (!lowerCaseUrl.startsWith("https://")) { | ||
| throw new UserFacingException("The URL must use HTTPS instead of plain HTTP"); | ||
| } | ||
| if (Stream.of("https://localhost/", "https://localhost:", "https://127.") | ||
| .anyMatch(lowerCaseUrl::startsWith)) { | ||
| throw new UserFacingException("The URL cannot be set to a local address"); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The new Url value object introduces non-trivial validation behavior (HTTPS vs localhost rules, query-parameter rejection, allowEmpty handling). Adding focused unit tests for Url.validate/of (including Jackson round-trip) would help prevent regressions and clarify the expected rules.
pedrocarvalhodcsa
left a comment
pedrocarvalhodcsa
left a comment
There was a problem hiding this comment.
All seems good. Just take a look at the comments from Copilot and the other AI bot to see if they make sense and they need changes
| /** | ||
| * Returns the length of the URL string. | ||
| */ | ||
| public int length() { |
There was a problem hiding this comment.
is this method used?
|
|
||
|
|
||
| @Override | ||
| public String toString() { |
There was a problem hiding this comment.
Do we need the toString, equals and hashCode?
No description provided.