feat(appium): implement GET /appium/sessions and deprecate GET /sessions #21233
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements a new endpoint, GET /appium/sessions, which returns session data with an added creation timestamp, replacing the deprecated GET /sessions. Key changes include:
- New interfaces and methods (e.g. DatedMultiSessionData and getAppiumSessions) in the types and driver files.
- Updates in tests to call getAppiumSessions instead of the old getSessions method.
- Addition of a sessionCreationTime property in core and driver implementations.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/types/lib/driver.ts | Added new interfaces for dated session data |
| packages/driver-test-support/lib/unit-suite.js | Updated tests to use getAppiumSessions |
| packages/base-driver/test/unit/basedriver/capability.spec.js | Replaced tests to reference the new session endpoint |
| packages/base-driver/lib/basedriver/driver.ts | Added sessionCreationTime initialization and getAppiumSessions method |
| packages/base-driver/lib/basedriver/core.ts | Extended DriverCore with the sessionCreationTime property |
| packages/appium/test/unit/appiumdriver.spec.js | Updated tests to use getAppiumSessions and verify the new 'created' field |
| packages/appium/lib/appium.js | Implemented getAppiumSessions and deprecated the old getSessions method |
Comments suppressed due to low confidence (2)
packages/base-driver/lib/basedriver/driver.ts:371
- [nitpick] The interface name 'DatedMultiSessionData' might be made more descriptive (for example, 'AppiumSessionData') to clearly convey that it is tailored for Appium sessions.
async getAppiumSessions() {
packages/appium/test/unit/appiumdriver.spec.js:323
- Consider adding additional tests to validate the format and correctness of the 'created' field, ensuring it accurately reflects a valid timestamp.
sessions[0].should.have.property('created');
| /** | ||
| * Returns the session id and capabilities for the session | ||
| * @deprecated Use AppiumDriver.getAppiumSessions instead for getting the session data. | ||
| */ | ||
| async getSessions() { | ||
| const ret: MultiSessionData<C>[] = []; |
There was a problem hiding this comment.
I'm not even sure this code is used anywhere. Shell we just delete it for good?
There was a problem hiding this comment.
It's probably due to the getSessions type in ISessionHandler, I guess it could be removed as well, but since it's removing an exported type, it's safer to do it in the Appium 3 branch
| |`--allow-insecure` |Setting this flag to a comma-separated list of feature names or a path to a file containing a feature list (each name on a separate line) will allow _only_ the features listed. For example, `--allow-insecure=adb_shell` will cause _only_ the ADB shell execution feature to be enabled. This is true _unless_ `--relaxed-security` is also used, in which case all features will still be enabled. It makes no sense to combine this flag with `--relaxed-security`.| | ||
| |`--deny-insecure` |This flag can likewise be set to a comma-separated list of feature names, or a path to a feature file. Any features listed here will be _disabled_, regardless of whether `--relaxed-security` is set and regardless of whether the names are also listed with `--allow-insecure`.| | ||
| |`--relaxed-security`|Turns on _all_ insecure features (unless blocked by `--deny-insecure`; see below)| | ||
| |`--allow-insecure`|Turns on _only_ specified features. Features can be provided as a comma-separated list of feature names, or in the [Appium Configuration file](./config.md). For example, `--allow-insecure=adb_shell` will cause _only_ the ADB shell execution feature to be enabled. Has no effect when used in combination with `--relaxed-security`.| |
There was a problem hiding this comment.
maybe we could also update the examples in docs to include the prefix (adb_shell -> uiautomator2:adb_shell)
There was a problem hiding this comment.
I'd prefer to do this in the Appium 3 branch where the scope is mandatory, since it would also require removing the driver scope security section and integrating it into the rest of the document
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new GET /appium/sessions endpoint with a TimestampedMultiSessionData interface and deprecates the legacy GET /sessions endpoint. Key changes include adding a new field (created) and timestamp property (sessionCreationTimestampMs) to session data, updating driver and test code to use the new endpoints, and enhancing insecure feature configuration along with corresponding documentation updates.
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/types/lib/driver.ts | Added TimestampedMultiSessionData and new timestamp field in driver interfaces |
| packages/driver-test-support/lib/unit-suite.js | Removed legacy getSessions tests in favor of new endpoint tests |
| packages/base-driver/* | Updated deprecation notices and added sessionCreationTimestampMs field initialization |
| packages/appium/* | Refactored insecure feature logic, updated GET sessions endpoint, and enhanced CLI/docs usage |
| packages/appium/docs/* | Revised insecure feature and CLI argument documentation |
Comments suppressed due to low confidence (1)
packages/driver-test-support/lib/unit-suite.js:93
- The removal of legacy getSessions tests suggests that there is no dedicated test validating the new 'created' field in the GET /appium/sessions endpoint. Please add tests specifically covering the correctness of the created timestamp.
it('should return sessions if no session exists', async function () {
packages/appium/lib/appium.js
Outdated
| * Configures insecure features according to the values in `args.relaxedSecurityEnabled`, | ||
| * `args.allowInsecure`, and `args.denyInsecure`, and informs the user about any | ||
| * globally-applied features. | ||
| * Uses `logger` instead of `this.log` to reduce user confusion. |
There was a problem hiding this comment.
why using this.log is confusing?
There was a problem hiding this comment.
It's a minor thing, but when the server is launched, all the default log messages have the [Appium] prefix. Using this.log would cause these messages to use the [AppiumDriver@<hash>] prefix.
There was a problem hiding this comment.
it's not the only log line that looks like that. Not sure why it should be confusing
There was a problem hiding this comment.
From what I can tell, it would be the only one that has this prefix out of the lines shown upon server launch. Once a session is created, the log is filled with all sorts of prefixes, including this one, but all lines in the default state (without a session) only have the [Appium] prefix.
There was a problem hiding this comment.
maybe then it might be a good idea to isolate this security-related stuff in a separate mixin module, similarly to https://github.com/appium/appium/blob/master/packages/appium/lib/bidi-commands.ts instead of putting helpers to utils and to the driver main module?
There was a problem hiding this comment.
Moved them to insecure-features.ts. It could still be improved by reducing duplication, though.
There was a problem hiding this comment.
Nicely done @eglitise
I think it's ok to merge the PR is the current state if you are happy about it.
Proposed changes
This PR adds an implementation for the
GET /appium/sessionsendpoint, according to #20880.The target branch is intentionally set to
master, notappium3- seeing asGET /sessionshas already been removed in Appium 3, it is beneficial for Appium 2 users to already have access to its replacement endpoint, for ease of migration.I confirmed that these changes work using the Inspector (which already has support for this endpoint). Still, it would also be beneficial to add new tests specifically for validating the new
createdfield - any suggestions welcome.Types of changes
What types of changes does your code introduce to Appium?
Put an
xin the boxes that applyChecklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...