Saved objects compatibility layer#12648
Conversation
881fc05 to
cebbf43
Compare
We fallback to v6, as we will be keeping v5 mappings in 5.6. Removed addition of API integration tests till we can fix the tests themselves Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
cebbf43 to
2782201
Compare
| async create(type, attributes = {}, options = {}) { | ||
| const method = options.id && !options.overwrite ? 'create' : 'index'; | ||
| const response = await this._withKibanaIndex(method, { | ||
| console.log('create> ', method, type, attributes, options); |
| .then(resolve) | ||
| .catch(err => { | ||
| console.log('err', err); | ||
| if (get(fallbacks, method, []).includes(get(err, 'data.type'))) { |
| return { | ||
| id: response._id, | ||
| type: response._type, | ||
| type: type, |
There was a problem hiding this comment.
Nitpick: this can use the short property syntax
| }); | ||
| const docs = objects.reduce((acc, { type, id }) => { | ||
| acc.push({}, createIdQuery({ type, id })); | ||
| return acc; |
There was a problem hiding this comment.
Rather than mutating acc, how about using spread:
return [...acc, {}, createIdQuery({ type, id })];That suggestion out of the way, why push an empty object here (ie the first arg)?
There was a problem hiding this comment.
It's the msearch header. We can leave it empty as we're specifying the index on the request instead of per query.
| .then(resp => { | ||
| let results = []; | ||
| const responses = get(resp, 'responses', []); | ||
| responses.forEach(r => { |
There was a problem hiding this comment.
This can be done with a single map rather than closing over and mutating a results accumulator inside a forEach.
| */ | ||
| async get(type, id) { | ||
| const response = await this._withKibanaIndex('search', { body: createIdQuery({ type, id }) }); | ||
| const [hit,] = get(response, 'hits.hits', []); |
There was a problem hiding this comment.
Is this trailing comma to appease the linter?
There was a problem hiding this comment.
Thought it was needed to ignore the additional assignment, but appears it's not necessary. Will remove.
|
|
||
| return { | ||
| id: hit._id, | ||
| type: type, |
| }; | ||
|
|
||
| return new Promise((resolve, reject) => { | ||
| this._withKibanaIndex(method, params) |
There was a problem hiding this comment.
There's no need to use the Promise constructor here. Since _withKibanaIndex returns a promise itself, you can just chain off of that and return the result.
Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
|
Here was my manual testing procedure - you will need to update id's before copy/pasting if you use: Starting with .kibana indexCreate a single object
response: Update the object
response: Get the object
response: Import dashboard
response: Export dashboard
response: Create templateuse template creation script in issue description Reindexuse reindex script in issue description Use .kibana-v6Config should now reflect Create a single object
response: Update the object
response: Get the object
response: Get an object from v5
response: Bulk get both objects
response: Delete the object
|
Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
| return objects.reduce((acc, object) => { | ||
| const method = object.id && !options.overwrite ? 'create' : 'index'; | ||
|
|
||
| acc.push({ [method]: { _type: 'doc', _id: object.id } }); |
There was a problem hiding this comment.
Can we move the V6_TYPE constant from saved_objects_client.js and use it here instead of hard-coding 'doc'?
Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
There was a problem hiding this comment.
I went through the test-scenario, and no immediate red flags. Seems to work well!
Below a couple of comments, unsure if they are all related to this PR.
After reindexing, object are assigned a new ID property. Is that a property of the re-indexing of ES? This seems to create some inconsistencies.
For example, wrt. dashboard. It was create with the "id": "test", and after re-indexing it gets assigned a new id, e.g.
{"id":"AV0UtNI1urO7MVHxeLol","type":"dashboard","version":1,"attributes":{"title":"My Test Dashboard"}}.
The REST-API then can return that same document using either of the IDs.
curl http://localhost:5601/api/saved_objects/dashboard/testcurl http://localhost:5601/api/saved_objects/dashboard/AV0UtNI1urO7MVHxeLol
will return the same document. Is there a reason we cannot reuse the same ID from v5?
A couple times, the bulk_get failed for me. Not sure how to reproduce consistently though
> ~/repos/kibana$ curl -X POST -H "Contenton" -H "kbn-xsrf: 6.0.0-alpha3" -d '[{"type": "config", "id": "AV0UqXp3urO7MVHxeLob"}, {"type": "config", "id": "AV0UuO54urO7MVHxeLon"}]' http://localhost:5601/api/saved_objects/bulk_get
curl: (7) Failed to connect to localhost port 5601: Connection refused
> ~/repos/kibana$ curl -X POST -H "Content-Type: application/json" -H "kbn-xsrf: 6.0.0-alpha3" -d '[{"type": "config", "id": "AV0UqXp3urO7MVHxeLob"}, {"type": "config", "id": "AV0UuO54urO7MVHxeLon"}]' http://localhost:5601/api/saved_objects/bulk_get
{"saved_objects":[{"id":"AV0UtNI1urO7MVHxeLok","type":"config","version":1,"attributes":{"title":"Test 1, updated - from v5"}},{"id":"AV0UuO54urO7MVHxeLon","type":"config","version":2,"attributes":{"title":"Test 2, updated - from v6"}}]}
Any reason why the export-method API turns on content-disposition, but the REST-API does not? May help if responses are aligned. (is easier to debug if removed as well).
HTTP/1.1 200 OK
kbn-name: kibana
kbn-version: 6.0.0-alpha3
vary: origin
content-type: application/json; charset=utf-8
cache-control: no-cache
content-length: 103
accept-ranges: bytes
Date: Wed, 05 Jul 2017 22:21:37 GMT
Connection: keep-alive
{"id":"AV0UtNI1urO7MVHxeLol","type":"dashboard","version":1,"attributes":{"title":"My Test Dashboard"}}```
vs
```~/repos/kibana$ ~url http://localhost:5601/api/kibana/dashboards/export?dashboard=test -i
HTTP/1.1 200 OK
content-disposition: attachment; filename="kibana-dashboards.2017-07-05-22-22-21.json"
content-type: application/json; charset=utf-8
content-length: 213
kbn-name: kibana
kbn-version: 6.0.0-alpha3
vary: origin
cache-control: no-cache
accept-ranges: bytes
Date: Wed, 05 Jul 2017 22:22:21 GMT
Connection: keep-alive
{
"version": "6.0.0-alpha3",
"objects": [
{
"id": "AV0UtNI1urO7MVHxeLol",
"type": "dashboard",
"version": 1,
"attributes": {
"title": "My Test Dashboard"
}
}
]
}```
| query: { | ||
| bool: { | ||
| should: [ | ||
| // v5 document |
There was a problem hiding this comment.
I guess this explains my earlier id-comment.
Isn't there a possible conflict here?
E.g., suppose I create a new dashboard after re-indexing with the same human readable id I had from an older v5 dashboard.
curl -X POST -H "Content-Type: application/json" -H "kbn-xsrf: 6.0.0-alpha3" -d '{ "version": "6.0.0-alpha3", "objects": [{ "id": "test", "type": "dashboard", "version": 1, "attributes": { "title": "My Test Dashboard From V6" }}]}' http://localhost:5601/api/kibana/dashboards/import
{"objects":[{"id":"test","type":"doc","version":1,"attributes":{"title":"My Test Dashboard From V6"}}]}
This creates a new dashboard:
curl http://localhost:5601/api/saved_objects/dashboard
{"saved_objects":[{"id":"AV0UtNI1urO7MVHxeLol","type":"dashboard","version":1,"attributes":{"title":"My Test Dashboard"}},{"id":"test","type":"dashboard","version":1,"attributes":{"title":"My Test Dashboard From V6"}}],"total":2,"per_page":20,"page":1}
But when I now query by id, I get the old v5 dashboard like earlier.
curl http://localhost:5601/api/saved_objects/dashboard/test
{"id":"AV0UtNI1urO7MVHxeLol","type":"dashboard","version":1,"attributes":{"title":"My Test Dashboard"}}
There was a problem hiding this comment.
When attempting to import that dashboard I get an error that the document already exists:
curl -X POST -H "Content-Type: application/json" -H "kbn-xsrf: 6.0.0-alpha3" -d '{ "version": "6.0.0-alpha3", "objects": [{ "id": "test", "type": "dashboard", "attributes": { "title": "My Test Dashboard From V6" }}]}' http://localhost:5601/api/kibana/dashboards/import
{"objects":[{"id":"test","type":"dashboard","attributes":{"title":"My Test Dashboard From V6"},"error":{"message":"[dashboard][test]: version conflict, document already exists (current version [3])"}}]}
Ping me tomorrow and let's go over this live.
There was a problem hiding this comment.
@thomasneirynck and I worked through and were able to distil the problem.
A user on 5.6 has the following visualization object in ES:
{
_id: 'foo',
_type: 'visualization'
}
They export the saved object, then run the Kibana index migration.
This will result in their ES object being transformed into the following:
{
_id: 'abc123',
_type: 'doc',
_source: {
type: 'visualization',
legacyId: 'foo'
}
}
If the user then re-imports the export previously made in 5.6 they would be forcing the document to be created with the id specified in order to avoid breaking associations.
{
_id: 'foo',
_type: 'doc',
_source: {
type: 'visualization'
}
}
The current thought is we need to first query for the document before inserting. If a document is returned, and we're forcing the overwrite we will simply update the document. For bulkCreate we would first need to perform a bulkGet on the documents. This doesn't seem like a great solution, as it's not atomic and feels like a hack we might not be able to get away from.
There was a problem hiding this comment.
@epixa and I discussed. There are a few changes we're going to make to address this.
First, instead of moving the existing _id to legacyId and auto-generating _id, we're going to set to it something we can expect: ${_type}:${_id}.
Secondly, we're going to begin including a saved_object_version in the exports, beginning with version 2. This will allow us to assume all existing exports are version 1 and should use the concatenated type and ID as the id.
|
@thomasneirynck, thanks for the review.
Previously id's were only unique to a single document type. Since the removal of types, we must move all of our documents under a single type which causes id conflicts. To address this, we are re-indexing and copying the existing
@simianhacker, can you provide insights into the reason specifying the content-disposition for the dashboard export API? |
|
@tylersmalley functionally this appears to be working properly. However, the SavedObjectsClient tests seem to only test the v5 functionality for create/update methods and not the fallbacks. |
|
@kobelb, good catch - will add those. |
Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
|
@kobelb - tests have been added surrounding the v6 fallback |
|
@kobelb, let me know if this looks good to you and I will go ahead an merge. |
Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
1f21ffb to
c0fa7af
Compare
Example:
```
{
_type: 'url'
_source: {
url: '/app/kibana'
}
}
```
Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
* Also fixes tests Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
c0fa7af to
8f6ba95
Compare
| const id = doc._id.replace(`${type}:`, ''); | ||
| const attributes = get(doc, `_source.${type}`) || doc._source; | ||
|
|
||
| const attributes = doc._type === 'doc' ? get(doc, `_source.${type}`) : doc._source; |
There was a problem hiding this comment.
Can we use the V6_TYPE from the 'saved_objects_client' here?
|
|
||
| // migrated v5 indices and objects created with a specified ID | ||
| // have the type prefixed to the id. | ||
| const id = doc._id.replace(`${type}:`, ''); |
There was a problem hiding this comment.
What's preventing this from removing the ${type}: from weird unconverted v5 ids? Could we do the doc._type === V6_TYPE check that's below here as well?
Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
Support for v5 and v6 index mappings
|
5.x: 7932e62 |
|
💃 💃 |
Made updates to #12334 and re-opened while @jbudz is out.
This adds a compatibility layer to the client of the saved object allowing it to support and return the same responses for crud operations on kibana indices under a single type (v6) and kibana indices using multiple types v{5,6}
To test v6 mappings:
Reindex:
Set kibana.index to .kibana-6.
The server will error until everything is switched over to use the api, but endpoints should be available. Test cases detailed in the saved objects pr should behave the same.