[bidi][java] Add storage module by pujagani · Pull Request #13675 · SeleniumHQ/selenium

pujagani · 2024-03-11T07:06:09Z

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Add storage module as described in https://w3c.github.io/webdriver-bidi/#module-storage

Motivation and Context

Implementing BiDi APIs incrementally

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist

I have read the contributing document.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.

Type

enhancement, tests

Description

Implemented the Storage class with methods for cookie management, including getting, setting, and deleting cookies.
Fixed JSON property names and expiry handling in the Cookie class.
Added supporting classes for storage operations, such as BrowsingContextPartitionDescriptor, CookieFilter, and more.
Added comprehensive tests for the storage module, focusing on cookie operations.
Updated Bazel build configurations to include the new storage package and its tests.

Changes walkthrough

Relevant files

Enhancement

Storage.java `Implement Storage Class with Cookie Management Methods` java/src/org/openqa/selenium/bidi/Storage.java Introduced the `Storage` class to handle storage-related commands. Implemented methods for getting, setting, and deleting cookies. Utilized the `BiDi` protocol for communication.	+74/-0
BytesValue.java `Update Visibility of BytesValue Components` java/src/org/openqa/selenium/bidi/network/BytesValue.java Made the `Type` enum and constructor of `BytesValue` public.	+2/-2
.java `Add Storage Module Supporting Classes` java/src/org/openqa/selenium/bidi/storage/.java Added multiple classes to support storage operations, including cookie and partition management. Classes include `BrowsingContextPartitionDescriptor`, `CookieFilter`, `DeleteCookiesParameters`, `GetCookiesParameters`, `GetCookiesResult`, `PartialCookie`, `PartitionDescriptor`, `PartitionKey`, `SetCookieParameters`, and `StorageKeyPartitionDescriptor`.

Bug fix

Cookie.java `Fix Cookie JSON Property Names and Expiry Handling` java/src/org/openqa/selenium/bidi/network/Cookie.java Fixed the JSON property name for `httpOnly` cookies. Changed how expiry time is read, now wrapped in `Optional`.	+2/-2

Tests

StorageCommandsTest.java `Add Tests for Storage Module Cookie Operations` java/test/org/openqa/selenium/bidi/storage/StorageCommandsTest.java Added tests for the storage module, specifically testing cookie get, set, and delete operations. Tests cover various scenarios including cookie retrieval by name, adding cookies, and deleting cookies.	+384/-0

Configuration changes

BUILD.bazel `Update Bazel Build to Include Storage Package` java/src/org/openqa/selenium/bidi/BUILD.bazel Updated Bazel build configuration to include the `storage` package.	+1/-0
BUILD.bazel `Add Bazel Build Configuration for Storage Module Tests` java/test/org/openqa/selenium/bidi/storage/BUILD.bazel Added a new Bazel build file for testing the storage module.	+27/-0

✨ PR-Agent usage:
Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

qodo-code-review · 2024-03-11T07:07:13Z

PR Description updated to latest commit (af9754f)

Copy walkthrough table to "Files Changed" Tab

qodo-code-review · 2024-03-11T07:07:26Z

PR Review

⏱️ Estimated effort to review [1-5]	4, because the PR introduces a significant amount of new functionality related to the storage module, including handling cookies and partition descriptors. The complexity is increased by the need to understand the BiDi protocol and how it integrates with the existing Selenium architecture. Additionally, there are numerous new classes and methods, each with their own logic and potential edge cases, which require thorough review and understanding.
🧪 Relevant tests	Yes
🔍 Possible issues	Possible Bug: In `Cookie.java`, the change from `isHttpOnly` to `httpOnly` might introduce inconsistencies if other parts of the codebase or documentation still refer to the old naming convention.
	Design Concern: The extensive use of `Optional` in method parameters (e.g., `DeleteCookiesParameters`, `GetCookiesParameters`) could lead to less clear code and potential misuse by calling methods without explicit intentions. Consider if there's a more straightforward way to handle optional parameters.
	Error Handling: There's no visible error handling or logging mechanism in the new storage module classes. Consider adding error handling to manage and log exceptions or unexpected behavior, especially for operations that involve external communication or complex logic.
🔒 Security concerns	No

✨ Review tool usage guide:

Overview:
The review tool scans the PR code changes, and generates a PR review. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.
When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:

/review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...

With a configuration file, use the following template:

[pr_reviewer]
some_config1=...
some_config2=...

Utilizing extra instructions The `review` tool can be configured with extra instructions, which can be used to guide the model to a feedback tailored to the needs of your project. Be specific, clear, and concise in the instructions. With extra instructions, you are the prompter. Specify the relevant sub-tool, and the relevant aspects of the PR that you want to emphasize. Examples for extra instructions: `[pr_reviewer] # /review # extra_instructions=""" In the 'possible issues' section, emphasize the following: - Does the code logic cover relevant edge cases? - Is the code logic clear and easy to understand? - Is the code logic efficient? ... """` Use triple quotes to write multi-line instructions. Use bullet points to make the instructions more readable.
How to enable\disable automation When you first install PR-Agent app, the default mode for the `review` tool is: `pr_commands = ["/review", ...]` meaning the `review` tool will run automatically on every PR, with the default configuration. Edit this field to enable/disable the tool, or to change the used configurations
Auto-labels The `review` tool can auto-generate two specific types of labels for a PR: a `possible security issue` label, that detects possible security issues (`enable_review_labels_security` flag) a `Review effort [1-5]: x` label, where x is the estimated effort to review the PR (`enable_review_labels_effort` flag)
Extra sub-tools The `review` tool provides a collection of possible feedbacks about a PR. It is recommended to review the possible options, and choose the ones relevant for your use case. Some of the feature that are disabled by default are quite useful, and should be considered for enabling. For example: `require_score_review`, `require_soc2_ticket`, and more.
Auto-approve PRs By invoking: `/review auto_approve` The tool will automatically approve the PR, and add a comment with the approval. To ensure safety, the auto-approval feature is disabled by default. To enable auto-approval, you need to actively set in a pre-defined configuration file the following: `[pr_reviewer] enable_auto_approval = true` (this specific flag cannot be set with a command line argument, only in the configuration file, committed to the repository) You can also enable auto-approval only if the PR meets certain requirements, such as that the `estimated_review_effort` is equal or below a certain threshold, by adjusting the flag: `[pr_reviewer] maximal_review_effort = 5`
More PR-Agent commands To invoke the PR-Agent, add a comment using one of the following commands: /review: Request a review of your Pull Request. /describe: Update the PR title and description based on the contents of the PR. /improve [--extended]: Suggest code improvements. Extended mode provides a higher quality feedback. /ask <QUESTION>: Ask a question about the PR. /update_changelog: Update the changelog based on the PR's contents. /add_docs 💎: Generate docstring for new components introduced in the PR. /generate_labels 💎: Generate labels for the PR based on the PR's contents. /analyze 💎: Automatically analyzes the PR, and presents changes walkthrough for each component. See the tools guide for more details. To list the possible configuration parameters, add a /config comment.

See the review usage page for a comprehensive guide on using this tool.

qodo-code-review · 2024-03-11T07:07:28Z

PR Code Suggestions

Category	Suggestions
Performance	Improve random number generation performance in concurrent environments. Replace the direct instantiation of `Random` with `ThreadLocalRandom.current()` for better performance in concurrent environments. java/test/org/openqa/selenium/bidi/storage/StorageCommandsTest.java [36] -private static final Random random = new Random(); +private static final Random random = ThreadLocalRandom.current();
Best practice	Ensure resources are closed properly using try-with-resources. Use try-with-resources to ensure that resources are closed properly and to make the code cleaner. java/test/org/openqa/selenium/bidi/storage/StorageCommandsTest.java [46-50] -try { +try (WebDriver driver = this.driver) { driver.get(appServer.whereIs("/common/animals")); } catch (IllegalArgumentException e) { return; }
	Make `PartialCookie` immutable for safer usage. Ensure that the `PartialCookie` class is immutable by removing setters and making the map unmodifiable. java/src/org/openqa/selenium/bidi/storage/PartialCookie.java [34-37] -public PartialCookie path(String path) { - map.put("path", path); - return this; +public PartialCookie(String name, BytesValue value, String domain, String path, long size, boolean httpOnly, boolean secure, Cookie.SameSite sameSite, long expiry) { + map = Map.of( + "name", name, + "value", value, + "domain", domain, + "path", path, + "size", size, + "httpOnly", httpOnly, + "secure", secure, + "sameSite", sameSite.toString(), + "expiry", expiry + ); }
	Verify and document the increased visibility of `Type` enum and `BytesValue` constructor. The `Type` enum and the constructor of `BytesValue` have been made public, which increases their visibility. Ensure that this change is intentional and aligns with the design principles of your project. If these elements are meant to be used externally, consider adding appropriate documentation or usage examples. java/src/org/openqa/selenium/bidi/network/BytesValue.java [24-44] -public enum Type { +public enum Type { // Ensure visibility is intentional ... -public BytesValue(Type type, String value) { +public BytesValue(Type type, String value) { // Confirm and document external usage
Enhancement	Use `var` for local variables to make the code more concise. Consider using `var` for local variables where the type is obvious from the right side of the assignment to make the code more concise and readable. java/src/org/openqa/selenium/bidi/Storage.java [38-39] -Function<JsonInput, GetCookiesResult> getCookiesResultMapper = - jsonInput -> jsonInput.read(GetCookiesResult.class); +var getCookiesResultMapper = jsonInput -> jsonInput.read(GetCookiesResult.class);
	Implement a builder pattern for `CookieFilter` to improve usability. Consider using a builder pattern for `CookieFilter` to improve the readability and usability of setting various properties. java/src/org/openqa/selenium/bidi/storage/CookieFilter.java [28-30] -public CookieFilter name(String name) { - map.put("name", name); - return this; +public static class Builder { + private Map<String, Object> map = new HashMap<>(); + + public Builder name(String name) { + map.put("name", name); + return this; + } + + // Other setters + + public CookieFilter build() { + return new CookieFilter(this.map); + } }
	Update all references to match the corrected "httpOnly" case. The case for "httpOnly" cookies has been corrected from "isHttpOnly" to "httpOnly" in the switch statement. Ensure that all references and documentations are updated accordingly to reflect this change, avoiding confusion and maintaining consistency across the codebase. java/src/org/openqa/selenium/bidi/network/Cookie.java [114] -case "httpOnly": +case "httpOnly": // Ensure all references are updated accordingly
Bug	Unwrap `Optional` objects before adding them to the map. In the `toMap` method, when adding `cookieFilter` and `partitionDescriptor` to the map, you're currently adding the `Optional` objects directly instead of their values. You should unwrap the `Optional` and add the contained value to the map. This ensures that the map contains the actual objects rather than the `Optional` wrappers. java/src/org/openqa/selenium/bidi/storage/DeleteCookiesParameters.java [43-44] -cookieFilter.ifPresent(filter -> map.put("filter", cookieFilter)); -partitionDescriptor.ifPresent(descriptor -> map.put("partition", partitionDescriptor)); +cookieFilter.ifPresent(filter -> map.put("filter", filter)); +partitionDescriptor.ifPresent(descriptor -> map.put("partition", descriptor));
	Correctly add unwrapped `Optional` values to the map. Similar to the previous suggestion, in the `toMap` method, the `Optional` objects for `cookieFilter` and `partitionDescriptor` are being added directly to the map. It's necessary to unwrap these `Optional` objects to ensure the actual values are added to the map, not the `Optional` wrappers. java/src/org/openqa/selenium/bidi/storage/GetCookiesParameters.java [42-43] -cookieFilter.ifPresent(filter -> map.put("filter", cookieFilter)); -partitionDescriptor.ifPresent(descriptor -> map.put("partition", partitionDescriptor)); +cookieFilter.ifPresent(filter -> map.put("filter", filter)); +partitionDescriptor.ifPresent(descriptor -> map.put("partition", descriptor));
	Unwrap `Optional` before adding to the map to ensure correct object type. In the `toMap` method, the `partitionDescriptor` is being added to the map as an `Optional` object. To ensure the map contains the actual `PartitionDescriptor` object, you should unwrap the `Optional` before adding it to the map. java/src/org/openqa/selenium/bidi/storage/SetCookieParameters.java [39] -partitionDescriptor.ifPresent(descriptor -> map.put("partition", partitionDescriptor)); +partitionDescriptor.ifPresent(descriptor -> map.put("partition", descriptor));

✨ Improve tool usage guide:

Overview:
The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.
When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:

/improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...

With a configuration file, use the following template:

[pr_code_suggestions]
some_config1=...
some_config2=...

Enabling\disabling automation When you first install the app, the default mode for the improve tool is: `pr_commands = ["/improve --pr_code_suggestions.summarize=true", ...]` meaning the `improve` tool will run automatically on every PR, with summarization enabled. Delete this line to disable the tool from running automatically.
Utilizing extra instructions Extra instructions are very important for the `improve` tool, since they enable to guide the model to suggestions that are more relevant to the specific needs of the project. Be specific, clear, and concise in the instructions. With extra instructions, you are the prompter. Specify relevant aspects that you want the model to focus on. Examples for extra instructions: `[pr_code_suggestions] # /improve # extra_instructions=""" Emphasize the following aspects: - Does the code logic cover relevant edge cases? - Is the code logic clear and easy to understand? - Is the code logic efficient? ... """` Use triple quotes to write multi-line instructions. Use bullet points to make the instructions more readable.
A note on code suggestions quality While the current AI for code is getting better and better (GPT-4), it's not flawless. Not all the suggestions will be perfect, and a user should not accept all of them automatically. Suggestions are not meant to be simplistic. Instead, they aim to give deep feedback and raise questions, ideas and thoughts to the user, who can then use his judgment, experience, and understanding of the code base. Recommended to use the 'extra_instructions' field to guide the model to suggestions that are more relevant to the specific needs of the project, or use the custom suggestions 💎 tool With large PRs, best quality will be obtained by using 'improve --extended' mode.
More PR-Agent commands To invoke the PR-Agent, add a comment using one of the following commands: /review: Request a review of your Pull Request. /describe: Update the PR title and description based on the contents of the PR. /improve [--extended]: Suggest code improvements. Extended mode provides a higher quality feedback. /ask <QUESTION>: Ask a question about the PR. /update_changelog: Update the changelog based on the PR's contents. /add_docs 💎: Generate docstring for new components introduced in the PR. /generate_labels 💎: Generate labels for the PR based on the PR's contents. /analyze 💎: Automatically analyzes the PR, and presents changes walkthrough for each component. See the tools guide for more details. To list the possible configuration parameters, add a /config comment.

See the improve usage page for a more comprehensive guide on using this tool.

codecov-commenter · 2024-03-11T07:28:02Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 58.48%. Comparing base (7384157) to head (842b9f6).

❗ Current head 842b9f6 differs from pull request most recent head 0e38d2f. Consider uploading reports for the commit 0e38d2f to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@           Coverage Diff           @@
##            trunk   #13675   +/-   ##
=======================================
  Coverage   58.48%   58.48%           
=======================================
  Files          86       86           
  Lines        5270     5270           
  Branches      220      220           
=======================================
  Hits         3082     3082           
  Misses       1968     1968           
  Partials      220      220

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

diemol

Thank you, @pujagani!

I think we need to land it and iterate over it when needed.

[bidi][java] Add storage module

af9754f

qodo-code-review bot added P-enhancement PR with a new feature tests labels Mar 11, 2024

qodo-code-review bot added the Review effort [1-5]: 4 label Mar 11, 2024

pujagani added 2 commits March 11, 2024 12:43

[java] Ensure actual parameters are passed

0689f84

[java] Use thread-safe random generator

0e38d2f

[java] Fix formatting and ignore tests working in Firefox Nightly

7eeec1b

diemol approved these changes Mar 11, 2024

View reviewed changes

pujagani merged commit 420f074 into SeleniumHQ:trunk Mar 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[bidi][java] Add storage module#13675

[bidi][java] Add storage module#13675
pujagani merged 4 commits intoSeleniumHQ:trunkfrom
pujagani:add-storage-module

pujagani commented Mar 11, 2024 •

edited by qodo-code-review bot

Loading

Uh oh!

qodo-code-review bot commented Mar 11, 2024

Uh oh!

qodo-code-review bot commented Mar 11, 2024

Uh oh!

qodo-code-review bot commented Mar 11, 2024 •

edited

Loading

Uh oh!

codecov-commenter commented Mar 11, 2024 •

edited

Loading

Uh oh!

diemol left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

pujagani commented Mar 11, 2024 • edited by qodo-code-review bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User description

Description

Motivation and Context

Types of changes

Checklist

Type

Description

Changes walkthrough

Uh oh!

qodo-code-review bot commented Mar 11, 2024

Uh oh!

qodo-code-review bot commented Mar 11, 2024

PR Review

Uh oh!

qodo-code-review bot commented Mar 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions

Uh oh!

codecov-commenter commented Mar 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

diemol left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pujagani commented Mar 11, 2024 •

edited by qodo-code-review bot

Loading

qodo-code-review bot commented Mar 11, 2024 •

edited

Loading

codecov-commenter commented Mar 11, 2024 •

edited

Loading