Skip to content

Give the script its callback URL without a query string, as the .NET builder does - #143

Merged
Automation51D merged 2 commits into
mainfrom
fix/javascript-url-no-query
Sep 18, 2026
Merged

Automation51D merged 2 commits into
mainfrom
fix/javascript-url-no-query

Conversation

@jwrosewell

@jwrosewell jwrosewell commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Problem

Every builder must render the same script for the same request. The .NET
JavaScript builder gives the script a callback URL made of protocol, host and
endpoint only (urlString = $"{protocol}://{host}{endpoint}" in
JavaScriptBuilderElement.cs),
and passes the query evidence separately as the serialized parameters object.

The Java builder also appended every query evidence value to the URL
(JavaScriptBuilderElement.java lines 158 to 161).
A script rendered with query.user-agent=iPhone and
query.id.usage=non-marketing requested
https://example.com/json?id.usage=non-marketing&user-agent=iPhone, where
.NET renders https://example.com/json. The Python builder had the same
fault and was fixed in
pipeline-python pull request 87.

What changed

  1. JavaScriptBuilderElement.getUrl builds protocol, host, context root and
    endpoint, with no query string. The query evidence still reaches the script
    through the serialized parameters object, which is unchanged.
  2. getQueryParams had no other caller, so it is removed.
  3. A new test class, JavaScriptUrlTests, renders the script without a
    browser.
    • Url_CarriesNoQueryEvidence checks that every URL the script requests
      is https://example.com/json and that the parameters object still holds
      user-agent and id.usage.
    • Url_KeepsContextRoot checks that a context root of /app/ gives
      https://example.com/app/json.

Verification

Without the change, both new tests fail.

JavaScriptUrlTests.Url_CarriesNoQueryEvidence:115 The script's URL must carry no query string. ==> expected: <https://example.com/json> but was: <https://example.com/json?id.usage=non-marketing&user-agent=iPhone>
JavaScriptUrlTests.Url_KeepsContextRoot:145 expected: <https://example.com/app/json> but was: <https://example.com/app/json?id.usage=non-marketing&user-agent=iPhone>
Tests run: 2, Failures: 2, Errors: 0, Skipped: 0

With the change, the module's tests that need no browser pass, both with the
template this repository records and with
javascript-templates
main at ede377cc swapped in locally.

Tests run: 4, Failures: 0, Errors: 0, Skipped: 0 - in CookieTests
Tests run: 14, Failures: 0, Errors: 0, Skipped: 0 - in JavaScriptBuilderElementBuilderTests
Tests run: 2, Failures: 0, Errors: 0, Skipped: 0 - in JavaScriptUrlTests
Tests run: 20, Failures: 0, Errors: 0, Skipped: 0
BUILD SUCCESS

Outstanding

JavaScriptBuilderTests starts Chrome and was not run locally, so it will run
in CI. Its URL test (JavaScriptBuilderElement_VerifyUrl) expects
https://localhost/json and its parameter tests read the values from the
script body, so neither depends on the query string that this change removes.

Overlap with other work

Three drafts change
pipeline.javascriptbuilder/.../JavaScriptBuilderElement.java, being this
one, the object name draft
#144 and the session
id and sequence draft
#145. Every pair was
merged locally, in both directions, and all six merges are clean. Merging
all three together is clean too, and the result keeps one
import java.util.regex.Pattern, one getUrl(protocol, host) with no
caller left passing a query string, and the int sequence from #145.

That three way merge was then built and tested with
mvn -o -pl pipeline.javascriptbuilder -am test -Dtest='!JavaScriptBuilderTests',
leaving out the test that starts a browser:

Tests run: 4, Failures: 0, Errors: 0, Skipped: 0 - in CookieTests
Tests run: 14, Failures: 0, Errors: 0, Skipped: 0 - in JavaScriptBuilderElementBuilderTests
Tests run: 22, Failures: 0, Errors: 0, Skipped: 0 - in JavaScriptObjectNameTests
Tests run: 2, Failures: 0, Errors: 0, Skipped: 0 - in JavaScriptUrlTests
Tests run: 32, Failures: 0, Errors: 0, Skipped: 0 - in SessionIdAndSequenceTests
Tests run: 74, Failures: 0, Errors: 0, Skipped: 0
BUILD SUCCESS

So the three can land in any order without a merge.

The same change in the other ports

CI on this branch, 17 September 2026

The "Pull Requests" workflow was dispatched on this branch with dryrun=true,
which builds the branch and merges nothing. Run
35189330497
ran on 157cb4c, the head of this branch, and concluded success with all
15 jobs green across the 11 Java and operating system configurations. In the
Ubuntu_Java_21 job the Maven totals are 627 tests run, 0 failures, 0 errors
and 6 skipped, with no error lines in the log, and the new
fiftyone.pipeline.javascriptbuilder.JavaScriptUrlTests runs 2 tests and
passes both.

This branch is three commits behind main and merges into it cleanly.
The nightly checks out the pull request, merges main into it and builds
that before it merges anything, so the combination is tested there.

….NET builder does

The Java builder appended every query evidence value to the URL the rendered
script requests, so the same request gave a different script in Java than in
.NET. The URL is now protocol, host, context root and endpoint only, and the
query evidence reaches the script through the serialized parameters object
as before. getQueryParams had no other caller and is removed.
@jwrosewell

Copy link
Copy Markdown
Contributor Author

Proven in CI

Full build and test run on this branch: https://github.com/51Degrees/pipeline-java/actions/runs/35189330497

A draft pull request only runs the link lint, so the Pull Requests workflow was dispatched against this branch as a dry run to get real evidence. Nothing was merged, and the log shows why: with no pull request based on this branch the run builds the branch on its own as pull request 0, and the checkout and completion steps both print "Not running for a PR".

All 15 jobs finished green, being 11 build and test jobs (Java 8, 11, 17 and 21 on Ubuntu and Windows, and Java 11, 17 and 21 on macOS), the pull request lookup, the configure step, the performance comparison and the completion step.

Reading the job logs rather than the conclusions, each job reports 23 Maven module summaries, and on Java 11 and later they add up to 627 tests run with 0 failures, 0 errors and 0 skipped. Java 8 runs 612, the difference being the tests that need a later Java. The log contains no "BUILD FAILURE", no "FAILURES!", no "ERRORS!" and no summary line with a non-zero failure or error count.

The pull request has not been marked ready and no review has been requested.

@Automation51D
Automation51D merged commit 88ab51c into main Sep 18, 2026
11 of 14 checks passed
@Automation51D
Automation51D deleted the fix/javascript-url-no-query branch September 18, 2026 04:54
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