Skip to content

url encoding changes #14

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
dkkplk wants to merge 1 commit into
base: master
Choose a base branch
from
Open

url encoding changes #14

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 31 additions & 1 deletion reportium-java/src/main/java/com/perfecto/reportium/client/PerfectoReportiumClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,15 @@
import com.perfecto.reportium.test.result.TestResult;
import org.apache.commons.lang3.StringUtils;
import org.apache.commons.lang3.tuple.Pair;
import org.apache.http.client.utils.URIBuilder;
import org.openqa.selenium.HasCapabilities;
import org.openqa.selenium.JavascriptExecutor;
import org.openqa.selenium.WebDriver;

import java.util.*;
import java.util.logging.Logger;
import java.net.URI;
import java.net.URISyntaxException;

import static com.perfecto.reportium.model.util.ExecutionContextPopulator.EQUALS;

Expand Down Expand Up @@ -218,8 +221,35 @@ public String getReportUrl() {
if (value == null) {
return null;
}

try {
String reportUrl = String.valueOf(value);
URI originalUri = new URI(reportUrl);

URIBuilder uriBuilder = new URIBuilder()
.setScheme(originalUri.getScheme())
.setHost(originalUri.getHost())
.setPort(originalUri.getPort())
.setPath(originalUri.getPath());

// Parse and add query parameters - URIBuilder will encode them
if (originalUri.getQuery() != null) {
String[] pairs = originalUri.getQuery().split("&");
for (String pair : pairs) {
String[] keyValue = pair.split("=", 2);
if (keyValue.length == 2) {
uriBuilder.addParameter(keyValue[0], keyValue[1]);
} else {
uriBuilder.addParameter(keyValue[0], "");
}
}
}

Comment on lines +229 to +247
Copy link

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

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

getQuery() returns the decoded query string, so splitting on '&' can break when a parameter value contains an encoded ampersand (%26) (it will be decoded to '&' before you split). Prefer parsing the raw query (getRawQuery) or, better, construct URIBuilder directly from the original URI/string so it handles query parsing/encoding correctly.

Suggested change
URIBuilder uriBuilder = new URIBuilder()
.setScheme(originalUri.getScheme())
.setHost(originalUri.getHost())
.setPort(originalUri.getPort())
.setPath(originalUri.getPath());
// Parse and add query parameters - URIBuilder will encode them
if (originalUri.getQuery() != null) {
String[] pairs = originalUri.getQuery().split("&");
for (String pair : pairs) {
String[] keyValue = pair.split("=", 2);
if (keyValue.length == 2) {
uriBuilder.addParameter(keyValue[0], keyValue[1]);
} else {
uriBuilder.addParameter(keyValue[0], "");
}
}
}
// Let URIBuilder parse the full original URI, including query parameters,
// to avoid manual splitting of the decoded query string.
URIBuilder uriBuilder = new URIBuilder(originalUri);

Copilot uses AI. Check for mistakes.
Comment on lines +229 to +247
Copy link

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

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

This rebuilds the URI from scheme/host/port/path only, which drops other components like fragment (#...), user-info, and any original path encoding (getPath() is decoded). If the report URL can contain these, the returned URL will be altered. Using URIBuilder(originalUri) (or URIBuilder(reportUrl)) and only adjusting encoding as needed will preserve all components.

Suggested change
URIBuilder uriBuilder = new URIBuilder()
.setScheme(originalUri.getScheme())
.setHost(originalUri.getHost())
.setPort(originalUri.getPort())
.setPath(originalUri.getPath());
// Parse and add query parameters - URIBuilder will encode them
if (originalUri.getQuery() != null) {
String[] pairs = originalUri.getQuery().split("&");
for (String pair : pairs) {
String[] keyValue = pair.split("=", 2);
if (keyValue.length == 2) {
uriBuilder.addParameter(keyValue[0], keyValue[1]);
} else {
uriBuilder.addParameter(keyValue[0], "");
}
}
}
URIBuilder uriBuilder = new URIBuilder(originalUri);

Copilot uses AI. Check for mistakes.
return uriBuilder.build().toString();

return String.valueOf(value);
} catch (URISyntaxException e) {
throw new ReportiumException("Failed to create report URL", e);
}
Comment on lines +251 to +252
Copy link

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

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

Previously this method returned String.valueOf(value) even if the capability value wasn't a valid URI. With the new parsing, malformed/partially-formed values will now throw ReportiumException, which is a behavior change for callers. If strict validation isn't required, consider falling back to returning the raw capability string on URISyntaxException (or only applying encoding when the URL parses cleanly).

Suggested change
throw new ReportiumException("Failed to create report URL", e);
}
// Fallback to previous behavior: return the raw capability string
return String.valueOf(value);

Copilot uses AI. Check for mistakes.
}

private void executeScript(String script, Map<String, Object> params) {
Expand Down
25 changes: 24 additions & 1 deletion reportium-java/src/test/java/com/perfecto/reportium/client/PerfectoReportiumClientTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
* Test case for {@link PerfectoReportiumClient}
*/
public class PerfectoReportiumClientTest {
public static final String EXPECTED_REPORT_URL = "https://tenant.reporting.perfectomobile.com/library?externalId%5B0%5D=32f76d03";

/**
* Marker interface for EasyMock to simulate a RemoteWebDriver.
Expand Down Expand Up @@ -48,7 +49,7 @@ public void testWebDriverInteraction() {
client.testStep("step1");
client.testStep("step2");
client.testStop(TestResultFactory.createFailure("Just because", new Throwable("Yikes")));
assertEquals(client.getReportUrl(), "link");
assertEquals(client.getReportUrl(), "/link");
Copy link

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

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

The test sets the executionReportUrl capability to the relative string "link" but asserts that getReportUrl() returns "/link". Unless getReportUrl intentionally normalizes relative URLs by adding a leading slash, this assertion looks inconsistent with the input and may fail/flap. Consider either returning "/link" from the mock capability or asserting the unmodified relative value.

Suggested change
assertEquals(client.getReportUrl(), "/link");
assertEquals(client.getReportUrl(), "link");

Copilot uses AI. Check for mistakes.

verify(webDriverMock, capabilitiesMock);
}
Expand All @@ -73,4 +74,26 @@ public void testStop_nullTestContext() {
public void testRequiredWebDriver() {
new PerfectoExecutionContext.PerfectoExecutionContextBuilder().build();
}

@Test
public void testGetReportUrl_WithQueryParameters() {
DriverWithCapabilities webDriverMock = createMock(DriverWithCapabilities.class);
PerfectoExecutionContext context = new PerfectoExecutionContext.PerfectoExecutionContextBuilder()
.withWebDriver(webDriverMock)
.build();
PerfectoReportiumClient client = new PerfectoReportiumClient(context);
Capabilities capabilitiesMock = createMock(Capabilities.class);

String urlWithParams = "https://tenant.reporting.perfectomobile.com/library?externalId[0]=32f76d03";
expect(webDriverMock.getCapabilities()).andReturn(capabilitiesMock);
expect(capabilitiesMock.getCapability(eq(Constants.Capabilities.executionReportUrl))).andReturn(urlWithParams);

replay(webDriverMock, capabilitiesMock);

String actualUrl = client.getReportUrl();
assertEquals(actualUrl, EXPECTED_REPORT_URL);

verify(webDriverMock, capabilitiesMock);
}

}
Loading