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 7 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
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 @@ -101,6 +103,7 @@ export class CasePlugin {
initCaseApi({
logger: this.log,
router,
kibanaVersion: this.kibanaVersion,
});
}

Expand Down
10 changes: 8 additions & 2 deletions x-pack/plugins/cases/server/routes/api/cases/get_case.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,10 @@
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) {
export function initGetCaseApi({ router, logger, kibanaVersion }: RouteDeps) {
router.get(
{
path: CASE_DETAILS_URL,
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,9 @@ 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(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,10 +8,13 @@
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';

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

return response.ok({
headers: {
...getWarningHeader(kibanaVersion),
},
body: await client.attachments.getAll({
caseID: request.params.case_id,
}),
Expand Down
3 changes: 2 additions & 1 deletion x-pack/plugins/cases/server/routes/api/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,14 @@
* 2.0.
*/

import type { Logger } from 'kibana/server';
import type { Logger, PluginInitializerContext } from 'kibana/server';

import type { CasesRouter } from '../../types';

export interface RouteDeps {
router: CasesRouter;
logger: Logger;
kibanaVersion: PluginInitializerContext['env']['packageInfo']['version'];
}

export interface TotalCommentByCase {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,13 @@
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';

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

return response.ok({
headers: {
...getWarningHeader(kibanaVersion),
},
body: await casesClient.userActions.getAll({ caseId }),
});
} catch (error) {
Expand Down
13 changes: 12 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,15 @@ export function wrapError(
}

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

/**
* Creates a warning header with a message formatted according to RFC7234.
* We follow the same formatting as Elasticsearch
* https://github.com/elastic/elasticsearch/blob/5baabff6670a8ed49297488ca8cac8ec12a2078d/server/src/main/java/org/elasticsearch/common/logging/HeaderWarning.java#L55
*/
export const getWarningHeader = (
kibanaVersion: string,
msg: string | undefined = 'Deprecated endpoint'
): { warning: string } => ({
warning: `299 Kibana-${kibanaVersion} "${msg}"`,
});
12 changes: 12 additions & 0 deletions x-pack/test/cases_api_integration/common/lib/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1131,3 +1131,15 @@ export const getServiceNowSimulationServer = async (): Promise<{

return { server, url };
};

/**
* Extracts the warning value a warning header that is formatted according to RFC 7234.
* For example for the string 299 Kibana-8.1.0 "Deprecation endpoint", the return value is Deprecation endpoint.
*
*/
export const extractWarningValueFromWarningHeader = (warningHeader: string) => {
const firstQuote = warningHeader.indexOf('"');
const lastQuote = warningHeader.length - 1;
const warningValue = warningHeader.substring(firstQuote + 1, lastQuote);
return warningValue;
};
13 changes: 13 additions & 0 deletions x-pack/test/cases_api_integration/common/lib/validation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,3 +37,16 @@ export function arraysToEqual(array1?: object[], array2?: object[]) {
const array1AsSet = new Set(array1);
return array2.every((item) => array1AsSet.has(item));
}

/**
* Regular expression to test if a string matches the RFC7234 specification (without warn-date) for warning headers. This pattern assumes that the warn code
* is always 299. Further, this pattern assumes that the warn agent represents a version of Kibana.
*
* Example: 299 Kibana-8.2.0 "Deprecated endpoint"
*/
const WARNING_HEADER_REGEX = /299 Kibana-\d+.\d+.\d+ \".+\"/g;

export const assertWarningHeader = (warningHeader: string) => {
const res = warningHeader.match(WARNING_HEADER_REGEX);
expect(res).not.to.be(null);
};
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import expect from '@kbn/expect';
import { FtrProviderContext } from '../../../../common/ftr_provider_context';

import { AttributesTypeUser } from '../../../../../../plugins/cases/common/api';
import { AttributesTypeUser, getCaseDetailsUrl } from '../../../../../../plugins/cases/common/api';
import { CASES_URL } from '../../../../../../plugins/cases/common/constants';
import {
defaultUser,
Expand All @@ -24,6 +24,8 @@ import {
createComment,
removeServerGeneratedPropertiesFromCase,
removeServerGeneratedPropertiesFromSavedObject,
assertWarningHeader,
extractWarningValueFromWarningHeader,
} from '../../../../common/lib/utils';
import {
secOnly,
Expand Down Expand Up @@ -191,5 +193,18 @@ export default ({ getService }: FtrProviderContext): void => {
});
});
});

describe('deprecations', () => {
it('should return a warning header', async () => {
const theCase = await createCase(supertest, postCaseReq);
const res = await supertest.get(getCaseDetailsUrl(theCase.id)).expect(200);
const warningHeader = res.header.warning;

assertWarningHeader(warningHeader);

const warningValue = extractWarningValueFromWarningHeader(warningHeader);
expect(warningValue).to.be('Deprecated query parameter includeComments');
});
});
});
};
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ import {
createComment,
getAllComments,
superUserSpace1Auth,
assertWarningHeader,
extractWarningValueFromWarningHeader,
} from '../../../../common/lib/utils';
import {
globalRead,
Expand All @@ -27,6 +29,7 @@ import {
secOnlyRead,
superUser,
} from '../../../../common/lib/authentication/users';
import { getCaseCommentsUrl } from '../../../../../../plugins/cases/common/api';

// eslint-disable-next-line import/no-default-export
export default ({ getService }: FtrProviderContext): void => {
Expand Down Expand Up @@ -148,5 +151,18 @@ export default ({ getService }: FtrProviderContext): void => {
});
});
});

describe('deprecations', () => {
it('should return a warning header', async () => {
const theCase = await createCase(supertest, postCaseReq);
const res = await supertest.get(getCaseCommentsUrl(theCase.id)).expect(200);
const warningHeader = res.header.warning;

assertWarningHeader(warningHeader);

const warningValue = extractWarningValueFromWarningHeader(warningHeader);
expect(warningValue).to.be('Deprecated endpoint');
});
});
});
};
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
CaseStatuses,
CommentType,
ConnectorTypes,
getCaseUserActionUrl,
} from '../../../../../../plugins/cases/common/api';
import { CreateCaseUserAction } from '../../../../../../plugins/cases/common/api/cases/user_actions/create_case';
import { postCaseReq, postCommentUserReq, getPostCaseRequest } from '../../../../common/lib/mock';
Expand All @@ -26,6 +27,8 @@ import {
createComment,
updateComment,
deleteComment,
assertWarningHeader,
extractWarningValueFromWarningHeader,
} from '../../../../common/lib/utils';
import {
globalRead,
Expand Down Expand Up @@ -354,5 +357,18 @@ export default ({ getService }: FtrProviderContext): void => {
});
}
});

describe('deprecations', () => {
it('should return a warning header', async () => {
const theCase = await createCase(supertest, postCaseReq);
const res = await supertest.get(getCaseUserActionUrl(theCase.id)).expect(200);
const warningHeader = res.header.warning;

assertWarningHeader(warningHeader);

const warningValue = extractWarningValueFromWarningHeader(warningHeader);
expect(warningValue).to.be('Deprecated endpoint');
});
});
});
};