Skip to content

[java] extend the scope of the properties of the HttpCommandExecutor class#16186

Merged
diemol merged 3 commits intoSeleniumHQ:trunkfrom
iampopovich:feat/16181-appium-compatibility
Aug 18, 2025
Merged

[java] extend the scope of the properties of the HttpCommandExecutor class#16186
diemol merged 3 commits intoSeleniumHQ:trunkfrom
iampopovich:feat/16181-appium-compatibility

Conversation

@iampopovich
Copy link
Contributor

@iampopovich iampopovich commented Aug 15, 2025 •
edited by qodo-code-review bot
Loading

User description

🔗 Related Issues

fixes #16181

💥 What does this PR do?

scope of properties changed to protected

  • httpClientFactory -> private to protected
  • additionalCommands -> private to protected

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

  • Cleanup (formatting, renaming)
  • Bug fix (backwards compatible)
  • New feature (non-breaking change which adds functionality and tests!)
  • Breaking change (fix or feature that would cause existing functionality to change)

PR Type

Enhancement

Description

  • Changed visibility of two fields from private to protected

  • Enables subclass access to httpClientFactory and additionalCommands

  • Improves extensibility for custom implementations

Diagram Walkthrough

flowchart LR
  A["HttpCommandExecutor class"] --> B["httpClientFactory field"]
  A --> C["additionalCommands field"]
  B --> D["private → protected"]
  C --> E["private → protected"]
Loading

File Walkthrough
Relevant files
Enhancement
HttpCommandExecutor.java
Field visibility enhancement for extensibility                     

java/src/org/openqa/selenium/remote/HttpCommandExecutor.java

  • Changed httpClientFactory field visibility from private to protected
  • Changed additionalCommands field visibility from private to protected
 +2/-2     

@selenium-ci selenium-ci added the C-java Java Bindings label Aug 15, 2025
@iampopovich iampopovich marked this pull request as ready for review August 15, 2025 13:48
@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Aug 15, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

API Surface Change

Changing field visibility from private to protected expands the public API for inheritance and may lock in backward compatibility; confirm this is intentional and documented in release notes, and verify no unintended subclassing side effects occur.

 
protected final HttpClient.Factory httpClientFactory;
protected final Map<String, CommandInfo> additionalCommands;
Encapsulation Risk

Exposing mutable fields like additionalCommands to subclasses can allow state changes that affect thread-safety or invariants; consider providing protected accessors or defensive patterns if mutation must be controlled.

 
protected final Map<String, CommandInfo> additionalCommands;
protected CommandCodec<HttpRequest> commandCodec;

@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Aug 15, 2025 •
edited
Loading

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Learned
best practice
Preserve encapsulation with accessors

Changing fields from private to protected broadens access and risks unintended
mutation. Prefer keeping fields private and providing controlled, documented
accessors that return defensive copies or unmodifiable views.

java/src/org/openqa/selenium/remote/HttpCommandExecutor.java [50-51]

-protected final HttpClient.Factory httpClientFactory;
-protected final Map<String, CommandInfo> additionalCommands;
+private final HttpClient.Factory httpClientFactory;
+private final Map<String, CommandInfo> additionalCommands;
 
+protected final HttpClient.Factory getHttpClientFactory() {
+  return httpClientFactory;
+}
+
+protected final Map<String, CommandInfo> getAdditionalCommands() {
+  return Collections.unmodifiableMap(additionalCommands);
+}
+
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Avoid weakening encapsulation without safeguards; exposing internals should be accompanied by clear contracts or immutability to prevent misuse.

Low
  • Update

diemol
diemol approved these changes Aug 18, 2025
View reviewed changes
Copy link
Member

@diemol diemol left a comment

Choose a reason for hiding this comment

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

Thank you!

@diemol diemol merged commit f227da9 into SeleniumHQ:trunk Aug 18, 2025
60 of 61 checks passed
This was referenced Oct 2, 2025
Closed
Closed
Closed
Closed
Closed
Merged
Merged
Closed
Merged
Closed
Closed
Closed
Merged
Merged
Closed
Merged
Merged
Closed
Closed
Closed
This was referenced Oct 6, 2025
Merged
Closed
This was referenced Oct 20, 2025
Closed
Merged
Closed
Closed
Closed
Closed
@dependabot dependabot bot mentioned this pull request Oct 28, 2025
Closed
This was referenced Dec 9, 2025
Open
Closed
Closed
This was referenced Dec 16, 2025
Closed
Merged
Open
Closed
Closed
Closed
Closed
This was referenced Jan 19, 2026
Open
Open
Open
Open
Open
Open
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@diemol diemol diemol approved these changes

Assignees

No one assigned

Labels

C-java Java Bindings Review effort 1/5

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[🚀 Feature]: Better Selenium and Appium java clients compatibility

3 participants

@iampopovich @diemol @selenium-ci