[java] Add "se" prefixed capabilities to session response

[pujagani]

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

Fixes #14316.

Motivation and Context

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.

---

PR Type

Enhancement, Tests

---

Description

• Added a new method readContainerName in DriverServiceSessionFactory to read and set the se:containerName capability.
• Integrated the readContainerName method into the session creation process to ensure the capability is included in the session response.
• Added a new test responseCapsShowContainerName in LocalNodeTest to verify that the se:containerName capability is correctly included in the session response.

---

Changes walkthrough 📝

	Relevant files
Enhancement	DriverServiceSessionFactory.java Add method to read and set `se:containerName` capability  java/src/org/openqa/selenium/grid/node/config/DriverServiceSessionFactory.java Added method readContainerName to read and set the se:containerName capability. Integrated readContainerName method into the session creation process. +11/-0
Tests	LocalNodeTest.java Add test for `se:containerName` capability in session response java/test/org/openqa/selenium/grid/node/local/LocalNodeTest.java Added a new test responseCapsShowContainerName to verify the se:containerName capability in session response. +29/-0

---

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

[qodo-merge-pro]

PR Reviewer Guide 🔍

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Key issues to review Type Safety The use of String.valueOf() at line 301 could potentially introduce type safety issues if the capability is not a string. Consider checking the type before casting. Error Handling There is no error handling if the se:containerName capability is not present or is invalid. This could lead to unexpected behavior or errors downstream.

[qodo-merge-pro]

PR Code Suggestions ✨

Category	Suggestion	Score
Possible issue	✅ Add a null check before setting the 'se:containerName' capability Suggestion Impact: The commit implemented a more general solution by checking for all capabilities starting with "se:" and ensuring they are not already present in the returned capabilities before setting them. This includes the 'se:containerName' capability, thus addressing the suggestion's intention. code diff: + private Capabilities readPrefixedCaps(Capabilities requestedCaps, Capabilities returnedCaps) { + + PersistentCapabilities returnPrefixedCaps = new PersistentCapabilities(returnedCaps); + + Map<String, Object> requestedCapsMap = requestedCaps.asMap(); + Map<String, Object> returnedCapsMap = returnedCaps.asMap(); + + requestedCapsMap.forEach( + (k, v) -> { + if (k.startsWith("se:") && !returnedCapsMap.containsKey(k)) { + returnPrefixedCaps.setCapability(k, v); + } + }); + + return returnPrefixedCaps; Consider checking if the seContainerName capability is actually provided before setting it. This can prevent potential issues where the capability might not be set or could be null, leading to unexpected behavior. java/src/org/openqa/selenium/grid/node/config/DriverServiceSessionFactory.java [301-304] -String seContainerName = String.valueOf(requestedCaps.getCapability(seContainerNameCap)); -returnedCaps = new PersistentCapabilities(returnedCaps).setCapability(seContainerNameCap, seContainerName); +if (requestedCaps.getCapability(seContainerNameCap) != null) { + String seContainerName = String.valueOf(requestedCaps.getCapability(seContainerNameCap)); + returnedCaps = new PersistentCapabilities(returnedCaps).setCapability(seContainerNameCap, seContainerName); +} Apply this suggestion Suggestion importance[1-10]: 9 Why: Adding a null check before setting the 'se:containerName' capability is a good practice to prevent potential null pointer exceptions and ensure robustness.	9
Enhancement	Improve type handling and error management for capability value conversion Use a more specific exception handling or logging mechanism when converting the capability value to a string, to handle potential ClassCastException or unexpected value types gracefully. java/src/org/openqa/selenium/grid/node/config/DriverServiceSessionFactory.java [301] -String seContainerName = String.valueOf(requestedCaps.getCapability(seContainerNameCap)); +Object capValue = requestedCaps.getCapability(seContainerNameCap); +String seContainerName = (capValue instanceof String) ? (String) capValue : capValue.toString(); Apply this suggestion Suggestion importance[1-10]: 8 Why: Improving type handling and error management for capability value conversion can prevent runtime errors and improve code reliability.	8
Performance	✅ Optimize the setting of capabilities by reducing unnecessary object creation Suggestion Impact: The commit refactored the method to avoid creating a new instance of PersistentCapabilities every time a capability is set, which aligns with the suggestion to reduce unnecessary object creation. code diff: - private Capabilities readContainerName(Capabilities requestedCaps, Capabilities returnedCaps) { - String seContainerNameCap = "se:containerName"; - String seContainerName = String.valueOf(requestedCaps.getCapability(seContainerNameCap)); - - returnedCaps = - new PersistentCapabilities(returnedCaps).setCapability(seContainerNameCap, seContainerName); - - return returnedCaps; + private Capabilities readPrefixedCaps(Capabilities requestedCaps, Capabilities returnedCaps) { + + PersistentCapabilities returnPrefixedCaps = new PersistentCapabilities(returnedCaps); + + Map<String, Object> requestedCapsMap = requestedCaps.asMap(); + Map<String, Object> returnedCapsMap = returnedCaps.asMap(); + + requestedCapsMap.forEach( + (k, v) -> { + if (k.startsWith("se:") && !returnedCapsMap.containsKey(k)) { + returnPrefixedCaps.setCapability(k, v); + } + }); + + return returnPrefixedCaps; Refactor the method to avoid creating a new instance of PersistentCapabilities every time the capability is set. This can improve performance by reducing object creation overhead. java/src/org/openqa/selenium/grid/node/config/DriverServiceSessionFactory.java [304] -returnedCaps = new PersistentCapabilities(returnedCaps).setCapability(seContainerNameCap, seContainerName); +((MutableCapabilities) returnedCaps).setCapability(seContainerNameCap, seContainerName); Apply this suggestion Suggestion importance[1-10]: 7 Why: Reducing unnecessary object creation can improve performance, although the impact might be minor in this context.	7
Best practice	Standardize the variable naming to follow Java naming conventions Ensure consistency in variable naming by using camelCase for seContainerNameCap to match Java standard practices. java/src/org/openqa/selenium/grid/node/config/DriverServiceSessionFactory.java [300] -String seContainerNameCap = "se:containerName"; +String seContainerNameCap = "seContainerName"; Apply this suggestion Suggestion importance[1-10]: 3 Why: While standardizing variable naming to follow Java conventions is good practice, this change is minor and does not significantly impact functionality or performance.	3

[diemol]

I thought #14316 was caused by the caps merge in ChromiumOptions, because it works well in Firefox.

[pujagani]

So Firefox returns any caps it does not recognize as part of session creation response. But Chromium based browsers filter it out. Hence the difference in behavior. The merge works as expected ( I debugged through it). So this capability needs to be added in the response, similar to what we do for devtools.

[diemol]

Understood. Thank you for explaining this.

Should we make this more generic and return all prefixed capabilities?

[pujagani]

@VietND96 Also suggested to make it generic. My concern was the "se:" notation is used by cloud providers too. So not sure if adding all such capabilities in the response with that notation is the best approach. What do you think?

[diemol]

I know it is used to overwrite the value of se:cdp, but I do not know in which other case. I guess my idea is to add the prefixed capabilities from the request if they do not exist already in the response.

[pujagani]

I understand. So it is okay to add the options that cloud providers send back in the response? Then we can do this.

[diemol]

I would say so. We can always iterate on it.

[VietND96]

How the complexity for that generic implementation? Can it be delivered in version 4.24?

[pujagani]

I will go add the support to return all prefixed capabilities.

[diemol]

Thank you, @pujagani!

[VietND96]

@pujagani , I have checked the Nightly build, but the result didn't meet expectation. Below is response from GraphQL for stereotype and session capabilities. se:containerName didn't present in session caps

{
  "data": {
    "session": {
      "id": "fee9aa7d5b9bfed8215dc4c13eb712e8",
      "capabilities": "{\n  \"acceptInsecureCerts\": false,\n  \"browserName\": \"chrome\",\n  \"browserVersion\": \"127.0.6533.119\",\n  \"chrome\": {\n    \"chromedriverVersion\": \"127.0.6533.119 (bdef6783a05f0b3f885591e7d2c7b2aec1a89dea-refs\\u002fbranch-heads\\u002f6533@{#1999})\",\n    \"userDataDir\": \"\\u002ftmp\\u002f.org.chromium.Chromium.ayWmnw\"\n  },\n  \"fedcm:accounts\": true,\n  \"goog:chromeOptions\": {\n    \"debuggerAddress\": \"localhost:46525\"\n  },\n  \"networkConnectionEnabled\": false,\n  \"pageLoadStrategy\": \"normal\",\n  \"platformName\": \"linux\",\n  \"proxy\": {\n  },\n  \"se:bidiEnabled\": false,\n  \"se:cdp\": \"wss:\\u002f\\u002f10.1.0.245\\u002fselenium\\u002fsession\\u002ffee9aa7d5b9bfed8215dc4c13eb712e8\\u002fse\\u002fcdp\",\n  \"se:cdpVersion\": \"127.0.6533.119\",\n  \"se:downloadsEnabled\": true,\n  \"se:name\": \"test_play_video (ChromeTests)\",\n  \"se:recordVideo\": true,\n  \"se:screenResolution\": \"1920x1080\",\n  \"se:vnc\": \"wss:\\u002f\\u002f10.1.0.245\\u002fselenium\\u002fsession\\u002ffee9aa7d5b9bfed8215dc4c13eb712e8\\u002fse\\u002fvnc\",\n  \"se:vncEnabled\": true,\n  \"se:vncLocalAddress\": \"ws:\\u002f\\u002f10.244.251.137:7900\",\n  \"setWindowRect\": true,\n  \"strictFileInteractability\": false,\n  \"timeouts\": {\n    \"implicit\": 0,\n    \"pageLoad\": 300000,\n    \"script\": 30000\n  },\n  \"unhandledPromptBehavior\": \"dismiss and notify\",\n  \"webauthn:extension:credBlob\": true,\n  \"webauthn:extension:largeBlob\": true,\n  \"webauthn:extension:minPinLength\": true,\n  \"webauthn:extension:prf\": true,\n  \"webauthn:virtualAuthenticators\": true\n}",
      "startTime": "21/08/2024 01:45:57",
      "uri": "http://10.244.251.137:6666",
      "nodeId": "b1f84412-a065-472d-8a83-c0eba6de300b",
      "nodeUri": "http://10.244.251.137:6666",
      "sessionDurationMillis": "1016",
      "slot": {
        "id": "62afd8fb-a123-4497-b475-8c0c383ff6c5",
        "stereotype": "{\n  \"browserName\": \"chrome\",\n  \"browserVersion\": \"127.0\",\n  \"goog:chromeOptions\": {\n    \"binary\": \"\\u002fusr\\u002fbin\\u002fchromium\"\n  },\n  \"platformName\": \"linux\",\n  \"se:containerName\": \"my-chrome-name-85c6f9fb-tmcqf\",\n  \"se:downloadsEnabled\": true,\n  \"se:noVncPort\": 7900,\n  \"se:vncEnabled\": true\n}",
        "lastStarted": "21/08/2024 01:45:57"
      }
    }
  }
}

[pujagani]

Which graphQL endpoint are you calling?

[VietND96]

@pujagani, the endpoint is selenium-hub:4444/graphql with query as below

'{"query":"{ session (id: "'${SESSION_ID}'") { id, capabilities, startTime, uri, nodeId, nodeUri, sessionDurationMillis, slot { id, stereotype, lastStarted } } } "}'

[VietND96]

In Hub logs, it printed caps as below, se:containerName also not included

21:26:15.825 INFO [LocalDistributor.newSession] - Session created by the Distributor. Id: a4b0bdac43d694550b2fa54eff99519e
Caps: Capabilities {acceptInsecureCerts: false, browserName: chrome, browserVersion: 128.0.6613.84, chrome: {chromedriverVersion: 128.0.6613.84 (606aa55c7d68..., userDataDir: /tmp/.org.chromium.Chromium...}, fedcm:accounts: true, goog:chromeOptions: {debuggerAddress: localhost:33739}, networkConnectionEnabled: false, pageLoadStrategy: normal, platformName: linux, proxy: {}, se:bidiEnabled: false, se:cdp: wss://10.1.0.47:31444/selen..., se:cdpVersion: 128.0.6613.84, se:downloadsEnabled: true, se:name: test_download_file (ChromeT..., se:recordVideo: true, se:screenResolution: 1920x1080, se:vnc: wss://10.1.0.47:31444/selen..., se:vncEnabled: true, se:vncLocalAddress: ws://10.244.103.12:7900, setWindowRect: true, strictFileInteractability: false, timeouts: {implicit: 0, pageLoad: 300000, script: 30000}, unhandledPromptBehavior: dismiss and notify, webauthn:extension:credBlob: true, webauthn:extension:largeBlob: true, webauthn:extension:minPinLength: true, webauthn:extension:prf: true, webauthn:virtualAuthenticators: true}

[pujagani]

Thank you for checking. Let me triage this further.

[diemol]

Maybe the issue is that setCapability returns a new PersistentCapabilities object and does not modify the current one.

[pujagani]

Yes, exactly. I fixed it via ef1b422.

[pujagani]

It was a quick fix, but how to add a test for it is tricky. We don't have an end-to-end test for DriverServiceSessionFactory.
However, locally, I am now seeing the expected result.

{
    "data": {
        "session": {
            "id": "d83a88c7cd21da860faab12db2ce159c",
            "capabilities": "{\n  \"acceptInsecureCerts\": false,\n  \"browserName\": \"chrome\",\n  \"browserVersion\": \"127.0.6533.120\",\n  \"chrome\": {\n    \"chromedriverVersion\": \"127.0.6533.119 (bdef6783a05f0b3f885591e7d2c7b2aec1a89dea-refs\/branch-heads\/6533@{#1999})\",\n    \"userDataDir\": \"\/var\/folders\/r2\/3dlyvgq1275gthvnwn_c5qy40000gq\/T\/.org.chromium.Chromium.F0Seh7\"\n  },\n  \"fedcm:accounts\": true,\n  \"goog:chromeOptions\": {\n    \"debuggerAddress\": \"localhost:56775\"\n  },\n  \"networkConnectionEnabled\": false,\n  \"pageLoadStrategy\": \"normal\",\n  \"platformName\": \"mac\",\n  \"proxy\": {\n  },\n  \"se:bidiEnabled\": false,\n  \"se:cdp\": \"ws:\/\/192.168.31.67:4444\/session\/d83a88c7cd21da860faab12db2ce159c\/se\/cdp\",\n  \"se:cdpVersion\": \"127.0.6533.120\",\n  \"se:containerName\": \"my-chrome-node\",\n  \"setWindowRect\": true,\n  \"strictFileInteractability\": false,\n  \"timeouts\": {\n    \"implicit\": 0,\n    \"pageLoad\": 300000,\n    \"script\": 30000\n  },\n  \"unhandledPromptBehavior\": \"dismiss and notify\",\n  \"webauthn:extension:credBlob\": true,\n  \"webauthn:extension:largeBlob\": true,\n  \"webauthn:extension:minPinLength\": true,\n  \"webauthn:extension:prf\": true,\n  \"webauthn:virtualAuthenticators\": true\n}",
            "startTime": "23/08/2024 18:45:44",
            "uri": "http://192.168.31.67:4444",
            "nodeId": "1532f5f6-db5a-4851-9466-e37c7d294607",
            "nodeUri": "http://192.168.31.67:4444",
            "sessionDurationMillis": "30347",
            "slot": {
                "id": "a18a014a-fbcc-4d5d-9cb2-67225378d7c9",
                "stereotype": "{\n  \"browserName\": \"chrome\",\n  \"platformName\": \"mac\",\n  \"se:containerName\": \"my-chrome-node\"\n}",
                "lastStarted": "23/08/2024 18:45:44"
            }
        }
    }
}