-
Notifications
You must be signed in to change notification settings - Fork 947
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
docs: remove attributes.
in SavedObjectsFindOptions.fields example
#4695
Conversation
Signed-off-by: Joshua Li <joshuali925@gmail.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4695 +/- ##
=======================================
Coverage 66.50% 66.50%
=======================================
Files 3403 3403
Lines 65026 65026
Branches 10401 10401
=======================================
+ Hits 43243 43245 +2
+ Misses 19214 19213 -1
+ Partials 2569 2568 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
why does coverage change when the commit only changes a comment? |
CodeCov has some margin of error - we often see diffs for changes that have no test changes at all. Don't worry about it. |
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.
LGTM
@joshuali925 do you think there is a potential for some recent change to have modified this behavior or is it a potential weird behavior in the current client? Was playing around with the import saved objects API and attempting to hit that endpoint with a POST without If I was to implement an OpenSearch Dashboards client I would probably ensure that the usage is consistent so users don't have to modify data to include specific keys that we already map in another endpoint. To be honest your point makes more sense but it makes me question if something off is happening somewhere. |
@kavilla I'm not sure why the rest API and saved object js client are inconsistent, I've not used the rest API much. But I don't think it's a recent change on the client, the related code I posted has not changed for more than 4 years side question, where is the documentation for saved object rest API? I couldn't find them in https://opensearch.org/docs/latest/dashboards/index/ |
|
Yeah as @joshuarrrr commented it is to be added. I can try to scrap something together today. |
@kavilla Assigning to you to either finish the final review or provide updated docs and close. |
@joshuali925, could this be the result of the type of If so then perhaps we can update this PR so that it references both examples |
@kavilla to be honest i don't remember the details or i didn't test on all types of saved objects, but based on the code i don't see the serialization logic changing for different object types. |
im currently looking into this area so I'm going to move it to 2.15 and add it to the the saved objects OE. |
…4695) (#6917) (cherry picked from commit 31b79e4) Signed-off-by: Joshua Li <joshuali925@gmail.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Josh Romero <rmerqg@amazon.com>
Description
Corrects the example in the comment for SavedObjectsFindOptions. The comment is misleading because it includes
attributes
, which will be a field in the responseBut the raw document does not include
attributes
field, saved object client will put fields under saved object type in the raw document as theattributes
field.OpenSearch-Dashboards/src/core/server/saved_objects/serialization/serializer.ts
Line 89 in 471877f
Raw document sample:
So including
attributes.<field>
(e.g.attributes.title
) in find options will not give correct results. Using<field>
(e.g.title
) is enough because the type will be prepended.OpenSearch-Dashboards/src/core/server/saved_objects/service/lib/included_fields.ts
Line 48 in 471877f
Issues Resolved
Screenshot
Testing the changes
Check List
yarn test:jest
yarn test:jest_integration
yarn test:ftr