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

HubStatusServlet - use GET with query string instead of GET with body #2771

Merged
merged 1 commit into from
Sep 20, 2016

Conversation

tommywo
Copy link
Contributor

@tommywo tommywo commented Sep 14, 2016

@tommywo tommywo changed the title HubStatusServlet - use GET with query string instead GET with body HubStatusServlet - use GET with query string instead of GET with body Sep 14, 2016
@tommywo tommywo force-pushed the master branch 2 times, most recently from da68765 to b236b6e Compare September 14, 2016 13:05
@lukeis
Copy link
Member

lukeis commented Sep 14, 2016

can you make it allow both POST data and url parameter? I'm a bit concerned here about breaking people unnecessarily.

@mach6

@mach6
Copy link
Member

mach6 commented Sep 14, 2016

I share the concerns of @lukeis -- It should continue support (although it is odd from an HTTP protocol point of view) GET with body too.

All said, personally I'm in the opinion that the HubStatusServlet needs to be deprecated in 3.0.0 and replaced with a REST api for getting "hub" status which includes the information this servlet provides and more.

It's also worth mentioning that nodes query this servlet.

@tommywo
Copy link
Contributor Author

tommywo commented Sep 15, 2016

It's now backward compatible with GET with body.
I like the idea of REST API (i see there is pr open for it already -#265)

Copy link
Member

@mach6 mach6 left a comment

Choose a reason for hiding this comment

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

Other than the inconsistent use of whitespace, this change looks good to me.

Any chance you can clean up the whitespace? :)

@@ -101,7 +102,10 @@ private JsonObject getResponse(HttpServletRequest request) throws IOException {
if (request.getInputStream() != null) {
JsonObject requestJSON = getRequestJSON(request);
List<String> keysToReturn = null;
if (requestJSON != null && requestJSON.has("configuration")) {

if (request.getParameter("configuration")!=null && !"".equals(request.getParameter("configuration"))) {
Copy link
Member

Choose a reason for hiding this comment

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

inconsistent use of whitespace here

keys.add(URLEncoder.encode("I'm not a valid key", "UTF-8"));
keys.add(URLEncoder.encode("servlets", "UTF-8"));

String query = "?configuration="+String.join(",",keys);
Copy link
Member

Choose a reason for hiding this comment

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

inconsistent use of whitespace here

@tommywo
Copy link
Contributor Author

tommywo commented Sep 19, 2016

@mach6 done

@tommywo
Copy link
Contributor Author

tommywo commented Sep 20, 2016

@lukeis is it ok now? can you merge?

@lukeis lukeis merged commit 978350b into SeleniumHQ:master Sep 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants