Improve Short URL HTTP error semantics (#128866)

* return 409 status code on duplicate slug

* support 404 error in by-slug resolution

* remove mime type header for errors

* harden error code type
This commit is contained in:
Vadim Kibana 2022-03-30 14:30:54 +02:00 committed by GitHub
parent 141081ea2a
commit dd81761869
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
10 changed files with 83 additions and 28 deletions

View file

@ -0,0 +1,15 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/
export type UrlServiceErrorCode = 'SLUG_EXISTS' | 'NOT_FOUND' | '';
export class UrlServiceError extends Error {
constructor(message: string, public readonly code: UrlServiceErrorCode = '') {
super(message);
}
}

View file

@ -8,6 +8,7 @@
import { schema } from '@kbn/config-schema'; import { schema } from '@kbn/config-schema';
import { IRouter } from 'kibana/server'; import { IRouter } from 'kibana/server';
import { UrlServiceError } from '../../error';
import { ServerUrlService } from '../../types'; import { ServerUrlService } from '../../types';
export const registerCreateRoute = (router: IRouter, url: ServerUrlService) => { export const registerCreateRoute = (router: IRouter, url: ServerUrlService) => {
@ -41,26 +42,35 @@ export const registerCreateRoute = (router: IRouter, url: ServerUrlService) => {
if (!locator) { if (!locator) {
return res.customError({ return res.customError({
statusCode: 409, statusCode: 409,
headers: {
'content-type': 'application/json',
},
body: 'Locator not found.', body: 'Locator not found.',
}); });
} }
const shortUrl = await shortUrls.create({ try {
locator, const shortUrl = await shortUrls.create({
params, locator,
slug, params,
humanReadableSlug, slug,
}); humanReadableSlug,
});
return res.ok({ return res.ok({
headers: { headers: {
'content-type': 'application/json', 'content-type': 'application/json',
}, },
body: shortUrl.data, body: shortUrl.data,
}); });
} catch (error) {
if (error instanceof UrlServiceError) {
if (error.code === 'SLUG_EXISTS') {
return res.customError({
statusCode: 409,
body: error.message,
});
}
}
throw error;
}
}) })
); );
}; };

View file

@ -8,6 +8,7 @@
import { schema } from '@kbn/config-schema'; import { schema } from '@kbn/config-schema';
import { IRouter } from 'kibana/server'; import { IRouter } from 'kibana/server';
import { UrlServiceError } from '../../error';
import { ServerUrlService } from '../../types'; import { ServerUrlService } from '../../types';
export const registerResolveRoute = (router: IRouter, url: ServerUrlService) => { export const registerResolveRoute = (router: IRouter, url: ServerUrlService) => {
@ -26,15 +27,28 @@ export const registerResolveRoute = (router: IRouter, url: ServerUrlService) =>
router.handleLegacyErrors(async (ctx, req, res) => { router.handleLegacyErrors(async (ctx, req, res) => {
const slug = req.params.slug; const slug = req.params.slug;
const savedObjects = ctx.core.savedObjects.client; const savedObjects = ctx.core.savedObjects.client;
const shortUrls = url.shortUrls.get({ savedObjects });
const shortUrl = await shortUrls.resolve(slug);
return res.ok({ try {
headers: { const shortUrls = url.shortUrls.get({ savedObjects });
'content-type': 'application/json', const shortUrl = await shortUrls.resolve(slug);
},
body: shortUrl.data, return res.ok({
}); headers: {
'content-type': 'application/json',
},
body: shortUrl.data,
});
} catch (error) {
if (error instanceof UrlServiceError) {
if (error.code === 'NOT_FOUND') {
return res.customError({
statusCode: 404,
body: error.message,
});
}
}
throw error;
}
}) })
); );
}; };

View file

@ -10,3 +10,4 @@ export * from './types';
export * from './short_urls'; export * from './short_urls';
export { registerUrlServiceRoutes } from './http/register_url_service_routes'; export { registerUrlServiceRoutes } from './http/register_url_service_routes';
export { registerUrlServiceSavedObjectType } from './saved_objects/register_url_service_saved_object_type'; export { registerUrlServiceSavedObjectType } from './saved_objects/register_url_service_saved_object_type';
export * from './error';

View file

@ -12,6 +12,7 @@ import { LegacyShortUrlLocatorDefinition } from '../../../common/url_service/loc
import { MemoryShortUrlStorage } from './storage/memory_short_url_storage'; import { MemoryShortUrlStorage } from './storage/memory_short_url_storage';
import { SerializableRecord } from '@kbn/utility-types'; import { SerializableRecord } from '@kbn/utility-types';
import { SavedObjectReference } from 'kibana/server'; import { SavedObjectReference } from 'kibana/server';
import { UrlServiceError } from '../error';
const setup = () => { const setup = () => {
const currentVersion = '1.2.3'; const currentVersion = '1.2.3';
@ -125,7 +126,7 @@ describe('ServerShortUrlClient', () => {
url: '/app/test#foo/bar/baz', url: '/app/test#foo/bar/baz',
}, },
}) })
).rejects.toThrowError(new Error(`Slug "lala" already exists.`)); ).rejects.toThrowError(new UrlServiceError(`Slug "lala" already exists.`, 'SLUG_EXISTS'));
}); });
test('can automatically generate human-readable slug', async () => { test('can automatically generate human-readable slug', async () => {

View file

@ -18,6 +18,7 @@ import type {
ShortUrlData, ShortUrlData,
LocatorData, LocatorData,
} from '../../../common/url_service'; } from '../../../common/url_service';
import { UrlServiceError } from '../error';
import type { ShortUrlStorage } from './types'; import type { ShortUrlStorage } from './types';
import { validateSlug } from './util'; import { validateSlug } from './util';
@ -74,7 +75,7 @@ export class ServerShortUrlClient implements IShortUrlClient {
if (slug) { if (slug) {
const isSlugTaken = await storage.exists(slug); const isSlugTaken = await storage.exists(slug);
if (isSlugTaken) { if (isSlugTaken) {
throw new Error(`Slug "${slug}" already exists.`); throw new UrlServiceError(`Slug "${slug}" already exists.`, 'SLUG_EXISTS');
} }
} }

View file

@ -9,6 +9,7 @@
import type { SerializableRecord } from '@kbn/utility-types'; import type { SerializableRecord } from '@kbn/utility-types';
import { SavedObject, SavedObjectReference, SavedObjectsClientContract } from 'kibana/server'; import { SavedObject, SavedObjectReference, SavedObjectsClientContract } from 'kibana/server';
import { ShortUrlRecord } from '..'; import { ShortUrlRecord } from '..';
import { UrlServiceError } from '../..';
import { LEGACY_SHORT_URL_LOCATOR_ID } from '../../../../common/url_service/locators/legacy_short_url_locator'; import { LEGACY_SHORT_URL_LOCATOR_ID } from '../../../../common/url_service/locators/legacy_short_url_locator';
import { ShortUrlData } from '../../../../common/url_service/short_urls/types'; import { ShortUrlData } from '../../../../common/url_service/short_urls/types';
import { ShortUrlStorage } from '../types'; import { ShortUrlStorage } from '../types';
@ -161,7 +162,7 @@ export class SavedObjectShortUrlStorage implements ShortUrlStorage {
}); });
if (result.saved_objects.length !== 1) { if (result.saved_objects.length !== 1) {
throw new Error('not found'); throw new UrlServiceError('not found', 'NOT_FOUND');
} }
const savedObject = result.saved_objects[0] as ShortUrlSavedObject; const savedObject = result.saved_objects[0] as ShortUrlSavedObject;

View file

@ -131,8 +131,8 @@ export default function ({ getService }: FtrProviderContext) {
slug, slug,
}); });
expect(response1.status === 200).to.be(true); expect(response1.status).to.be(200);
expect(response2.status >= 400).to.be(true); expect(response2.status).to.be(409);
}); });
}); });
}); });

View file

@ -23,6 +23,12 @@ export default function ({ getService }: FtrProviderContext) {
expect(response2.body).to.eql(response1.body); expect(response2.body).to.eql(response1.body);
}); });
it('returns 404 error when short URL does not exist', async () => {
const response = await supertest.get('/api/short_url/NotExistingID');
expect(response.status).to.be(404);
});
it('supports legacy short URLs', async () => { it('supports legacy short URLs', async () => {
const id = 'abcdefghjabcdefghjabcdefghjabcdefghj'; const id = 'abcdefghjabcdefghjabcdefghjabcdefghj';
await supertest.post('/api/saved_objects/url/' + id).send({ await supertest.post('/api/saved_objects/url/' + id).send({

View file

@ -26,6 +26,12 @@ export default function ({ getService }: FtrProviderContext) {
expect(response2.body).to.eql(response1.body); expect(response2.body).to.eql(response1.body);
}); });
it('returns 404 error when short URL does not exist', async () => {
const response = await supertest.get('/api/short_url/_slug/not-existing-slug');
expect(response.status).to.be(404);
});
it('can resolve a short URL by its slug, when slugs are similar', async () => { it('can resolve a short URL by its slug, when slugs are similar', async () => {
const rnd = Math.round(Math.random() * 1e6) + 1; const rnd = Math.round(Math.random() * 1e6) + 1;
const now = Date.now(); const now = Date.now();