Skip to content

Commit

Permalink
Fix bug caused by subtle difference between this.type and type
Browse files Browse the repository at this point in the history
Add a comment and rename one of the variables to make the distinction
clearer.
  • Loading branch information
stacey-gammon committed Feb 23, 2017
1 parent 8881739 commit f4011ce
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 17 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ import _ from 'lodash';
* sensitive, it may not exactly match the title of the object.
*/
export function getTitleAlreadyExists(savedObject, esAdmin) {
const { index, title, type, id } = savedObject;
const { index, title, id } = savedObject;
const esType = savedObject.getEsType();
if (!title) {
throw new Error('Title must be supplied');
}
Expand All @@ -25,7 +26,7 @@ export function getTitleAlreadyExists(savedObject, esAdmin) {
// Elastic search will return the most relevant results first, which means exact matches should come
// first, and so we shouldn't need to request everything. Using 10 just to be on the safe side.
const size = 10;
return esAdmin.search({ index, type, body, size })
return esAdmin.search({ index, type: esType, body, size })
.then((response) => {
const match = _.find(response.hits.hits, function currentVersion(hit) {
return hit._source.title.toLowerCase() === title.toLowerCase();
Expand Down
35 changes: 20 additions & 15 deletions src/ui/public/courier/saved_object/saved_object.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,17 @@ export default function SavedObjectFactory(esAdmin, kbnIndex, Promise, Private,
const docSource = new DocSource();

// type name for this object, used as the ES-type
const type = config.type;
this.type = type;
const esType = config.type;
this.index = kbnIndex;

this.getDisplayName = function () {
return type;
return esType;
};

// NOTE: this.type (not set in this file, but somewhere else) is the sub type, e.g. 'area' or
// 'data table', while esType is the more generic type - e.g. 'visualization' or 'saved search'.
this.getEsType = function () {
return esType;
};

/**
Expand All @@ -54,7 +59,7 @@ export default function SavedObjectFactory(esAdmin, kbnIndex, Promise, Private,

// Create a notifier for sending alerts
const notify = new Notifier({
location: 'Saved ' + type
location: 'Saved ' + this.getDisplayName()
});

// mapping definition for the fields that this object will expose
Expand Down Expand Up @@ -135,17 +140,17 @@ export default function SavedObjectFactory(esAdmin, kbnIndex, Promise, Private,
* @resolved {SavedObject}
*/
this.init = _.once(() => {
// ensure that the type is defined
if (!type) throw new Error('You must define a type name to use SavedObject objects.');
// ensure that the esType is defined
if (!esType) throw new Error('You must define a type name to use SavedObject objects.');

// tell the docSource where to find the doc
docSource
.index(kbnIndex)
.type(type)
.type(esType)
.id(this.id);

// check that the mapping for this type is defined
return mappingSetup.isDefined(type)
// check that the mapping for this esType is defined
return mappingSetup.isDefined(esType)
.then(function (defined) {
// if it is already defined skip this step
if (defined) return true;
Expand All @@ -159,8 +164,8 @@ export default function SavedObjectFactory(esAdmin, kbnIndex, Promise, Private,
}
};

// tell mappingSetup to set type
return mappingSetup.setup(type, mapping);
// tell mappingSetup to set esType
return mappingSetup.setup(esType, mapping);
})
.then(() => {
// If there is not id, then there is no document to fetch from elasticsearch
Expand All @@ -187,7 +192,7 @@ export default function SavedObjectFactory(esAdmin, kbnIndex, Promise, Private,
this.applyESResp = (resp) => {
this._source = _.cloneDeep(resp._source);

if (resp.found != null && !resp.found) throw new errors.SavedObjectNotFound(type, this.id);
if (resp.found != null && !resp.found) throw new errors.SavedObjectNotFound(esType, this.id);

const meta = resp._source.kibanaSavedObjectMeta || {};
delete resp._source.kibanaSavedObjectMeta;
Expand Down Expand Up @@ -317,9 +322,9 @@ export default function SavedObjectFactory(esAdmin, kbnIndex, Promise, Private,
.then((duplicateTitle) => {
if (!duplicateTitle) return true;
const confirmMessage =
`A ${type} with the title '${duplicateTitle}' already exists. Would you like to save anyway?`;
`A ${this.getDisplayName()} with the title '${duplicateTitle}' already exists. Would you like to save anyway?`;

return confirmModalPromise(confirmMessage, { confirmButtonText: `Save ${type}` })
return confirmModalPromise(confirmMessage, { confirmButtonText: `Save ${this.getDisplayName()}` })
.catch(() => Promise.reject(new Error(SAVE_DUPLICATE_REJECTED)));
});
};
Expand Down Expand Up @@ -399,7 +404,7 @@ export default function SavedObjectFactory(esAdmin, kbnIndex, Promise, Private,
return esAdmin.delete(
{
index: kbnIndex,
type: type,
type: esType,
id: this.id
})
.then(() => {
Expand Down

0 comments on commit f4011ce

Please sign in to comment.