maxmustermann/kibana
1
0
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity

[App Search] Add new encodePathParams helper (fixes unencoded document IDs) (#88648)

Browse source 
* Add encodePathParams helper to EnterpriseSearchRequestHandler

This helper accomplishes two things:

- Fixes 404s from the Enterprise Search server for user-generated IDs with special characters (e.g. ? char)

- Allows us to simplify some of our createRequest calls - no longer will we have to grab request.params to create paths, this helper will do so for us

* Update AS document route to use new helper

- This was the primary view/API I was testing to fix this bug

* Update remaining AS routes to use new helper

- shorter, saves us a few lines
+ remove unnecessary payload: params that doesn't actually validate params

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
This commit is contained in:
Constance 2021-01-20 11:07:32 -08:00 committed by GitHub
parent 8b1a228c29
commit f4f6cb687c
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
12 changed files with 100 additions and 108 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

30
x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.test.ts
View file

@ -116,16 +116,40 @@ describe('EnterpriseSearchRequestHandler', () => {
);
});
it('correctly encodes paths and query string parameters', async () => {
it('correctly encodes query string parameters', async () => {
const requestHandler = enterpriseSearchRequestHandler.createRequest({
path: '/api/some example',
path: '/api/example',
});
await makeAPICall(requestHandler, { query: { 'page[current]': 1 } });
EnterpriseSearchAPI.shouldHaveBeenCalledWith(
'http://localhost:3002/api/some%20example?page%5Bcurrent%5D=1'
'http://localhost:3002/api/example?page%5Bcurrent%5D=1'
);
});
describe('encodePathParams', () => {
it('correctly replaces :pathVariables with request.params', async () => {
const requestHandler = enterpriseSearchRequestHandler.createRequest({
path: '/api/examples/:example/some/:id',
});
await makeAPICall(requestHandler, { params: { example: 'hello', id: 'world' } });
EnterpriseSearchAPI.shouldHaveBeenCalledWith(
'http://localhost:3002/api/examples/hello/some/world'
);
});
it('correctly encodes path params as URI components', async () => {
const requestHandler = enterpriseSearchRequestHandler.createRequest({
path: '/api/examples/:example',
});
await makeAPICall(requestHandler, { params: { example: 'hello#@/$%^/&[]{}/";world' } });
EnterpriseSearchAPI.shouldHaveBeenCalledWith(
'http://localhost:3002/api/examples/hello%23%40%2F%24%25%5E%2F%26%5B%5D%7B%7D%2F%22%3Bworld'
);
});
});
});
describe('response passing', () => {

33
x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.ts
View file

@ -65,11 +65,12 @@ export class EnterpriseSearchRequestHandler {
) => {
try {
// Set up API URL
const encodedPath = this.encodePathParams(path, request.params as Record<string, string>);
const queryParams = { ...(request.query as object), ...params };
const queryString = !this.isEmptyObj(queryParams)
? `?${querystring.stringify(queryParams)}`
: '';
const url = encodeURI(this.enterpriseSearchUrl + path) + queryString;
const url = encodeURI(this.enterpriseSearchUrl) + encodedPath + queryString;
// Set up API options
const { method } = request.route;
@ -126,6 +127,36 @@ export class EnterpriseSearchRequestHandler {
};
}
/**
* This path helper is similar to React Router's generatePath, but much simpler &
* does not use regexes. It enables us to pass a static '/foo/:bar/baz' string to
* createRequest({ path }) and have :bar be automatically replaced by the value of
* request.params.bar.
* It also (very importantly) wraps all URL request params with encodeURIComponent(),
* which is an extra layer of encoding required by the Enterprise Search server in
* order to correctly & safely parse user-generated IDs with special characters in
* their names - just encodeURI alone won't work.
*/
encodePathParams(path: string, params: Record<string, string>) {
const hasParams = path.includes(':');
if (!hasParams) {
return path;
} else {
return path
.split('/')
.map((pathPart) => {
const isParam = pathPart.startsWith(':');
if (!isParam) {
return pathPart;
} else {
const pathParam = pathPart.replace(':', '');
return encodeURIComponent(params[pathParam]);
}
})
.join('/');
}
}
/**
* Attempt to grab a usable error body from Enterprise Search - this isn't
* always possible because some of our internal endpoints send back blank

12
x-pack/plugins/enterprise_search/server/routes/app_search/analytics.test.ts
View file

@ -27,12 +27,8 @@ describe('analytics routes', () => {
});
it('creates a request handler', () => {
mockRouter.callRoute({
params: { engineName: 'some-engine' },
});
expect(mockRequestHandler.createRequest).toHaveBeenCalledWith({
path: '/as/engines/some-engine/analytics/queries',
path: '/as/engines/:engineName/analytics/queries',
});
});
@ -84,12 +80,8 @@ describe('analytics routes', () => {
});
it('creates a request handler', () => {
mockRouter.callRoute({
params: { engineName: 'some-engine', query: 'some-query' },
});
expect(mockRequestHandler.createRequest).toHaveBeenCalledWith({
path: '/as/engines/some-engine/analytics/query/some-query',
path: '/as/engines/:engineName/analytics/query/:query',
});
});

20
x-pack/plugins/enterprise_search/server/routes/app_search/analytics.ts
View file

@ -32,13 +32,9 @@ export function registerAnalyticsRoutes({
query: schema.object(queriesSchema),
},
},
async (context, request, response) => {
const { engineName } = request.params;
return enterpriseSearchRequestHandler.createRequest({
path: `/as/engines/${engineName}/analytics/queries`,
})(context, request, response);
}
enterpriseSearchRequestHandler.createRequest({
path: '/as/engines/:engineName/analytics/queries',
})
);
router.get(
@ -52,12 +48,8 @@ export function registerAnalyticsRoutes({
query: schema.object(querySchema),
},
},
async (context, request, response) => {
const { engineName, query } = request.params;
return enterpriseSearchRequestHandler.createRequest({
path: `/as/engines/${engineName}/analytics/query/${query}`,
})(context, request, response);
}
enterpriseSearchRequestHandler.createRequest({
path: '/as/engines/:engineName/analytics/query/:query',
})
);
}

21
x-pack/plugins/enterprise_search/server/routes/app_search/credentials.test.ts
View file

@ -200,16 +200,8 @@ describe('credentials routes', () => {
});
it('creates a request to enterprise search', () => {
const mockRequest = {
params: {
name: 'abc123',
},
};
mockRouter.callRoute(mockRequest);
expect(mockRequestHandler.createRequest).toHaveBeenCalledWith({
path: '/as/credentials/abc123',
path: '/as/credentials/:name',
});
});
@ -311,7 +303,6 @@ describe('credentials routes', () => {
mockRouter = new MockRouter({
method: 'delete',
path: '/api/app_search/credentials/{name}',
payload: 'params',
});
registerCredentialsRoutes({
@ -321,16 +312,8 @@ describe('credentials routes', () => {
});
it('creates a request to enterprise search', () => {
const mockRequest = {
params: {
name: 'abc123',
},
};
mockRouter.callRoute(mockRequest);
expect(mockRequestHandler.createRequest).toHaveBeenCalledWith({
path: '/as/credentials/abc123',
path: '/as/credentials/:name',
});
});
});

16
x-pack/plugins/enterprise_search/server/routes/app_search/credentials.ts
View file

@ -81,11 +81,9 @@ export function registerCredentialsRoutes({
body: tokenSchema,
},
},
async (context, request, response) => {
return enterpriseSearchRequestHandler.createRequest({
path: `/as/credentials/${request.params.name}`,
})(context, request, response);
}
enterpriseSearchRequestHandler.createRequest({
path: '/as/credentials/:name',
})
);
router.delete(
{
@ -96,10 +94,8 @@ export function registerCredentialsRoutes({
}),
},
},
async (context, request, response) => {
return enterpriseSearchRequestHandler.createRequest({
path: `/as/credentials/${request.params.name}`,
})(context, request, response);
}
enterpriseSearchRequestHandler.createRequest({
path: '/as/credentials/:name',
})
);
}

15
x-pack/plugins/enterprise_search/server/routes/app_search/documents.test.ts
View file

@ -27,13 +27,8 @@ describe('documents routes', () => {
});
it('creates a request to enterprise search', () => {
mockRouter.callRoute({
params: { engineName: 'some-engine' },
body: { documents: [{ foo: 'bar' }] },
});
expect(mockRequestHandler.createRequest).toHaveBeenCalledWith({
path: '/as/engines/some-engine/documents/new',
path: '/as/engines/:engineName/documents/new',
});
});
@ -79,10 +74,8 @@ describe('document routes', () => {
});
it('creates a request to enterprise search', () => {
mockRouter.callRoute({ params: { engineName: 'some-engine', documentId: '1' } });
expect(mockRequestHandler.createRequest).toHaveBeenCalledWith({
path: '/as/engines/some-engine/documents/1',
path: '/as/engines/:engineName/documents/:documentId',
});
});
});
@ -104,10 +97,8 @@ describe('document routes', () => {
});
it('creates a request to enterprise search', () => {
mockRouter.callRoute({ params: { engineName: 'some-engine', documentId: '1' } });
expect(mockRequestHandler.createRequest).toHaveBeenCalledWith({
path: '/as/engines/some-engine/documents/1',
path: '/as/engines/:engineName/documents/:documentId',
});
});
});

24
x-pack/plugins/enterprise_search/server/routes/app_search/documents.ts
View file

@ -24,11 +24,9 @@ export function registerDocumentsRoutes({
}),
},
},
async (context, request, response) => {
return enterpriseSearchRequestHandler.createRequest({
path: `/as/engines/${request.params.engineName}/documents/new`,
})(context, request, response);
}
enterpriseSearchRequestHandler.createRequest({
path: `/as/engines/:engineName/documents/new`,
})
);
}
@ -46,11 +44,9 @@ export function registerDocumentRoutes({
}),
},
},
async (context, request, response) => {
return enterpriseSearchRequestHandler.createRequest({
path: `/as/engines/${request.params.engineName}/documents/${request.params.documentId}`,
})(context, request, response);
}
enterpriseSearchRequestHandler.createRequest({
path: `/as/engines/:engineName/documents/:documentId`,
})
);
router.delete(
{
@ -62,10 +58,8 @@ export function registerDocumentRoutes({
}),
},
},
async (context, request, response) => {
return enterpriseSearchRequestHandler.createRequest({
path: `/as/engines/${request.params.engineName}/documents/${request.params.documentId}`,
})(context, request, response);
}
enterpriseSearchRequestHandler.createRequest({
path: `/as/engines/:engineName/documents/:documentId`,
})
);
}

10
x-pack/plugins/enterprise_search/server/routes/app_search/engines.test.ts
View file

@ -59,6 +59,7 @@ describe('engine routes', () => {
describe('hasValidData', () => {
it('should correctly validate that the response has data', () => {
mockRequestHandler.createRequest.mockClear();
const response = {
meta: {
page: {
@ -73,6 +74,7 @@ describe('engine routes', () => {
});
it('should correctly validate that a response does not have data', () => {
mockRequestHandler.createRequest.mockClear();
const response = {};
mockRouter.callRoute(mockRequest);
@ -125,10 +127,8 @@ describe('engine routes', () => {
});
it('creates a request to enterprise search', () => {
mockRouter.callRoute({ params: { name: 'some-engine' } });
expect(mockRequestHandler.createRequest).toHaveBeenCalledWith({
path: '/as/engines/some-engine/details',
path: '/as/engines/:name/details',
});
});
});
@ -150,10 +150,8 @@ describe('engine routes', () => {
});
it('creates a request to enterprise search', () => {
mockRouter.callRoute({ params: { name: 'some-engine' } });
expect(mockRequestHandler.createRequest).toHaveBeenCalledWith({
path: '/as/engines/some-engine/overview_metrics',
path: '/as/engines/:name/overview_metrics',
});
});
});

16
x-pack/plugins/enterprise_search/server/routes/app_search/engines.ts
View file

@ -54,11 +54,9 @@ export function registerEnginesRoutes({
}),
},
},
async (context, request, response) => {
return enterpriseSearchRequestHandler.createRequest({
path: `/as/engines/${request.params.name}/details`,
})(context, request, response);
}
enterpriseSearchRequestHandler.createRequest({
path: `/as/engines/:name/details`,
})
);
router.get(
{
@ -69,10 +67,8 @@ export function registerEnginesRoutes({
}),
},
},
async (context, request, response) => {
return enterpriseSearchRequestHandler.createRequest({
path: `/as/engines/${request.params.name}/overview_metrics`,
})(context, request, response);
}
enterpriseSearchRequestHandler.createRequest({
path: `/as/engines/:name/overview_metrics`,
})
);
}

3
x-pack/plugins/enterprise_search/server/routes/app_search/settings.test.ts
View file

@ -26,8 +26,6 @@ describe('log settings routes', () => {
});
it('creates a request to enterprise search', () => {
mockRouter.callRoute({});
expect(mockRequestHandler.createRequest).toHaveBeenCalledWith({
path: '/as/log_settings',
});
@ -52,7 +50,6 @@ describe('log settings routes', () => {
});
it('creates a request to enterprise search', () => {
mockRouter.callRoute({});
expect(mockRequestHandler.createRequest).toHaveBeenCalledWith({
path: '/as/log_settings',
});

8
x-pack/plugins/enterprise_search/server/routes/app_search/settings.ts
View file

@ -40,10 +40,8 @@ export function registerSettingsRoutes({
}),
},
},
async (context, request, response) => {
return enterpriseSearchRequestHandler.createRequest({
path: '/as/log_settings',
})(context, request, response);
}
enterpriseSearchRequestHandler.createRequest({
path: '/as/log_settings',
})
);
}