-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
[File upload] Migrate routing to NP & add route validation #52313
Merged
Merged
Changes from all commits
Commits
Show all changes
28 commits
Select commit
Hold shift + click to select a range
b6b8423
Cursory validation
3056a7b
Handle empty data arrays and settings conditionally
2b1de15
Set validation defaults. Move logic to routes folder and separate for…
24b7c90
Merge remote-tracking branch 'upstream/master' into file-upload-route…
ac97af5
Move plugin init back into routes folder. Syntax updates
f013212
Merge remote-tracking branch 'upstream/master' into file-upload-route…
b67d203
Migrate router to NP
9cff803
Use new np router and fetch. Add placeholder schema validation
45cbe96
Merge remote-tracking branch 'upstream/master' into file-upload-route…
024f274
Override default 'maxBytes'
f7d50a2
Merge remote-tracking branch 'upstream/master' into file-upload-route…
65f6c2a
Body with first-level schema keys in place
c8c334c
Add conditional validation of mappings, data and settings. Clean up o…
8fb5dd6
Merge remote-tracking branch 'upstream/master' into file-upload-route…
ff59043
Ensure query is handled correctly on both sides. Iron out decision lo…
399d13a
Merge remote-tracking branch 'upstream/master' into file-upload-route…
23af548
Move conditional validation to first step in payload handling
fd71efe
Merge remote-tracking branch 'upstream/master' into file-upload-route…
331ba9a
Merge remote-tracking branch 'upstream/master' into file-upload-route…
e10166a
Update http_service to work with latest NP changes on master
7c90c06
Some reorg. Only update telemetry if no errors
d1b5892
Clean up
995082c
Test file upload validation logic
638168d
Merge remote-tracking branch 'upstream/master' into file-upload-route…
79b7d79
Linting
8edf58a
Review feedback. Remove unneeded apiBasePath var
d840f7f
Pass entire req object with through to ES, not just the validated fields
fad1337
Merge remote-tracking branch 'upstream/master' into file-upload-route…
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,7 @@ | |
*/ | ||
|
||
import { http as httpService } from './http_service'; | ||
import { indexPatternService, apiBasePath, savedObjectsClient } from '../kibana_services'; | ||
import { indexPatternService, savedObjectsClient } from '../kibana_services'; | ||
import { getGeoJsonIndexingDetails } from './geo_processing'; | ||
import { sizeLimitedChunking } from './size_limited_chunking'; | ||
import { i18n } from '@kbn/i18n'; | ||
|
@@ -37,11 +37,9 @@ export async function indexData(parsedFile, transformDetails, indexName, dataTyp | |
data: [], | ||
index: indexName, | ||
}); | ||
let id; | ||
const id = createdIndex && createdIndex.id; | ||
try { | ||
if (createdIndex && createdIndex.id) { | ||
id = createdIndex.id; | ||
} else { | ||
if (!id) { | ||
throw i18n.translate('xpack.fileUpload.indexingService.errorCreatingIndex', { | ||
defaultMessage: 'Error creating index', | ||
}); | ||
|
@@ -117,12 +115,13 @@ function transformDataByFormatForIndexing(transform, parsedFile, dataType) { | |
} | ||
|
||
async function writeToIndex(indexingDetails) { | ||
const paramString = indexingDetails.id !== undefined ? `?id=${indexingDetails.id}` : ''; | ||
const query = indexingDetails.id ? { id: indexingDetails.id } : null; | ||
const { appName, index, data, settings, mappings, ingestPipeline } = indexingDetails; | ||
|
||
return await httpService({ | ||
url: `${apiBasePath}/fileupload/import${paramString}`, | ||
url: `/api/fileupload/import`, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using NP internal fetch, it's no longer necessary to prepend a basepath |
||
method: 'POST', | ||
...(query ? { query } : {}), | ||
data: { | ||
index, | ||
data, | ||
|
@@ -227,7 +226,7 @@ async function getIndexPatternId(name) { | |
|
||
export const getExistingIndexNames = async () => { | ||
const indexes = await httpService({ | ||
url: `${apiBasePath}/index_management/indices`, | ||
url: `/api/index_management/indices`, | ||
method: 'GET', | ||
}); | ||
return indexes ? indexes.map(({ name }) => name) : []; | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
76 changes: 76 additions & 0 deletions
76
x-pack/legacy/plugins/file_upload/server/routes/file_upload.test.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,76 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import { querySchema, bodySchema, idConditionalValidation } from './file_upload'; | ||
|
||
const queryWithId = { | ||
id: '123', | ||
}; | ||
|
||
const bodyWithoutQueryId = { | ||
index: 'islandofone', | ||
data: [], | ||
settings: { number_of_shards: 1 }, | ||
mappings: { coordinates: { type: 'geo_point' } }, | ||
ingestPipeline: {}, | ||
fileType: 'json', | ||
app: 'Maps', | ||
}; | ||
|
||
const bodyWithQueryId = { | ||
index: 'islandofone2', | ||
data: [{ coordinates: [], name: 'islandofone2' }], | ||
settings: {}, | ||
mappings: {}, | ||
ingestPipeline: {}, | ||
fileType: 'json', | ||
}; | ||
|
||
describe('route validation', () => { | ||
it(`validates query with id`, async () => { | ||
const validationResult = querySchema.validate(queryWithId); | ||
expect(validationResult.id).toBe(queryWithId.id); | ||
}); | ||
|
||
it(`validates query without id`, async () => { | ||
const validationResult = querySchema.validate({}); | ||
expect(validationResult.id).toBeNull(); | ||
}); | ||
|
||
it(`throws when query contains content other than an id`, async () => { | ||
expect(() => querySchema.validate({ notAnId: 123 })).toThrowError( | ||
`[notAnId]: definition for this key is missing` | ||
); | ||
}); | ||
|
||
it(`validates body with valid fields`, async () => { | ||
const validationResult = bodySchema.validate(bodyWithoutQueryId); | ||
expect(validationResult).toEqual(bodyWithoutQueryId); | ||
}); | ||
|
||
it(`throws if an expected field is missing`, async () => { | ||
/* eslint-disable no-unused-vars */ | ||
const { index, ...bodyWithoutIndexField } = bodyWithoutQueryId; | ||
expect(() => bodySchema.validate(bodyWithoutIndexField)).toThrowError( | ||
`[index]: expected value of type [string] but got [undefined]` | ||
); | ||
}); | ||
|
||
it(`validates conditional fields when id has been provided in query`, async () => { | ||
const validationResult = idConditionalValidation(bodyWithQueryId, true); | ||
expect(validationResult).toEqual(bodyWithQueryId); | ||
}); | ||
|
||
it(`validates conditional fields when no id has been provided in query`, async () => { | ||
const validationResultWhenIdPresent = idConditionalValidation(bodyWithoutQueryId, false); | ||
expect(validationResultWhenIdPresent).toEqual(bodyWithoutQueryId); | ||
// Conditions for no id are more strict since this query sets up the index, | ||
// expect it to throw if expected fields aren't present | ||
expect(() => idConditionalValidation(bodyWithoutQueryId, true)).toThrowError( | ||
`[data]: array size is [0], but cannot be smaller than [1]` | ||
); | ||
}); | ||
}); |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI you should be passing the
asSystemRequest
option to http.fetch if you still need this. That said, I'm not sure this request should be a system request. That flag is mostly used to bypass logic like session expiration or unauthorized responses that should redirect to the login page.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know. I don't think we'd want to treat it as a system request since eventually we'll want it to redirect to login if needed (i.e.- not bypass the logic that checks auth, etc.). So, sounds like the changes in place make sense?