Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feat]: create sessions endpoint plugin #19856

Open
KazuCocoa opened this issue Mar 4, 2024 · 13 comments
Open

[Feat]: create sessions endpoint plugin #19856

KazuCocoa opened this issue Mar 4, 2024 · 13 comments
Labels

Comments

@KazuCocoa
Copy link
Member

KazuCocoa commented Mar 4, 2024

To cover appium-inspector's usage. It is not in w3c spec, so it would be nice to have it as a plugin instead of base-driver side.

#19851

(if needed)

[update] removed the grid's comment as just an exmaple.

Users should enable this plugin explicitly, so appium server won't expose these info unexpectedly with the /status endpoint call.

@eglitise

This comment was marked as resolved.

@KazuCocoa

This comment was marked as resolved.

@eglitise

This comment was marked as resolved.

@KazuCocoa
Copy link
Member Author

KazuCocoa commented Mar 4, 2024

Current our message is just a string as:

async getStatus() {
// https://www.w3.org/TR/webdriver/#dfn-status
const statusObj = this._isShuttingDown
? {
ready: false,
message: 'The server is shutting down',
}
: {
ready: true,
message: 'The server is ready to accept new connections',
};
return {
...statusObj,
build: _.clone(getBuildInfo()),
};
}

So, maybe adding it with a breaking change could be reasonable...? Maybe then, bumping minor could be good...? (maybe we won't increase appium's major version to v3 with this)

I don't have strong opinion to add it in the message itself.

Or... add a new sessions key in the /status endpoint to keep the message as string...? It could not be for w3c but maybe won't break compatibility

{
  "ready": true or false
  "message": "current string",
  "sessions": {the same body with /sessions}
}

@eglitise
Copy link
Collaborator

eglitise commented Mar 4, 2024

{
  "ready": true or false
  "message": "current string",
  "sessions": {the same body with /sessions}
}

Yes, this is basically what I was thinking of. The ready and message properties are unchanged, and adding any other properties in the response is still perfectly fine within the W3C spec (at least the way I understand it).
I wonder if this even counts as a breaking change. In theory it could break code that only expects ready and message in the response, but, again, the W3C spec appears to allow other arbitrary parameters, meaning any code that expects only those 2 keys isn't W3C compliant to begin with.

@KazuCocoa
Copy link
Member Author

I see, understood. Maybe it could have instead. My main thing in this pr was only for /sessions endpoint in case someone relies on that like the inspector PR's use case.

Let me create a new one to add the body.

@KazuCocoa
Copy link
Member Author

KazuCocoa commented Mar 4, 2024

-> #19857 for the /status endpoint thing

@jlipps
Copy link
Member

jlipps commented Mar 4, 2024

I don't think this should be a plugin. If we want to ensure W3C compatibility, I think it should either be an additional endpoint behind a vendor prefix (GET /appium/sessions, with the same behaviour as currently), or an execute method (appium: getSessions). The disadvantage of the execute method is that I don't think we have any globally defined execute methods on the appium driver, whereas we do have globally handled routes. So implementing the route is likely easier and potentially less prone to interfering with what drivers are doing.

@mykola-mokhnach
Copy link
Collaborator

we currently have nothing to be able to run sessionless execute methods as they still use /session_id/execute/sync API under the hood

@jlipps
Copy link
Member

jlipps commented Mar 4, 2024

(this feature is global enough to be put into the core appium code, IMO. and we could hide it behind a security flag so that administrators must opt into making session data available to consumers)

@mykola-mokhnach
Copy link
Collaborator

also adding a new non-w3c route into the base driver seems like meh... We have recently deprecated most of these

@KazuCocoa
Copy link
Member Author

KazuCocoa commented Mar 4, 2024

i personally like to keep /status minimal so that the server can get many requests without performance concerns etc. So it works like a liveness check endpoint for the appium server instance. (just a comment)

@jlipps
Copy link
Member

jlipps commented Mar 5, 2024

@mykola-mokhnach if you have a good idea for how to add execute method handling to the umbrella server, then I'm ok if we want to do that. Maybe it's not hard.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants