[java][bidi]: add getData, addDataCollector and removeDataCollector commands

[navin772]

User description

🔗 Related Issues

💥 What does this PR do?

Adds getData, addDataCollector and removeDataCollector commands as in BiDi spec:

1. w3c.github.io/webdriver-bidi#command-network-addDataCollector
2. w3c.github.io/webdriver-bidi#command-network-getData
3. w3c.github.io/webdriver-bidi#command-network-removeDataCollector

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

• New feature (non-breaking change which adds functionality and tests!)

---

PR Type

Enhancement

---

Description

• Add BiDi network data collection commands per W3C spec

• Implement addDataCollector, removeDataCollector, and getData methods

• Create parameter classes for data collection configuration

• Add comprehensive test coverage for all new functionality

---

Diagram Walkthrough

flowchart LR
  A["Network Module"] --> B["addDataCollector"]
  A --> C["removeDataCollector"]
  A --> D["getData"]
  B --> E["AddDataCollectorParameters"]
  D --> F["GetDataParameters"]
  D --> G["BytesValue"]
  E --> H["DataType enum"]
  F --> H

Loading

File Walkthrough

	Relevant files
Enhancement	Network.java Add network data collection methods                                            java/src/org/openqa/selenium/bidi/module/Network.java Add three new methods: addDataCollector, removeDataCollector, getData Import new parameter classes and JSON handling utilities Implement result mapping for getData command response +44/-0    AddDataCollectorParameters.java Create AddDataCollectorParameters class                                    java/src/org/openqa/selenium/bidi/network/AddDataCollectorParameters.java New parameter class for addDataCollector command Support for data types, max size, collector type, contexts Builder pattern with fluent API and validation +78/-0    DataType.java Add DataType enum                                                                                java/src/org/openqa/selenium/bidi/network/DataType.java New enum defining supported data types Currently supports RESPONSE data type +33/-0    GetDataParameters.java Create GetDataParameters class                                                      java/src/org/openqa/selenium/bidi/network/GetDataParameters.java New parameter class for getData command Support for data type, request ID, collector, disown flag Builder pattern with validation +59/-0
Tests	NetworkCommandsTest.java Add data collection command tests                                                java/test/org/openqa/selenium/bidi/network/NetworkCommandsTest.java Add comprehensive tests for all new data collection methods Test various parameter combinations and edge cases Verify error handling for invalid scenarios +157/-0

---

[qodo-code-review]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 5678 - Partially compliant Compliant requirements: Non-compliant requirements: Investigate and resolve ChromeDriver "ConnectFailure" on repeated instantiation. Reproduce and address environment-specific issue. Provide fix or guidance for the error scenario. Requires further human verification: Validation on the specified OS/Chrome/ChromeDriver/Selenium versions in a real environment. Confirmation that the error no longer appears across multiple driver instantiations. 1234 - Partially compliant Compliant requirements: Non-compliant requirements: Ensure click() triggers href JavaScript in Firefox as in 2.47.1. Requires further human verification: Manual/browser testing across Firefox versions to confirm behavior.

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Result Mapping The getData result mapping uses a manual Map read and a second JsonInput to parse BytesValue. Verify schema stability and that "bytes" is always present; consider direct parsing or null-safety to avoid NPE if backend omits "bytes". private final Function<JsonInput, BytesValue> getDataResultMapper = jsonInput -> { Map<String, Object> result = jsonInput.read(Map.class); @SuppressWarnings("unchecked") Map<String, Object> bytesMap = (Map<String, Object>) result.get("bytes"); try (StringReader reader = new StringReader(JSON.toJson(bytesMap)); JsonInput bytesInput = JSON.newInput(reader)) { return BytesValue.fromJson(bytesInput); } }; Validation Only maxEncodedDataSize is validated; allow-empty checks for dataTypes and collectorType values are minimal. Consider enforcing non-empty dataTypes and validating collectorType against supported values to prevent server errors. public AddDataCollectorParameters(List<DataType> dataTypes, long maxEncodedDataSize) { Require.nonNull("Data types", dataTypes); if (maxEncodedDataSize <= 0) { throw new IllegalArgumentException("Max encoded data size must be positive"); } dataTypes.forEach(dataType -> this.dataTypes.add(dataType.toString())); this.maxEncodedDataSize = maxEncodedDataSize; } public AddDataCollectorParameters collectorType(String collectorType) { this.collectorType = Require.nonNull("Collector type", collectorType); return this; } Defaults/Flags The "disown" flag is always included (defaults false). Confirm server tolerates explicit false; if optional, consider omitting when false. Also ensure request and collector IDs format are validated upstream. public Map<String, Object> toMap() { Map<String, Object> map = new HashMap<>(); map.put("dataType", dataType.toString()); map.put("request", request); if (collector != null) { map.put("collector", collector); } map.put("disown", disown); return Map.copyOf(map); }

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	✅ Simplify the JSON parsing logic Suggestion Impact: The commit removed the Json/StringReader round-trip and refactored getDataResultMapper to iterate the JsonInput object directly, reading the "bytes" field via BytesValue.fromJson(jsonInput) and skipping others. code diff: @@ -71,14 +67,18 @@ private final Function<JsonInput, BytesValue> getDataResultMapper = jsonInput -> { - Map<String, Object> result = jsonInput.read(Map.class); - @SuppressWarnings("unchecked") - Map<String, Object> bytesMap = (Map<String, Object>) result.get("bytes"); - - try (StringReader reader = new StringReader(JSON.toJson(bytesMap)); - JsonInput bytesInput = JSON.newInput(reader)) { - return BytesValue.fromJson(bytesInput); + jsonInput.beginObject(); + BytesValue bytes = null; + while (jsonInput.hasNext()) { + String name = jsonInput.nextName(); + if ("bytes".equals(name)) { + bytes = BytesValue.fromJson(jsonInput); + } else { + jsonInput.skipValue(); + } } + jsonInput.endObject(); + return bytes; }; The JSON parsing for the getData command in Network.java is inefficient due to a redundant serialize-deserialize cycle. Refactor this to process the nested JSON map directly. Examples: java/src/org/openqa/selenium/bidi/module/Network.java [72-82] private final Function<JsonInput, BytesValue> getDataResultMapper = jsonInput -> { Map<String, Object> result = jsonInput.read(Map.class); @SuppressWarnings("unchecked") Map<String, Object> bytesMap = (Map<String, Object>) result.get("bytes"); try (StringReader reader = new StringReader(JSON.toJson(bytesMap)); JsonInput bytesInput = JSON.newInput(reader)) { return BytesValue.fromJson(bytesInput); } ... (clipped 1 lines) Solution Walkthrough: Before: private final Function<JsonInput, BytesValue> getDataResultMapper = jsonInput -> { Map<String, Object> result = jsonInput.read(Map.class); Map<String, Object> bytesMap = (Map<String, Object>) result.get("bytes"); try (StringReader reader = new StringReader(JSON.toJson(bytesMap)); JsonInput bytesInput = JSON.newInput(reader)) { return BytesValue.fromJson(bytesInput); } }; After: private final Function<JsonInput, BytesValue> getDataResultMapper = jsonInput -> { jsonInput.beginObject(); while (jsonInput.hasNext()) { if ("bytes".equals(jsonInput.nextName())) { return BytesValue.fromJson(jsonInput); } else { jsonInput.skipValue(); } } jsonInput.endObject(); // Or throw an exception if "bytes" is not found return null; }; Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies an inefficient JSON processing pattern and proposes a valid simplification, which improves performance and code clarity.	Medium
Possible issue	Validate against an empty list In the AddDataCollectorParameters constructor, add a validation check to ensure the dataTypes list is not empty, as required by the BiDi specification. java/src/org/openqa/selenium/bidi/network/AddDataCollectorParameters.java [34-42] public AddDataCollectorParameters(List<DataType> dataTypes, long maxEncodedDataSize) { Require.nonNull("Data types", dataTypes); + Require.nonNull("Max encoded data size", maxEncodedDataSize); + if (dataTypes.isEmpty()) { + throw new IllegalArgumentException("Data types list must not be empty"); + } if (maxEncodedDataSize <= 0) { throw new IllegalArgumentException("Max encoded data size must be positive"); } dataTypes.forEach(dataType -> this.dataTypes.add(dataType.toString())); this.maxEncodedDataSize = maxEncodedDataSize; } Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a missing validation for an empty dataTypes list, which improves API robustness by enforcing a contract from the BiDi specification.	Medium
General	✅ Improve performance by avoiding re-serialization Suggestion Impact: The commit replaced Map-based parsing and re-serialization with streaming parsing using JsonInput.beginObject/hasNext/nextName/skipValue and directly invoked BytesValue.fromJson, eliminating JSON re-serialization and removing Json/StringReader usage. code diff: @@ -71,14 +67,18 @@ private final Function<JsonInput, BytesValue> getDataResultMapper = jsonInput -> { - Map<String, Object> result = jsonInput.read(Map.class); - @SuppressWarnings("unchecked") - Map<String, Object> bytesMap = (Map<String, Object>) result.get("bytes"); - - try (StringReader reader = new StringReader(JSON.toJson(bytesMap)); - JsonInput bytesInput = JSON.newInput(reader)) { - return BytesValue.fromJson(bytesInput); + jsonInput.beginObject(); + BytesValue bytes = null; + while (jsonInput.hasNext()) { + String name = jsonInput.nextName(); + if ("bytes".equals(name)) { + bytes = BytesValue.fromJson(jsonInput); + } else { + jsonInput.skipValue(); + } } + jsonInput.endObject(); + return bytes; }; Refactor getDataResultMapper to use the JsonInput streaming API, avoiding inefficient intermediate object creation and JSON re-serialization when parsing the response. java/src/org/openqa/selenium/bidi/module/Network.java [72-82] private final Function<JsonInput, BytesValue> getDataResultMapper = jsonInput -> { - Map<String, Object> result = jsonInput.read(Map.class); - @SuppressWarnings("unchecked") - Map<String, Object> bytesMap = (Map<String, Object>) result.get("bytes"); - - try (StringReader reader = new StringReader(JSON.toJson(bytesMap)); - JsonInput bytesInput = JSON.newInput(reader)) { - return BytesValue.fromJson(bytesInput); + BytesValue bytesValue = null; + jsonInput.beginObject(); + while (jsonInput.hasNext()) { + if ("bytes".equals(jsonInput.nextName())) { + bytesValue = BytesValue.fromJson(jsonInput); + } else { + jsonInput.skipValue(); + } } + jsonInput.endObject(); + return bytesValue; }; [Suggestion processed] Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies an inefficient JSON parsing pattern and proposes a more performant, streaming-based approach which improves code quality and efficiency.	Low
Update

[diemol]

Why can't the test reuse the driver? Is this a BiDi limitation?

[navin772]

I added it since other tests were using it too, I will check if its strictly required or not.

[navin772]

@diemol 3 tests fail when @NeedsFreshDriver is removed from all the tests, 2 auth ones and 1 data collector that was added.
Should I remove it for other tests? It does reduce the test execution time by a lot (45sec -> 14sec)

[titusfortner]

We need to be able to clear handlers to reuse drivers

[titusfortner]

So, data collection doesn't make sense outside the context of an interception. You have to pass a request id to get the data, which is only accessible from a handler.

I think this should all be private API. When you register a handler you specify if you need the body, and that method registers the collector and manages it in the background.

[navin772]

I think this should all be private API. When you register a handler you specify if you need the body, and that method registers the collector and manages it in the background.

@titusfortner should we overload the event handlers to something like this and manage the collector inside the event handler?:

public void onBeforeRequestSent(Consumer<BeforeRequestSent> consumer, boolean captureBody)

Also, will this be applicable to only these events:

1. onBeforeRequestSent
2. onResponseCompleted
3. onResponseStarted

[Copilot]

Pull request overview

This PR implements three BiDi network data collection commands following the W3C WebDriver BiDi specification: addDataCollector, removeDataCollector, and getData. These commands enable collecting and retrieving network request/response body data.

Changes:

• Add three network data collection methods to the Network module with proper result mapping
• Implement parameter classes (AddDataCollectorParameters, GetDataParameters) with fluent builder pattern
• Add DataType enum to specify request or response data collection
• Provide comprehensive test coverage for all new functionality including edge cases

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
Network.java	Adds addDataCollector, removeDataCollector, and getData methods with proper BiDi command mapping and JSON result parsing
AddDataCollectorParameters.java	Parameter builder for configuring data collectors with support for data types, max size, collector type, and contexts
DataType.java	Enum defining REQUEST and RESPONSE data types for network data collection
GetDataParameters.java	Parameter builder for retrieving collected network data with support for collector ID and disown flag
NetworkCommandsTest.java	Comprehensive test suite covering various scenarios including collector lifecycle, data retrieval, disown behavior, and request/response data collection

[Copilot]

The constructor should validate that the dataTypes list is not empty. According to the W3C BiDi spec, dataTypes is a required parameter and must contain at least one element. An empty list would result in invalid BiDi command parameters. Consider adding validation similar to other parameter classes in the codebase.

Suggested change

	Require.nonNull("Data types", dataTypes);
	Require.nonNull("Data types", dataTypes);
	if (dataTypes.isEmpty()) {
	throw new IllegalArgumentException("Data types must not be empty");
	}

[Copilot]

Pull request overview

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