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

[Cases] Deprecate endpoints #124773

Merged
merged 18 commits into from
Feb 11, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions x-pack/plugins/cases/server/client/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import { createMetricsSubClient, MetricsSubClient } from './metrics/client';
* Client wrapper that contains accessor methods for individual entities within the cases system.
*/
export class CasesClient {
private readonly _kibanaVersion: CasesClientArgs['kibanaVersion'];
private readonly _casesClientInternal: CasesClientInternal;
private readonly _cases: CasesSubClient;
private readonly _attachments: AttachmentsSubClient;
Expand All @@ -27,6 +28,7 @@ export class CasesClient {
private readonly _metrics: MetricsSubClient;

constructor(args: CasesClientArgs) {
this._kibanaVersion = args.kibanaVersion;
this._casesClientInternal = createCasesClientInternal(args);
this._cases = createCasesSubClient(args, this, this._casesClientInternal);
this._attachments = createAttachmentsSubClient(args, this, this._casesClientInternal);
Expand All @@ -36,6 +38,13 @@ export class CasesClient {
this._metrics = createMetricsSubClient(args, this);
}

/**
* Retrieves the current kibana version
*/
public get kibanaVersion() {
return this._kibanaVersion;
}

/**
* Retrieves an interface for interacting with cases entities.
*/
Expand Down
3 changes: 3 additions & 0 deletions x-pack/plugins/cases/server/client/factory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
SavedObjectsServiceStart,
Logger,
ElasticsearchClient,
PluginInitializerContext,
} from 'kibana/server';
import { SecurityPluginSetup, SecurityPluginStart } from '../../../security/server';
import { SAVED_OBJECT_TYPES } from '../../common/constants';
Expand All @@ -31,6 +32,7 @@ import { AuthorizationAuditLogger } from '../authorization';
import { CasesClient, createCasesClient } from '.';

interface CasesClientFactoryArgs {
kibanaVersion: PluginInitializerContext['env']['packageInfo']['version'];
securityPluginSetup?: SecurityPluginSetup;
securityPluginStart?: SecurityPluginStart;
getSpace: GetSpaceFn;
Expand Down Expand Up @@ -113,6 +115,7 @@ export class CasesClientFactory {
lensEmbeddableFactory: this.options.lensEmbeddableFactory,
authorization: auth,
actionsClient: await this.options.actionsPluginStart.getActionsClientWithRequest(request),
kibanaVersion: this.options.kibanaVersion,
});
}
}
3 changes: 2 additions & 1 deletion x-pack/plugins/cases/server/client/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
*/

import type { PublicMethodsOf } from '@kbn/utility-types';
import { SavedObjectsClientContract, Logger } from 'kibana/server';
import { SavedObjectsClientContract, Logger, PluginInitializerContext } from 'kibana/server';
import { User } from '../../common/api';
import { Authorization } from '../authorization/authorization';
import {
Expand All @@ -24,6 +24,7 @@ import { LensServerPluginSetup } from '../../../lens/server';
* Parameters for initializing a cases client
*/
export interface CasesClientArgs {
readonly kibanaVersion: PluginInitializerContext['env']['packageInfo']['version'];
readonly caseConfigureService: CaseConfigureService;
readonly caseService: CasesService;
readonly connectorMappingsService: ConnectorMappingsService;
Expand Down
3 changes: 3 additions & 0 deletions x-pack/plugins/cases/server/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,11 +59,13 @@ export interface PluginStartContract {

export class CasePlugin {
private readonly log: Logger;
private readonly kibanaVersion: PluginInitializerContext['env']['packageInfo']['version'];
private clientFactory: CasesClientFactory;
private securityPluginSetup?: SecurityPluginSetup;
private lensEmbeddableFactory?: LensServerPluginSetup['lensEmbeddableFactory'];

constructor(private readonly initializerContext: PluginInitializerContext) {
this.kibanaVersion = initializerContext.env.packageInfo.version;
cnasikas marked this conversation as resolved.
Show resolved Hide resolved
this.log = this.initializerContext.logger.get();
this.clientFactory = new CasesClientFactory(this.log);
}
Expand Down Expand Up @@ -121,6 +123,7 @@ export class CasePlugin {
*/
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
lensEmbeddableFactory: this.lensEmbeddableFactory!,
kibanaVersion: this.kibanaVersion,
});

const client = core.elasticsearch.client;
Expand Down
11 changes: 10 additions & 1 deletion x-pack/plugins/cases/server/routes/api/cases/get_case.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import { schema } from '@kbn/config-schema';

import { RouteDeps } from '../types';
import { wrapError } from '../utils';
import { getWarningHeader, wrapError } from '../utils';
import { CASE_DETAILS_URL } from '../../../../common/constants';

export function initGetCaseApi({ router, logger }: RouteDeps) {
Expand All @@ -20,6 +20,9 @@ export function initGetCaseApi({ router, logger }: RouteDeps) {
case_id: schema.string(),
}),
query: schema.object({
/**
* @deprecated since version 8.1.0
*/
includeComments: schema.boolean({ defaultValue: true }),
}),
},
Expand All @@ -30,6 +33,12 @@ export function initGetCaseApi({ router, logger }: RouteDeps) {
const id = request.params.case_id;

return response.ok({
headers: {
Copy link
Member

Choose a reason for hiding this comment

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

It looks like the warning header will be returned on every call to this endpoint - but don't you just want the header if they use the query.includeComments property?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, good point! I will update accordingly.

Copy link
Member Author

@cnasikas cnasikas Feb 8, 2022

Choose a reason for hiding this comment

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

@pmuellr I just noticed that we default it to true if the user omits it so I don't think we can distinguish if the user provided the parameter or not.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, bummer.

You could change the type to boolean | undefined, and then apply the default outside of the validation, but that's clumsy, and changes your typing which could require more undefined checks. Perhaps instead, since false is the "deprecated" version, you can just check after the validation if it's false and then add the header? So it wouldn't be marked deprecated if they used the param and it was true.

Another possibility would be to see if there's a way to get the parameters in more of a "raw" state, after the validation, from the request. And then put the check there.

I really don't think we want this header used every time the API is used.

Copy link
Member Author

Choose a reason for hiding this comment

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

@pmuellr Amazing idea about the "raw" state. I pushed the changes you suggested.

...getWarningHeader(
casesClient.kibanaVersion,
'Deprecated query parameter includeComments'
),
},
body: await casesClient.cases.get({
id,
includeComments: request.query.includeComments,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,12 @@
import { schema } from '@kbn/config-schema';

import { RouteDeps } from '../types';
import { wrapError } from '../utils';
import { wrapError, getWarningHeader } from '../utils';
import { CASE_COMMENTS_URL } from '../../../../common/constants';

/**
* @deprecated since version 8.1.0
*/
export function initGetAllCommentsApi({ router, logger }: RouteDeps) {
router.get(
{
Expand All @@ -26,6 +29,9 @@ export function initGetAllCommentsApi({ router, logger }: RouteDeps) {
const client = await context.cases.getCasesClient();

return response.ok({
headers: {
...getWarningHeader(client.kibanaVersion),
},
body: await client.attachments.getAll({
caseID: request.params.case_id,
}),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,12 @@
import { schema } from '@kbn/config-schema';

import { RouteDeps } from '../types';
import { wrapError } from '../utils';
import { getWarningHeader, wrapError } from '../utils';
import { CASE_USER_ACTIONS_URL } from '../../../../common/constants';

/**
* @deprecated since version 8.1.0
*/
export function initGetAllCaseUserActionsApi({ router, logger }: RouteDeps) {
router.get(
{
Expand All @@ -31,6 +34,9 @@ export function initGetAllCaseUserActionsApi({ router, logger }: RouteDeps) {
const caseId = request.params.case_id;

return response.ok({
headers: {
...getWarningHeader(casesClient.kibanaVersion),
},
body: await casesClient.userActions.getAll({ caseId }),
});
} catch (error) {
Expand Down
8 changes: 7 additions & 1 deletion x-pack/plugins/cases/server/routes/api/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
*/

import { Boom, boomify, isBoom } from '@hapi/boom';

import { schema } from '@kbn/config-schema';
import { CustomHttpResponseOptions, ResponseError } from 'kibana/server';
import { CaseError, isCaseError, HTTPError, isHTTPError } from '../../common/error';
Expand Down Expand Up @@ -35,3 +34,10 @@ export function wrapError(
}

export const escapeHatch = schema.object({}, { unknowns: 'allow' });

export const getWarningHeader = (
kibanaVersion: string,
msg: string | undefined = 'Deprecated endpoint'
): { warning: string } => ({
warning: `299 Kibana-${kibanaVersion} "${msg}"`,
});