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

[data views] Implement content management api #153733

Closed
wants to merge 52 commits into from

Conversation

mattkime
Copy link
Contributor

@mattkime mattkime commented Mar 27, 2023

Summary

Data views implementation of content management api.

This PR includes the changes in #154985 which should be merged first.

@mattkime mattkime changed the title initial changes, mostly working...why doesn't find work? [data views] implement content management api Mar 27, 2023
return response.map<SavedObject<T>>(simpleSavedObjectToSavedObject);
async find(options: SavedObjectsClientCommonFindArgs) {
console.log('******* find', options);
const results = await this.contentManagemntClient.search<DataViewSearchIn, DataViewSearchOut>({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sebelga I must be doing something wrong here.

Copy link
Contributor

Choose a reason for hiding this comment

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

You need to pass an object for the query.

Screenshot 2023-03-27 at 11 20 39

Add an empty object if you don't need any parameters

const results = await this.contentManagemntClient.search<DataViewSearchIn, DataViewSearchOut>({
  contentTypeId: 'index-pattern',
  query: {},
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

export type DataViewCreateIn = CreateIn<
typeof DataViewContentType,
DataViewAttributes,
CreateOptions
Copy link
Contributor

Choose a reason for hiding this comment

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

You are explicitely declaring that DataViewCreateIn takes CreateOptions. You now have to pass it in saved_objects_client_wrapper.ts L91

return (await this.contentManagemntClient.create<DataViewCreateIn, DataViewCreateOut>({
contentTypeId: 'index-pattern',
data: attributes,
// this is required, shouldn't be.
Copy link
Contributor

Choose a reason for hiding this comment

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

See my comment above

@mattkime mattkime changed the title [data views] implement content management api [data views] Implement content management api Mar 29, 2023
@@ -14,7 +14,6 @@
"@kbn/object-versioning",
"@kbn/core-saved-objects-api-server-mocks",
"@kbn/core-saved-objects-api-server",
"@kbn/saved-objects-finder-plugin",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was necessary to remove a circular dependency.

@mattkime mattkime added Feature:Data Views Data Views code and UI - index patterns before 8.0 Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. release_note:skip Skip the PR/issue when compiling release notes labels Apr 16, 2023
@mattkime mattkime marked this pull request as ready for review April 16, 2023 16:40
@mattkime mattkime requested review from a team as code owners April 16, 2023 16:40
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

return savedObjects.client;
};

export class DataViewsStorage implements ContentStorage {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sebelga This would be my starting point for abstracting ContentStorage for use with Saved Objects. Is there anything in particular that I should keep an eye on regarding the Maps implementation? It currently has versioning code so I'd like to make sure I we're on the same page regarding what the end product should look like.

Copy link
Contributor

Choose a reason for hiding this comment

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

Overall looks good. You would probably need to add all the BWC logic for up/down transform + validate the inputs/outputs.
You would also need to implement the bulkGet method.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
dataViews 47 49 +2

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/content-management-utils - 33 +33
data 2577 2575 -2
dataViews 245 255 +10
total +41

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
dataViewManagement 118.6KB 118.6KB +15.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
data 403.9KB 403.9KB +2.0B
dataViews 43.8KB 44.4KB +554.0B
total +556.0B
Unknown metric groups

API count

id before after diff
@kbn/content-management-utils - 36 +36
data 3271 3297 +26
dataViews 1029 1044 +15
total +77

ESLint disabled line counts

id before after diff
securitySolution 432 435 +3

References to deprecated APIs

id before after diff
@kbn/core 38 32 -6
@kbn/core-saved-objects-api-browser 60 24 -36
@kbn/core-saved-objects-api-server 17 5 -12
@kbn/core-saved-objects-api-server-internal 8 11 +3
@kbn/core-saved-objects-browser-internal 251 170 -81
@kbn/core-saved-objects-browser-mocks 35 29 -6
@kbn/core-saved-objects-common 9 6 -3
@kbn/core-saved-objects-import-export-server-internal 58 19 -39
@kbn/core-saved-objects-server-internal 6 9 +3
@kbn/core-ui-settings-server-internal 12 3 -9
canvas 152 134 -18
cases 90 36 -54
data 114 86 -28
dataViews 148 50 -98
fleet 62 68 +6
graph 88 97 +9
home 54 75 +21
lists 95 83 -12
osquery 41 17 -24
savedObjects 106 100 -6
savedObjectsManagement 35 23 -12
savedObjectsTagging 75 36 -39
savedObjectsTaggingOss 29 20 -9
securitySolution 390 459 +69
synthetics 96 51 -45
upgradeAssistant 20 14 -6
total -432

Total ESLint disabled count

id before after diff
securitySolution 512 515 +3

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

perPage: query.limit,
page: query.cursor ? +query.cursor : undefined,
defaultSearchOperator: 'AND',
searchFields: options.searchFields || ['title', 'name'],
Copy link
Contributor

Choose a reason for hiding this comment

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

You are exposing searching accross any fields on the database from the UI. It is better to limit what fields can be searched on.
In maps I added an onlyTitle option to avoid that (https://github.com/elastic/kibana/pull/153304/files#diff-3196f0787529ecacf22f90a6081b048f25eb3cf980b548437da12df2879ccdadR287).

@jughosta
Copy link
Contributor

@mattkime There are multiple conflicts with main.

Also, does it depend on changes in #155680 ? Should it be reviewed later then?

@mattkime
Copy link
Contributor Author

@jughosta You're correct, this isn't quite ready for review.

@kertal
Copy link
Member

kertal commented May 23, 2023

I guess this can be closed in favor of #155803?

@mattkime mattkime closed this May 23, 2023
@kertal
Copy link
Member

kertal commented May 23, 2023

thx!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Data Views Data Views code and UI - index patterns before 8.0 release_note:skip Skip the PR/issue when compiling release notes Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants