-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Spaces] - prepend space id to document id #21372
Changes from 56 commits
f208d3c
7538bf3
bb3e511
0e2e4e8
a6287cc
7db0a4a
192d9c2
1c4afd8
d573457
d99cec7
7e2d1e3
862752b
a63128c
858eff0
1b95aa0
fc663d9
fc61594
776da8a
d2545d4
1fd7699
9469742
e24578f
5fe4bfd
c6e8925
f4a19ab
7827e02
dee335b
452de10
6bf3515
8cb871a
53bb020
90892ca
4181c9e
a35d15f
093dd47
2dd7464
3bb2aa7
6e1c4c4
2195ee0
5fced69
48c5f23
3a832e9
71f0634
00bd94c
415fa09
8d48c80
7ee1cb0
bc8aaef
839a0cf
0ebe3e3
8b1ff6b
e7815e6
70284a5
7e851d0
7042889
560acfa
a5161bf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,13 +1,13 @@ | ||
// Jest Snapshot v1, https://goo.gl/fbAQLP | ||
|
||
exports[`current space (space_1) #create #delete does not allow an object to be deleted via a different space 1`] = `"not found"`; | ||
exports[`current space (space_1) #delete does not allow an object to be deleted via a different space 1`] = `"not found: foo space_1:object_2"`; | ||
|
||
exports[`current space (space_1) #create #update does not allow an object to be updated via a different space 1`] = `"not found"`; | ||
exports[`current space (space_1) #get returns error when the object belongs to a different space 1`] = `"not found: foo space_1:object_2"`; | ||
|
||
exports[`current space (space_1) #get returns error when the object belongs to a different space 1`] = `"not found"`; | ||
exports[`current space (space_1) #update does not allow an object to be updated via a different space 1`] = `"not found: foo space_1:object_2"`; | ||
|
||
exports[`default space #delete does not allow an object to be deleted via a different space 1`] = `"not found"`; | ||
exports[`default space #delete does not allow an object to be deleted via a different space 1`] = `"not found: foo object_2"`; | ||
|
||
exports[`default space #get returns error when the object belongs to a different space 1`] = `"not found"`; | ||
exports[`default space #get returns error when the object belongs to a different space 1`] = `"not found: foo object_2"`; | ||
|
||
exports[`default space #update does not allow an object to be updated via a different space 1`] = `"not found"`; | ||
exports[`default space #update does not allow an object to be updated via a different space 1`] = `"not found: foo object_2"`; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,7 @@ import { DEFAULT_SPACE_ID } from '../../../common/constants'; | |
import { isTypeSpaceAware } from './lib/is_type_space_aware'; | ||
import { getSpacesQueryFilters } from './lib/query_filters'; | ||
import uniq from 'lodash'; | ||
import uuid from 'uuid'; | ||
|
||
export class SpacesSavedObjectsClient { | ||
constructor(options) { | ||
|
@@ -45,7 +46,8 @@ export class SpacesSavedObjectsClient { | |
...options, | ||
extraDocumentProperties: { | ||
...options.extraDocumentProperties | ||
} | ||
}, | ||
id: this._generateDocumentId(type, options.id) | ||
}; | ||
|
||
if (this._shouldAssignSpaceId(type, spaceId)) { | ||
|
@@ -54,7 +56,8 @@ export class SpacesSavedObjectsClient { | |
delete createOptions.extraDocumentProperties.spaceId; | ||
} | ||
|
||
return await this._client.create(type, attributes, createOptions); | ||
const result = await this._client.create(type, attributes, createOptions); | ||
return this._trimSpaceId(result); | ||
} | ||
|
||
/** | ||
|
@@ -73,7 +76,8 @@ export class SpacesSavedObjectsClient { | |
...object, | ||
extraDocumentProperties: { | ||
...object.extraDocumentProperties | ||
} | ||
}, | ||
id: this._generateDocumentId(object.type, object.id) | ||
}; | ||
|
||
if (this._shouldAssignSpaceId(object.type, spaceId)) { | ||
|
@@ -85,7 +89,10 @@ export class SpacesSavedObjectsClient { | |
return objectToCreate; | ||
}); | ||
|
||
return await this._client.bulkCreate(objectsToCreate, options); | ||
const result = await this._client.bulkCreate(objectsToCreate, options); | ||
result.saved_objects.forEach(this._trimSpaceId.bind(this)); | ||
|
||
return result; | ||
} | ||
|
||
/** | ||
|
@@ -96,11 +103,13 @@ export class SpacesSavedObjectsClient { | |
* @returns {promise} | ||
*/ | ||
async delete(type, id) { | ||
const documentId = this._generateDocumentId(type, id); | ||
|
||
// attempt to retrieve document before deleting. | ||
// this ensures that the document belongs to the current space. | ||
await this.get(type, id); | ||
|
||
return await this._client.delete(type, id); | ||
return await this._client.delete(type, documentId); | ||
} | ||
|
||
/** | ||
|
@@ -131,7 +140,9 @@ export class SpacesSavedObjectsClient { | |
|
||
spaceOptions.filters = [...filters, ...getSpacesQueryFilters(spaceId, types)]; | ||
|
||
return await this._client.find({ ...options, ...spaceOptions }); | ||
const result = await this._client.find({ ...options, ...spaceOptions }); | ||
result.saved_objects.forEach(this._trimSpaceId.bind(this)); | ||
return result; | ||
} | ||
|
||
/** | ||
|
@@ -154,13 +165,18 @@ export class SpacesSavedObjectsClient { | |
|
||
const extraDocumentProperties = this._collectExtraDocumentProperties(['spaceId', 'type'], options.extraDocumentProperties); | ||
|
||
const result = await this._client.bulkGet(objects, { | ||
const objectsToRetrieve = objects.map(object => ({ | ||
...object, | ||
id: this._generateDocumentId(object.type, object.id) | ||
})); | ||
|
||
const result = await this._client.bulkGet(objectsToRetrieve, { | ||
...options, | ||
extraDocumentProperties | ||
}); | ||
|
||
result.saved_objects = result.saved_objects.map(savedObject => { | ||
const { id, type, spaceId = DEFAULT_SPACE_ID } = savedObject; | ||
const { id, type, spaceId = DEFAULT_SPACE_ID } = this._trimSpaceId(savedObject); | ||
|
||
if (isTypeSpaceAware(type)) { | ||
if (spaceId !== thisSpaceId) { | ||
|
@@ -190,9 +206,11 @@ export class SpacesSavedObjectsClient { | |
async get(type, id, options = {}) { | ||
// ES 'get' does not support queries, so we have to filter results after the fact. | ||
|
||
const documentId = this._generateDocumentId(type, id); | ||
|
||
const extraDocumentProperties = this._collectExtraDocumentProperties(['spaceId'], options.extraDocumentProperties); | ||
|
||
const response = await this._client.get(type, id, { | ||
const response = await this._client.get(type, documentId, { | ||
...options, | ||
extraDocumentProperties | ||
}); | ||
|
@@ -206,7 +224,7 @@ export class SpacesSavedObjectsClient { | |
} | ||
} | ||
|
||
return response; | ||
return this._trimSpaceId(response); | ||
} | ||
|
||
/** | ||
|
@@ -227,6 +245,8 @@ export class SpacesSavedObjectsClient { | |
} | ||
}; | ||
|
||
const documentId = this._generateDocumentId(type, id); | ||
|
||
// attempt to retrieve document before updating. | ||
// this ensures that the document belongs to the current space. | ||
if (isTypeSpaceAware(type)) { | ||
|
@@ -241,7 +261,8 @@ export class SpacesSavedObjectsClient { | |
} | ||
} | ||
|
||
return await this._client.update(type, id, attributes, updateOptions); | ||
const result = await this._client.update(type, documentId, attributes, updateOptions); | ||
return this._trimSpaceId(result); | ||
} | ||
|
||
_collectExtraDocumentProperties(thisClientProperties, optionalProperties = []) { | ||
|
@@ -251,4 +272,21 @@ export class SpacesSavedObjectsClient { | |
_shouldAssignSpaceId(type, spaceId) { | ||
return spaceId !== DEFAULT_SPACE_ID && isTypeSpaceAware(type); | ||
} | ||
|
||
_generateDocumentId(type, id = uuid.v1()) { | ||
if (!this._spaceId || this._spaceId === DEFAULT_SPACE_ID || !isTypeSpaceAware(type)) { | ||
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. When wouldn't we have a 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. we should always have one -- this is an old habit from when the SOC had a URL Context that may or may not exist. I'll clean this up! |
||
return id; | ||
} | ||
return `${this._spaceId}:${id}`; | ||
} | ||
|
||
_trimSpaceId(savedObject) { | ||
const prefix = `${this._spaceId}:`; | ||
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. The way that we're trimming the spaceId is very similar to here, it seems like a rather naive implementation that could have unwanted behaviors if we're missing a spaceId prefix and we should have one for the type. I wonder why they weren't throwing errors in the base |
||
|
||
if (savedObject.id.startsWith(prefix)) { | ||
savedObject.id = savedObject.id.slice(prefix.length); | ||
} | ||
|
||
return savedObject; | ||
} | ||
} |
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.
nit: I'm not sure how I feel about calling it a "documentId" as it's not the actual ID that is used for the document in Elasticsearch. Perhaps just
this._prependSpaceId
, but I'm not too fond of this either...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.
hmm, that's a good point. I don't hate
this._prependSpaceId
. Do you not like this because it's not always prepending the space id?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.
Yeah... it's all I got, and I'm not completely opposed to it.