Unify base path in HttpService by mshustov · Pull Request #38237 · elastic/kibana

mshustov · 2019-06-06T07:46:42Z

Summary

This PR contains breaking changes for NP plugins

Closes #35816
@eliperelman agreed on the proposed interface #36690 (comment)
as he is on vacation I implemented for both client and server.

What's done:

client and server use common modifyUrl helpers.
on client and server provide unified interfaces

// client
interface BasePath {
    get: () => string;
    prepend: (url: string) => string;
    remove: (url: string) => string;
};
// server
interface BasePath {
    // probably we will make request optional to return default basePath from config
    get: (request: KibanaRequest | Request) => string;
    set: (request: KibanaRequest | Request, basePath: string) => void;
    prepend: (url: string) => string;
    remove: (url: string) => string;
};

server code was enhanced with additional methods, as Legacy platform implements the same logic in plugins https://github.com/restrry/kibana/blob/191ad4caba9f73e6b0648974ff28bad7c806a168/src/legacy/ui/ui_render/ui_render_mixin.js#L121
https://github.com/restrry/kibana/blob/191ad4caba9f73e6b0648974ff28bad7c806a168/x-pack/plugins/reporting/common/get_absolute_url.ts#L25
cleaned up HttpServer from BasePath specific logic

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~[ ] This was checked for cross-browser compatibility, including a check against IE11~~
~~[ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
~~[ ] This was checked for keyboard-only and screenreader accessibility~~

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

Dev Docs

All requests to Kibana server should be concerned about server.basePath option, which affects all routes when Kibana is run behind a proxy. To simplify URL manipulations to include/exclude server.basePath part, New platform collocates under basePath namespace a set of helpers as a part of CoreSetup.http contract:

// on client side
basePath: {
  get: () => string;
  prepend: (url: string) => string;
  remove: (url: string) => string;
};
// on server side
basePath {
  get: (request: KibanaRequest | Request) => string;
  set: (request: KibanaRequest | Request, basePath: string) => void;
  prepend: (url: string) => string;
  remove: (url: string) => string;
};

elasticmachine · 2019-06-06T07:46:44Z

Pinging @elastic/kibana-platform

mshustov · 2019-06-06T07:48:08Z

src/core/utils/url.ts

-  slashes: boolean | null;
-  port: string | null;
-  query: ParsedUrlQuery | {};
+  auth?: string | null;


@azasypkin what do you think if we relay on node typings and get rid of null?

import { UrlWithParsedQuery } from 'url'; export type URLMeaningfulParts = Pick< UrlWithParsedQuery, 'auth' | 'hash' | 'hostname' | 'pathname' | 'protocol' | 'slashes' | 'port' | 'query' >;

I'm fine if Node typings are correct. IIRC last time I checked these didn't reflect completely what was happening in reality, like Node typings were saying that auth/whatever?: string, but in reality it was auth: null | string.

Indeed, it works as you described and that's very confusing. Let's keep it as is. I will add a comment to re-consider behavior with newer @types/node version, because the latest v10.12.30 has the same problems.

We should consider submitting an upstream PR, in my experience it's pretty quick to get fixes merged into the types project.

mshustov · 2019-06-06T07:50:26Z

src/core/server/http/base_path_service.ts

+  public prepend = (path: string): string => {
+    if (!this.basePath) return path;
+    return modifyUrl(path, parts => {
+      if (!parts.hostname && parts.pathname && parts.pathname.startsWith('/')) {


logic was copied from https://github.com/restrry/kibana/blob/master/src/core/public/http/http_setup.ts#L50-L56

mshustov · 2019-06-06T07:55:51Z

@joshdover should we attach release_note:breaking label for experimental API?

elasticmachine · 2019-06-06T09:10:09Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

mshustov · 2019-06-06T09:47:31Z

src/core/server/http/http_server.ts

+  basePath: {
+    get: (request: KibanaRequest | Request) => string;
+    set: (request: KibanaRequest | Request, basePath: string) => void;
+    prepend: (url: string) => string;


prepend on client and server are different a bit. basePath can be request specific on the server side, so probably we can extend api to

prepend: (url: string) => string; prepend: (request: KibanaRequest | Request, url: string) => string;

in our codebase https://github.com/restrry/kibana/blob/33c08f4e20b92e36c8a0df37528f410ad808d73c/x-pack/plugins/security/server/lib/authentication/providers/basic.ts#L93

The request-specific basePath is needed because of spaces, correct? The client still needs the space prefix in its basePath, but the user can only be on a single space at a given time, where as the server may be serving multiple spaces at a time. For this reason, the request needs to be passed to the basePath methods on the server each time we get the basePath, but on the client, the current space is globally configured so this is not needed.

Just trying to make sure I understand.

with handler RFC we might want to unify them, but atm it works as you described.

Another way to make prepend and other helpers request specific would be to make these static functions on the BasePath class:

prepend: (basepath: Basepath, url) => string;

This way these helpers are exactly the same between client and server and there's only one type signature.

in this case we have to create BastPath instance for every Request. don't we?

yeah it gets a bit weird... BasePath as a data type isn't that useful since it's really just a string, so we could just accept a string instead:

prepend: (basepath: string, url) => string;

that's the thing, end-user doesn't know basepath value, it is stored in the core by Spaces plugin

src/core/server/http/http_server.ts

src/core/server/http/base_path_service.ts

joshdover · 2019-06-06T15:17:11Z

src/core/server/http/http_server.ts

+  basePath: {
+    get: (request: KibanaRequest | Request) => string;
+    set: (request: KibanaRequest | Request, basePath: string) => void;
+    prepend: (url: string) => string;


The request-specific basePath is needed because of spaces, correct? The client still needs the space prefix in its basePath, but the user can only be on a single space at a given time, where as the server may be serving multiple spaces at a time. For this reason, the request needs to be passed to the basePath methods on the server each time we get the basePath, but on the client, the current space is globally configured so this is not needed.

Just trying to make sure I understand.

elasticmachine · 2019-06-07T15:42:40Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-06-11T09:30:28Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

mshustov · 2019-06-11T12:20:03Z

retest

rudolf · 2019-06-11T12:41:05Z

src/core/server/http/http_server.ts

+  basePath: {
+    get: (request: KibanaRequest | Request) => string;
+    set: (request: KibanaRequest | Request, basePath: string) => void;
+    prepend: (url: string) => string;


Another way to make prepend and other helpers request specific would be to make these static functions on the BasePath class:

prepend: (basepath: Basepath, url) => string;

This way these helpers are exactly the same between client and server and there's only one type signature.

rudolf · 2019-06-11T12:47:00Z

src/core/server/http/base_path_service.test.ts

+      expect(basePath.prepend('/a/b?x=y#c/d/e')).toBe('/foo/bar/a/b?x=y#c/d/e');
+    });
+
+    it('returns the path unchanged if it does not start with a slash', () => {


Optional: This is the same as the existing behaviour, but I'm not sure why we implemented this behaviour. It feels like your code can silently fail if you forget a / in a path you're prepending. The only way you would notice this mistake is by testing it and noticing that the basepath was never added which feels brittle.

Is there a reason we can't handle this case by inserting a /? If there is a good reason we don't want to accept strings not starting with / should we perhaps throw an exception to show that this is unexpected/unsupported behaviour instead of pretending like we're giving you a prepended basepath when we are not.

ping @spalger as an author

I agree that the implicitness is undesirable. I originally did it this way so that the code would ignore fully resolved urls, but I'm not sure if that's being used... sounds like a really tricky thing to figure out.

rudolf · 2019-06-11T12:51:41Z

src/core/utils/url.ts

-  slashes: boolean | null;
-  port: string | null;
-  query: ParsedUrlQuery | {};
+  auth?: string | null;


We should consider submitting an upstream PR, in my experience it's pretty quick to get fixes merged into the types project.

elasticmachine · 2019-06-11T13:24:22Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

joshdover · 2019-06-13T14:31:38Z

@joshdover should we attach release_note:breaking label for experimental API?

Got the answer from the docs team: only use release_note:dev_docs for breaking changes to experimental APIs.

elasticmachine · 2019-06-16T12:17:51Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

* unify modifyUrl on client and server * create BasePath as a separate entity on server * use BasePath class in http server * use BasePath a separate entity on client * use BasePath class on Http service on the client * switch client code to the new api * improve setver http service mocks * address comments #1 * address comments #2 * update docs * add comment why we define own typings

mshustov added 7 commits June 6, 2019 08:48

unify modifyUrl on client and server

8f1f85f

create BasePath as a separate entity on server

43d5614

use BasePath class in http server

817e3ac

use BasePath a separate entity on client

3a7d69d

use BasePath class on Http service on the client

280cb57

switch client code to the new api

81bdc06

improve setver http service mocks

191ad4c

mshustov commented Jun 6, 2019

View reviewed changes

mshustov added the review label Jun 6, 2019

mshustov marked this pull request as ready for review June 6, 2019 07:54

mshustov requested a review from a team as a code owner June 6, 2019 07:54

mshustov added release_note:breaking and removed release_note:breaking labels Jun 6, 2019

mshustov commented Jun 6, 2019

View reviewed changes

joshdover reviewed Jun 6, 2019

View reviewed changes

mshustov added 3 commits June 7, 2019 15:56

address comments #1

5a334db

Merge branch 'master' into issue-35816-unify-base-path

491e830

address comments #2

76ac6c1

mshustov requested a review from joshdover June 7, 2019 14:22

update docs

b395297

elastic deleted a comment from elasticmachine Jun 7, 2019

mshustov added 2 commits June 11, 2019 10:09

add comment why we define own typings

1839fc4

Merge branch 'master' into issue-35816-unify-base-path

1e6a08c

rudolf reviewed Jun 11, 2019

View reviewed changes

joshdover removed the release_note:breaking label Jun 13, 2019

mshustov requested review from a team and rudolf June 14, 2019 11:59

joshdover approved these changes Jun 14, 2019

View reviewed changes

Merge branch 'master' into issue-35816-unify-base-path

1f5682b

mshustov removed the review label Jun 16, 2019

mshustov merged commit de5c452 into elastic:master Jun 16, 2019

mshustov deleted the issue-35816-unify-base-path branch June 16, 2019 14:36

mshustov mentioned this pull request Jun 16, 2019

Spaces - New Platform Migration, Step 1 #35429

Merged

11 tasks

mshustov mentioned this pull request Jun 16, 2019

Unify base path in HttpService (#38237) #39049

Merged

This was referenced Nov 15, 2019

Loading maps, discover, visualize and dashboards logs backend.js:6 TypeError: (0 , _url.modifyUrl) is not a function in console #50173

Closed

[Maps] prevent users from overflowing URL when filtering by shape #50747

Merged

Conversation

mshustov commented Jun 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Dev Docs

Uh oh!

elasticmachine commented Jun 6, 2019

Uh oh!

mshustov Jun 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mshustov commented Jun 6, 2019

Uh oh!

elasticmachine commented Jun 6, 2019

💔 Build Failed

Uh oh!

mshustov Jun 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mshustov Jun 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Jun 7, 2019

💚 Build Succeeded

Uh oh!

elasticmachine commented Jun 11, 2019

💔 Build Failed

Uh oh!

mshustov commented Jun 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Jun 11, 2019

💚 Build Succeeded

Uh oh!

joshdover commented Jun 13, 2019

Uh oh!

elasticmachine commented Jun 16, 2019

💚 Build Succeeded

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

mshustov commented Jun 6, 2019 •

edited

Loading

mshustov Jun 6, 2019 •

edited

Loading

mshustov Jun 6, 2019 •

edited

Loading

mshustov Jun 7, 2019 •

edited

Loading