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

NP Licensing plugin route handler context #46586

Merged
merged 5 commits into from
Sep 30, 2019

Conversation

legrego
Copy link
Member

@legrego legrego commented Sep 25, 2019

Summary

This PR adds a http route handler context for the licensing plugin. Plugins which depend on the licensing plugin can receive the current license in their route handlers for easy consumption, with the guarantee that their view of the license will remain consistent for the lifetime of the request.

@legrego legrego mentioned this pull request Sep 25, 2019
7 tasks
@@ -80,4 +80,21 @@ describe('licensing plugin', () => {

expect(licenseTypes).toEqual(['basic', 'gold', 'platinum']);
});

test('provides a licensing context to http routes', async () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure of the best way to test the new context. This test is basically useless, so I'm open to ideas. I could write a functional test that stands up another plugin for the sole purpose of testing this, but that felt like overkill...

Copy link
Contributor

Choose a reason for hiding this comment

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

It'd probably be easier to test if you export the context provider function separately from registering it. That way you could have this test to ensure it gets registered, and a separate test that verifies the behavior of the provider function

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good to me! Done in 477bd5b

@legrego legrego marked this pull request as ready for review September 25, 2019 14:49
@legrego legrego added release_note:skip Skip the PR/issue when compiling release notes v7.5.0 v8.0.0 labels Sep 25, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@legrego
Copy link
Member Author

legrego commented Sep 25, 2019

Related, I've been using the kbnTestServer to test my API routes, and so far, this has been working pretty well:

root = kbnTestServer.createRoot();
const { http } = await root.setup();
const router = http.createRouter('/');

This gets tricky with contexts, however. If I wanted to write a test that ensured a specific route responded in a certain way based on the status of the license (provided by context), how would I do that? I managed to create a mock license plugin and have the test server load it for my test, but then injecting a specific license was going to be cumbersome, and it's likely not something we want each plugin replicating whenever they want to test their route based on a specific context value.

All that to say, is there a way for tests to instruct the kbnTestServer to inject mock context values?

@joshdover
Copy link
Contributor

joshdover commented Sep 26, 2019

All that to say, is there a way for tests to instruct the kbnTestServer to inject mock context values?

I'm not too familiar with the kbnTestServer utility, but I think we should probably have an utility in Core similar to Hapi's server.inject that would allow you to override the context values. This could be implemented in the HTTP test mock, since we don't really need the full Hapi server underneath to be able to test routes against the HTTP service's interface.

Example:

import { httpServiceMock } from 'src/core/server/mocks';

// The `createSetupContract` function could return the contract and a `inject`
// function.
const { httpSetup, inject } = httpServiceMock.createSetupContract();

// Register your plugin's routes with the httpSetup
import { registerRoutes } from './routes';
const router = httpSetup.createRouter();
registerRoutes(router);

// Use `inject` to make HTTP requests
const res = await inject({ path: '/myroute' });

// Override context with special mocks. The `context` key could be merged with the default context
const licensing = { license: myMock };
const res = await inject({ path: '/myroute', context: { licensing } });

Alternatively, you could export your handler functions and test them directly, but I think it's good to be able to test validation behavior.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@eliperelman eliperelman left a comment

Choose a reason for hiding this comment

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

Looks great!

@@ -121,8 +122,12 @@ export class Plugin implements CorePlugin<LicensingPluginSetup> {
const config = await this.config$.pipe(first()).toPromise();
const poller = this.create(config, core);

const license$ = poller.subject$.asObservable();
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Remove blank line before this declaration.


describe('licensingRouteHandlerContext', () => {
it('provides the initial license value', async () => {
const { license } = await setup();
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be satisfied by consuming the license$ available here?

const { license$, license } = await setup();
const context = createRouteHandlerContext(license$);
const { license: contextResult } = await context({});

expect(contextResult).toBe(license);

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yep, I missed that license$ was available as part of setup. I'll simplify this!

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@legrego legrego merged commit 4473b14 into elastic:master Sep 30, 2019
@legrego legrego deleted the licensing/http-context branch September 30, 2019 12:19
legrego added a commit to legrego/kibana that referenced this pull request Sep 30, 2019
* introducing a licensing route handler context

* extracting route handler context for easier testing

* address PR feedback
legrego added a commit that referenced this pull request Sep 30, 2019
* introducing a licensing route handler context

* extracting route handler context for easier testing

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

Successfully merging this pull request may close these issues.

4 participants