[java][bidi] Add subscription scope - #17925
Open
pujagani wants to merge 1 commit into
Open
Conversation
selenium-ci
added
C-java
Contributor
PR Summary by QodoAdd scoped Java BiDi event subscriptions
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
Contributor
Code Review by Qodo
1. Scoped callbacks cross-deliver
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| * session.SubscriptionParameters</a> | ||
| */ | ||
| @Beta | ||
| public final class SubscriptionScope { |
Contributor
There was a problem hiding this comment.
1. subscriptionscope constructor lacks javadoc 📘 Rule violation ✧ Quality
The new public SubscriptionScope class exposes an implicit public no-argument constructor without constructor Javadoc. Public API constructors must be explicitly documented.
Agent Prompt
## Issue description
`SubscriptionScope` currently exposes an undocumented implicit public no-argument constructor.
## Issue Context
Public API constructors must have Javadoc immediately preceding their declarations. Declare the constructor explicitly and document its purpose.
## Fix Focus Areas
- java/src/org/openqa/selenium/bidi/SubscriptionScope.java[37-40]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+69
to
+70
| public final <X> String subscribe(Event<X> event, Consumer<X> handler, SubscriptionScope scope) { | ||
| return handle.subscribe(event, handler, scope); |
Contributor
There was a problem hiding this comment.
2. Scoped subscriptions lack tests 📘 Rule violation ▣ Testability
The PR adds scoped subscription construction and a public subscribe overload without adding or updating automated tests. Regressions in browsing-context or user-context parameter serialization would therefore go undetected.
Agent Prompt
## Issue description
The new scoped subscription API and serialization paths have no corresponding automated tests.
## Issue Context
Add assertions covering browsing-context scopes, user-context scopes, combined scopes, and propagation through the new `Module.subscribe` overload. Tests should fail against the implementation before this PR.
## Fix Focus Areas
- java/src/org/openqa/selenium/bidi/Module.java[69-70]
- java/src/org/openqa/selenium/bidi/SubscriptionScope.java[64-72]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| Map<String, Object> params = new HashMap<>(scope.toMap()); | ||
| params.put("events", List.of(event.getMethod())); | ||
| String subscriptionId = subscribe(params); | ||
| connection.addListener(subscriptionId, event, handler); |
Contributor
There was a problem hiding this comment.
3. Scoped callbacks cross-deliver 🐞 Bug ≡ Correctness
The scoped addListener registers only the subscription ID and event, discarding its SubscriptionScope; Connection then invokes every handler registered for the matching event method. Consequently, when the same event has subscriptions for different contexts, each handler receives the other scopes' events (or duplicate callbacks if the remote emits per subscription), so the new scope API does not provide scoped listener behavior.
Agent Prompt
## Issue description
Scoped subscriptions discard their scope during listener registration. Event dispatch subsequently invokes all handlers for the event method, causing events to cross browsing-context or user-context subscription boundaries.
## Issue Context
`SubscriptionScope` affects the remote `session.subscribe` request, but incoming events currently carry no routing decision into `Connection`. The implementation must either preserve and enforce scope during dispatch using reliable event metadata, route by protocol-provided subscription metadata, or defer the scoped public API until correct routing is possible.
## Fix Focus Areas
- java/src/org/openqa/selenium/bidi/BiDi.java[120-129]
- java/src/org/openqa/selenium/bidi/Connection.java[178-190]
- java/src/org/openqa/selenium/bidi/Connection.java[347-394]
- java/src/org/openqa/selenium/bidi/SubscriptionScope.java[64-72]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔗 Related Issues
Add subscription scope to allow users to subscribe using user contexts and browsing contexts. Though, currently BiDi spec draft is working on received events to provide these parameters so clients can redirect events to correct subscribers. This will be required eventually for high-level work and for generated modules.
💥 What does this PR do?
🔧 Implementation Notes
Additional builder class is the a Java pattern that fits the best, in case in future more parameters for subscription are added.
🤖 AI assistance
💡 Additional Considerations
🔄 Types of changes