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

Saved Object Namespaces #22357

Merged
merged 25 commits into from
Sep 4, 2018
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
a8c38b7
Adding a namespace
kobelb Aug 10, 2018
a3c92ad
Allowing the saved objects client wrappers to specify the namespace
kobelb Aug 23, 2018
5e37485
Moving namespace agnosticism to OSS
kobelb Aug 24, 2018
c90476b
Fixing rbac tests, spaces can be managed with the SOC temporarily
kobelb Aug 24, 2018
6ec276f
Putting trimIdPrefix back to it's original name
kobelb Aug 24, 2018
7cfc737
Removing unused code and debug statements
kobelb Aug 24, 2018
27c60c0
Fixing some jsdocs
kobelb Aug 24, 2018
5b937bb
Removing unused type parameter
kobelb Aug 24, 2018
709dffd
Another stray console.log...
kobelb Aug 24, 2018
e06a0ee
Merge remote-tracking branch 'upstream/spaces-phase-1' into ns
kobelb Aug 24, 2018
ff08700
Fixing repository provider test
kobelb Aug 24, 2018
57487af
Fixing repository tests
kobelb Aug 24, 2018
f877510
No longer exposing the namespace in get and bulkGet
kobelb Aug 24, 2018
c9059e7
Fixing SavedObjectClient tests, using more Symbols...
kobelb Aug 24, 2018
2c4f135
Fixing getSearchDsl tests
kobelb Aug 24, 2018
636d583
Removing filters, we don't use them anymore
kobelb Aug 24, 2018
c05b369
Fixing query param tests
kobelb Aug 27, 2018
ad190b9
Adding Schema tests
kobelb Aug 27, 2018
3bb65dd
Fixing secure saved objects client test
kobelb Aug 27, 2018
a2c19ec
Namespaces via options
kobelb Aug 27, 2018
c424230
Removing duplicate test
kobelb Aug 28, 2018
607b807
Removing spaceId from mappings
kobelb Aug 28, 2018
002e3b0
Fixing test
kobelb Aug 28, 2018
d213cd8
Registering the namespace agnostic types using uiExports
kobelb Sep 4, 2018
95ef4b0
Even better schema
kobelb Sep 4, 2018
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 src/server/mappings/kibana_index_mappings_mixin.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ const BASE_SAVED_OBJECT_MAPPINGS = {
doc: {
dynamic: 'strict',
properties: {
namespace: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that the base mappings have a namespace property, we can probably remove the spaceId property from the Spaces plugin mappings.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...and all associated esArchiver mappings 😉

type: 'keyword'
},
type: {
type: 'keyword'
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@
* under the License.
*/

import { getRootPropertiesObjects } from '../..//mappings';
import { SavedObjectsRepository, ScopedSavedObjectsClientProvider, SavedObjectsRepositoryProvider } from './lib';
import { getRootPropertiesObjects } from '../../mappings';
import { SavedObjectsRepository, ScopedSavedObjectsClientProvider, SavedObjectsRepositoryProvider, SavedObjectsSchema } from './lib';
import { SavedObjectsClient } from './saved_objects_client';

export function createSavedObjectsService(server) {
Expand Down Expand Up @@ -59,10 +59,13 @@ export function createSavedObjectsService(server) {
}
};

const schema = new SavedObjectsSchema();

const mappings = server.getKibanaIndexMappingsDsl();
const repositoryProvider = new SavedObjectsRepositoryProvider({
index: server.config().get('kibana.index'),
mappings,
schema,
onBeforeWrite,
});

Expand All @@ -86,6 +89,7 @@ export function createSavedObjectsService(server) {
types: Object.keys(getRootPropertiesObjects(mappings)),
SavedObjectsClient,
SavedObjectsRepository,
schema,
getSavedObjectsRepository: (...args) =>
repositoryProvider.getRepository(...args),
getScopedSavedObjectsClient: (...args) =>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`#addNamespaceAgnosticType can't add Symbol type 1`] = `"type must be a string"`;

exports[`#addNamespaceAgnosticType can't add a bool type 1`] = `"type must be a string"`;

exports[`#addNamespaceAgnosticType can't add function type 1`] = `"type must be a string"`;

exports[`#addNamespaceAgnosticType can't add null type 1`] = `"type must be a string"`;

exports[`#addNamespaceAgnosticType can't add number type 1`] = `"type must be a string"`;
1 change: 1 addition & 0 deletions src/server/saved_objects/service/lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
export { SavedObjectsRepository } from './repository';
export { ScopedSavedObjectsClientProvider } from './scoped_client_provider';
export { SavedObjectsRepositoryProvider } from './repository_provider';
export { SavedObjectsSchema } from './schema';

import * as errors from './errors';
export { errors };
96 changes: 53 additions & 43 deletions src/server/saved_objects/service/lib/repository.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,14 @@ export class SavedObjectsRepository {
const {
index,
mappings,
schema,
callCluster,
onBeforeWrite = () => { },
} = options;

this._index = index;
this._mappings = mappings;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would make sense for mappings and schema to merge at some point.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I completely agree.

this._schema = schema;
this._type = getRootType(this._mappings);
this._onBeforeWrite = onBeforeWrite;
this._unwrappedCallCluster = callCluster;
Expand All @@ -54,35 +56,35 @@ export class SavedObjectsRepository {
* @param {object} [options={}]
* @property {string} [options.id] - force id on creation, not recommended
* @property {boolean} [options.overwrite=false]
* @property {object} [options.extraDocumentProperties={}] - extra properties to append to the document body, outside of the object's type property
* @property {string} [options.namespace]
* @returns {promise} - { id, type, version, attributes }
*/
async create(type, attributes = {}, options = {}) {
const {
id,
extraDocumentProperties = {},
overwrite = false
overwrite = false,
namespace,
} = options;

const method = id && !overwrite ? 'create' : 'index';
const time = this._getCurrentTime();

try {
const response = await this._writeToCluster(method, {
id: this._generateEsId(type, id),
id: this._generateEsId(namespace, type, id),
type: this._type,
index: this._index,
refresh: 'wait_for',
body: {
...extraDocumentProperties,
...namespace && !this._schema.isNamespaceAgnostic(type) && { namespace },
type,
updated_at: time,
[type]: attributes,
},
});

return {
id: trimIdPrefix(response._id, type),
id: trimIdPrefix(this._schema, response._id, namespace, type),
type,
updated_at: time,
version: response._version,
Expand All @@ -101,14 +103,16 @@ export class SavedObjectsRepository {
/**
* Creates multiple documents at once
*
* @param {array} objects - [{ type, id, attributes, extraDocumentProperties }]
* @param {array} objects - [{ type, id, attributes }]
* @param {object} [options={}]
* @property {boolean} [options.overwrite=false] - overwrites existing documents
* @property {string} [options.namespace]
* @returns {promise} - {saved_objects: [[{ id, type, version, attributes, error: { message } }]}
*/
async bulkCreate(objects, options = {}) {
const {
overwrite = false
overwrite = false,
namespace
} = options;
const time = this._getCurrentTime();
const objectToBulkRequest = (object) => {
Expand All @@ -117,12 +121,12 @@ export class SavedObjectsRepository {
return [
{
[method]: {
_id: this._generateEsId(object.type, object.id),
_id: this._generateEsId(namespace, object.type, object.id),
_type: this._type,
}
},
{
...object.extraDocumentProperties,
... namespace && !this._schema.isNamespaceAgnostic(object.type) && { namespace },
type: object.type,
updated_at: time,
[object.type]: object.attributes,
Expand Down Expand Up @@ -186,11 +190,17 @@ export class SavedObjectsRepository {
*
* @param {string} type
* @param {string} id
* @param {object} [options={}]
* @property {string} [options.namespace]
* @returns {promise}
*/
async delete(type, id) {
async delete(type, id, options = {}) {
const {
namespace
} = options;

const response = await this._writeToCluster('delete', {
id: this._generateEsId(type, id),
id: this._generateEsId(namespace, type, id),
type: this._type,
index: this._index,
refresh: 'wait_for',
Expand Down Expand Up @@ -220,12 +230,12 @@ export class SavedObjectsRepository {
* @property {string} [options.search]
* @property {Array<string>} [options.searchFields] - see Elasticsearch Simple Query String
* Query field argument for more information
* @property {object} [options.filters] - ES Query filters to append
* @property {integer} [options.page=1]
* @property {integer} [options.perPage=20]
* @property {string} [options.sortField]
* @property {string} [options.sortOrder]
* @property {Array<string>} [options.fields]
* @property {string} [options.namespace]
* @returns {promise} - { saved_objects: [{ id, type, version, attributes }], total, per_page, page }
*/
async find(options = {}) {
Expand All @@ -238,7 +248,7 @@ export class SavedObjectsRepository {
sortField,
sortOrder,
fields,
filters,
namespace,
} = options;

if (searchFields && !Array.isArray(searchFields)) {
Expand All @@ -249,10 +259,6 @@ export class SavedObjectsRepository {
throw new TypeError('options.searchFields must be an array');
}

if (filters && !Array.isArray(filters)) {
throw new TypeError('options.filters must be an array');
}

const esOptions = {
index: this._index,
size: perPage,
Expand All @@ -261,13 +267,13 @@ export class SavedObjectsRepository {
ignore: [404],
body: {
version: true,
...getSearchDsl(this._mappings, {
...getSearchDsl(this._mappings, this._schema, {
namespace,
search,
searchFields,
type,
sortField,
sortOrder,
filters
})
}
};
Expand All @@ -292,7 +298,7 @@ export class SavedObjectsRepository {
saved_objects: response.hits.hits.map(hit => {
const { type, updated_at: updatedAt } = hit._source;
return {
id: trimIdPrefix(hit._id, type),
id: trimIdPrefix(this._schema, hit._id, namespace, type),
type,
...updatedAt && { updated_at: updatedAt },
version: hit._version,
Expand All @@ -306,8 +312,8 @@ export class SavedObjectsRepository {
* Returns an array of objects by id
*
* @param {array} objects - an array ids, or an array of objects containing id and optionally type
* @param {object} [options = {}]
* @param {array} [options.extraDocumentProperties = []] - an array of extra properties to return from the underlying document
* @param {object} [options={}]
* @property {string} [options.namespace]
* @returns {promise} - { saved_objects: [{ id, type, version, attributes }] }
* @example
*
Expand All @@ -317,6 +323,10 @@ export class SavedObjectsRepository {
* ])
*/
async bulkGet(objects = [], options = {}) {
const {
namespace
} = options;

if (objects.length === 0) {
return { saved_objects: [] };
}
Expand All @@ -325,16 +335,14 @@ export class SavedObjectsRepository {
index: this._index,
body: {
docs: objects.map(object => ({
_id: this._generateEsId(object.type, object.id),
_id: this._generateEsId(namespace, object.type, object.id),
_type: this._type,
}))
}
});

const { docs } = response;

const { extraDocumentProperties = [] } = options;

return {
saved_objects: docs.map((doc, i) => {
const { id, type } = objects[i];
Expand All @@ -353,9 +361,6 @@ export class SavedObjectsRepository {
type,
...time && { updated_at: time },
version: doc._version,
...extraDocumentProperties
Copy link
Member

@legrego legrego Aug 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously, extraDocumentProperties was used to retrieve the spaceId (or now, namespace) property in order to verify that the object belongs to the current space.

Now, we are still restricting to the current space, but we aren't using this property to do so. We are using the composite document id to perform this check instead. This is more efficient than what I had, but it deviates from the property being the authoritative source of this information. In other words, we aren't using the namespace property for get or bulk_get operations anymore. By extension, this property isn't being used to verify update or delete operations, either.

I'm not necessarily opposed to changing our direction, but if I understand this right, the namespace property is only used for find at this point, which feels inconsistent.

On the other hand, adding the code back in to check the namespace property would end up introducing code paths that wouldn't get hit...

What are your thoughts?

Copy link
Contributor Author

@kobelb kobelb Aug 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I shared your hesitation when initially making these changes, and it does remove the namespace property itself being the authoritative source of this information. However, it's equivalent to how we're using the type within the Repository itself. When in the context of delete \ get \ update we use the type and the namespace prepended to the IDs as the source of this information; however, the find operation relies upon the type and namespace attributes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense -- thanks for drawing the parallel to how we use the type property. That convinced me!

.map(s => ({ [s]: doc._source[s] }))
.reduce((acc, prop) => ({ ...acc, ...prop }), {}),
attributes: {
...doc._source[type],
}
Expand All @@ -371,13 +376,17 @@ export class SavedObjectsRepository {
*
* @param {string} type
* @param {string} id
* @param {object} [options = {}]
* @param {array} [options.extraDocumentProperties = []] - an array of extra properties to return from the underlying document
* @param {object} [options={}]
* @property {string} [options.namespace]
* @returns {promise} - { id, type, version, attributes }
*/
async get(type, id, options = {}) {
const {
namespace
} = options;

const response = await this._callCluster('get', {
id: this._generateEsId(type, id),
id: this._generateEsId(namespace, type, id),
type: this._type,
index: this._index,
ignore: [404]
Expand All @@ -390,18 +399,13 @@ export class SavedObjectsRepository {
throw errors.createGenericNotFoundError(type, id);
}

const { extraDocumentProperties = [] } = options;

const { updated_at: updatedAt } = response._source;

return {
id,
type,
...updatedAt && { updated_at: updatedAt },
version: response._version,
...extraDocumentProperties
.map(s => ({ [s]: response._source[s] }))
.reduce((acc, prop) => ({ ...acc, ...prop }), {}),
attributes: {
...response._source[type],
}
Expand All @@ -415,21 +419,26 @@ export class SavedObjectsRepository {
* @param {string} id
* @param {object} [options={}]
* @property {integer} options.version - ensures version matches that of persisted object
* @param {array} [options.extraDocumentProperties = {}] - an object of extra properties to write into the underlying document
* @property {string} [options.namespace]
* @returns {promise}
*/
async update(type, id, attributes, options = {}) {
const {
version,
namespace
} = options;

const time = this._getCurrentTime();
const response = await this._writeToCluster('update', {
id: this._generateEsId(type, id),
id: this._generateEsId(namespace, type, id),
type: this._type,
index: this._index,
version: options.version,
version,
refresh: 'wait_for',
ignore: [404],
body: {
doc: {
...options.extraDocumentProperties,
...namespace && !this._schema.isNamespaceAgnostic(type) && { namespace },
updated_at: time,
[type]: attributes,
}
Expand Down Expand Up @@ -467,8 +476,9 @@ export class SavedObjectsRepository {
}
}

_generateEsId(type, id) {
return `${type}:${id || uuid.v1()}`;
_generateEsId(namespace, type, id) {
const namespacePrefix = namespace && !this._schema.isNamespaceAgnostic(type) ? `${namespace}:` : '';
return `${namespacePrefix}${type}:${id || uuid.v1()}`;
}

_getCurrentTime() {
Expand Down
Loading