validate-browsing-context-id #12678

manuelsblanco · 2023-09-03T22:26:30Z

Enhanced the BrowsingContext Constructor to Include Additional Validation for 'id' Parameter

Hello Selenium community! Your contribution matters and well-described pull requests make reviews and merges efficient. Thank you for your effort!

Before submitting your pull request, please review our contributing guidelines. Keeping pull requests concise and focused helps reviewers.

Description

In this pull request, I've enhanced the BrowsingContext constructor to include additional validation for the 'id' parameter. The objective is to ensure that the 'id' parameter is not only non-null but also non-empty, providing an extra layer of validation. Additionally, I've introduced a validation step to confirm that the WebDriver instance supports the BiDi protocol before retrieving the BiDi instance.

Motivation and Context

This enhancement was motivated by the need to strengthen the validation process for the 'id' parameter in the BrowsingContext constructor. By introducing a check for empty string values, we ensure that the 'id' parameter is not only non-null but also contains meaningful content. Additionally, verifying the support for the BiDi protocol in the WebDriver instance helps prevent compatibility issues.

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 guidelines.
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.

Enhanced the BrowsingContext Constructor to Include Additional Validation for 'id' Parameter **Hello Selenium community! Your contribution matters and well-described pull requests make reviews and merges efficient. Thank you for your effort!** Before submitting your pull request, please review our [contributing guidelines](https://github.com/SeleniumHQ/selenium/blob/trunk/CONTRIBUTING.md). Keeping pull requests concise and focused helps reviewers.  ### Description In this pull request, I've enhanced the `BrowsingContext` constructor to include additional validation for the 'id' parameter. The objective is to ensure that the 'id' parameter is not only non-null but also non-empty, providing an extra layer of validation. Additionally, I've introduced a validation step to confirm that the WebDriver instance supports the BiDi protocol before retrieving the BiDi instance. ### Motivation and Context This enhancement was motivated by the need to strengthen the validation process for the 'id' parameter in the `BrowsingContext` constructor. By introducing a check for empty string values, we ensure that the 'id' parameter is not only non-null but also contains meaningful content. Additionally, verifying the support for the BiDi protocol in the WebDriver instance helps prevent compatibility issues. ### Types of Changes - [ ] Bug fix (non-breaking change which fixes an issue) - [x] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) ### Checklist   - [x] I have read the [contributing guidelines](https://github.com/SeleniumHQ/selenium/blob/trunk/CONTRIBUTING.md). - [ ] My change requires a change to the documentation. - [ ] I have updated the documentation accordingly. - [] I have added tests to cover my changes. - [x] All new and existing tests passed.

CLAassistant · 2023-09-03T22:26:35Z

All committers have signed the CLA.

diemol · 2023-09-04T13:22:21Z

@pujagani, does this make sense?

In this commit, the LogEntryUtils interface is introduced to centralize the JSON deserialization logic for log entries. This helps eliminate code duplication in the GenericLogEntry and JavascriptLogEntry classes. The LogEntryUtils interface provides a common method for parsing JSON input into log entry objects. Additionally, a new test class, LogEntryUtilsTest, has been created to test the functionality of the LogEntryUtils interface. The JsonInputFactory class is utilized to create JsonInput instances consistently across tests.

manuelsblanco · 2023-09-08T18:01:50Z

Description

In this pull request, I've introduced the LogEntryUtils interface to enhance code reusability and centralize JSON deserialization logic for log entries. This interface provides a common method, fromJson, which simplifies the creation of log entry objects from JSON input.

The primary motivation behind this change is to eliminate code duplication in the GenericLogEntry and JavascriptLogEntry classes. The fromJson method reduces redundant code and improves maintainability. Furthermore, I've added a new test class, LogEntryUtilsTest, to thoroughly test the functionality of the LogEntryUtils interface.

Motivation and Context

This enhancement is motivated by the need to improve code maintainability and reduce redundancy in the codebase. By centralizing JSON deserialization logic in the LogEntryUtils interface, we ensure that the same parsing process is consistently applied to all log entry types. This change also promotes better testing practices by allowing us to easily test the deserialization process using the LogEntryUtilsTest class.

Types of Changes

New feature, a new interface to avoid duplicate code (non-breaking change which adds functionality)

Checklist

I have read the contributing guidelines.
[] My change does not require a change to the documentation.
I have added tests to cover my changes.
All new and existing tests passed.

manuelsblanco · 2023-09-08T18:09:43Z

Hi @diemol,

I noticed that @pujagani is currently out of the office. I have added new code to the PR. If you'd like to discuss it, you can reach me on Slack.

Thanks in advance.

codecov-commenter · 2023-09-24T21:56:15Z

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (bbcfc9a) 56.51% compared to head (25cff9c) 56.51%.

❗ Current head 25cff9c differs from pull request most recent head 0a106e2. Consider uploading reports for the commit 0a106e2 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   #12678   +/-   ##
=======================================
  Coverage   56.51%   56.51%           
=======================================
  Files          86       86           
  Lines        5255     5255           
  Branches      187      187           
=======================================
  Hits         2970     2970           
  Misses       2098     2098           
  Partials      187      187

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

pujagani · 2023-09-25T12:25:20Z

java/src/org/openqa/selenium/bidi/browsingcontext/BrowsingContext.java

@@ -77,6 +77,10 @@ public BrowsingContext(WebDriver driver, String id) {
      throw new IllegalArgumentException("WebDriver instance must support BiDi protocol");
    }

+    if (id.isEmpty()) {


It might be a good idea to use "Require.precondition" to check for null and empty string.

selenium/java/src/org/openqa/selenium/internal/Require.java

Line 52 in e7c60f5

public static void precondition(boolean condition, String message, Object... args) {

pujagani · 2023-09-25T12:28:21Z

@manuelsblanco I appreciate your help here. I agree with the motivation stated. However, I see two unrelated changes (motivation-wise) in the same PR. Can you please help split it up? Even if the change is small. It will be easier to review and land them independently. Thank you!

manuelsblanco · 2023-09-25T12:51:16Z

I appreciate your help here. I agree with the motivation stated. However, I see two unrelated changes (motivation-wise) in the same PR. Can you please help split it up? Even if the change is small. It will be easier to review and land them independently. Thank you!

Sure thing, I'll do this split as soon as possible.

Using Require.precondition()

manuelsblanco · 2023-10-05T15:21:37Z

Hi @pujagani, I have just implemented your suggestion regarding the use of Require.Precondition. Please let me know if there is anything else I can do.

pujagani · 2023-10-06T04:45:41Z

Thank you for incorporating the changes. Can you please help split up the PR as well? Thank you!

…mment) Using Require.precondition()

@pujagani

- Created the `LogEntryUtils` interface to centralize common JSON parsing and conversion methods. - Moved the `JsonMap` method to the `BaseLogEntry` class so it can be shared by its subclasses. - Implemented the `toJson()` method in the `JavascriptLogEntry` and `GenericLogEntry` classes. - Updated the `fromJson()` method in the `LogEntryUtils` interface to handle different log entry types. These changes improve code organization and maintainability by centralizing JSON-related functionality in the `LogEntryUtils` interface and reducing code duplication in log entry classes. As requested by @pujagani SeleniumHQ#12678 (comment)

manuelsblanco · 2023-10-11T20:22:40Z

This PR was split in two, we can close this one now.

Merge branch 'trunk' into validate-browsing-context-id

055a381

diemol requested a review from pujagani September 4, 2023 13:22

manuelsblanco added 2 commits September 8, 2023 14:55

Merge branch 'trunk' into validate-browsing-context-id

859b263

manuelsblanco and others added 2 commits September 16, 2023 13:43

Merge branch 'trunk' into validate-browsing-context-id

09f5d2e

Merge branch 'trunk' into validate-browsing-context-id

f452033

pujagani reviewed Sep 25, 2023

View reviewed changes

pujagani added the C-java label Sep 25, 2023

manuelsblanco and others added 2 commits September 25, 2023 09:51

Merge branch 'trunk' into validate-browsing-context-id

c5cd7ac

Merge branch 'trunk' into validate-browsing-context-id

7d654da

pujagani added the C-devtools BiDi or Chrome DevTools related issues label Sep 26, 2023

manuelsblanco and others added 3 commits September 26, 2023 10:10

Merge branch 'SeleniumHQ:trunk' into validate-browsing-context-id

f8a5d0b

Merge branch 'trunk' into validate-browsing-context-id

fc09eba

Modified BrowsingContext as per @pujagani suggestion

f5f2710

Using Require.precondition()

Merge branch 'trunk' into validate-browsing-context-id

9a4ef79

Merge branch 'SeleniumHQ:trunk' into validate-browsing-context-id

017b05a

manuelsblanco added a commit to manuelsblanco/selenium that referenced this pull request Oct 10, 2023

Split the pull request as requested by @pujagani SeleniumHQ#12678 (co…

dfff04d

…mment) Using Require.precondition()

Merge branch 'trunk' into validate-browsing-context-id

0a106e2

manuelsblanco mentioned this pull request Oct 11, 2023

[Feature] Refactor log entry classes and new interface #12932

Closed

8 tasks

manuelsblanco closed this Oct 11, 2023

manuelsblanco deleted the validate-browsing-context-id branch October 11, 2023 20:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

validate-browsing-context-id #12678

validate-browsing-context-id #12678

manuelsblanco commented Sep 3, 2023

CLAassistant commented Sep 3, 2023 •

edited

Loading

diemol commented Sep 4, 2023

manuelsblanco commented Sep 8, 2023 •

edited

Loading

manuelsblanco commented Sep 8, 2023

codecov-commenter commented Sep 24, 2023 •

edited

Loading

pujagani Sep 25, 2023 •

edited

Loading

pujagani commented Sep 25, 2023

manuelsblanco commented Sep 25, 2023

manuelsblanco commented Oct 5, 2023

pujagani commented Oct 6, 2023

manuelsblanco commented Oct 11, 2023

validate-browsing-context-id #12678

validate-browsing-context-id #12678

Conversation

manuelsblanco commented Sep 3, 2023

Description

Motivation and Context

Types of Changes

Checklist

CLAassistant commented Sep 3, 2023 • edited Loading

diemol commented Sep 4, 2023

manuelsblanco commented Sep 8, 2023 • edited Loading

manuelsblanco commented Sep 8, 2023

codecov-commenter commented Sep 24, 2023 • edited Loading

Codecov Report

pujagani Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

pujagani commented Sep 25, 2023

manuelsblanco commented Sep 25, 2023

manuelsblanco commented Oct 5, 2023

pujagani commented Oct 6, 2023

manuelsblanco commented Oct 11, 2023

CLAassistant commented Sep 3, 2023 •

edited

Loading

manuelsblanco commented Sep 8, 2023 •

edited

Loading

codecov-commenter commented Sep 24, 2023 •

edited

Loading

pujagani Sep 25, 2023 •

edited

Loading