Skip to content

[Console]remove completion for type for filter queries and aggs#68103

Merged
hendrikmuhs merged 1 commit intoelastic:masterfrom
hendrikmuhs:remove-completion-for-type
Jun 4, 2020
Merged

[Console]remove completion for type for filter queries and aggs#68103
hendrikmuhs merged 1 commit intoelastic:masterfrom
hendrikmuhs:remove-completion-for-type

Conversation

@hendrikmuhs
Copy link

Summary

remove suggesting "type" for filter queries and aggregations, because "type"
got removed in the backend.

How to test

Start Kibana and navigate to Console. Type a query that contains a filter, type should no longer be suggested. Example:

GET _search
{
  "query": {
    "bool": {
      "filter": [
        {
          "t"
          }
        }
      ]
    }
  }
}

Note: In 7.x the type query is deprecated and it's highly discouraged to use it, I leave it up to you to decide whether to backport this change or not.

@hendrikmuhs hendrikmuhs added Feature:Console Dev Tools Console Feature Feature:Dev Tools v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more t// release_note:skip Skip the PR/issue when compiling release notes labels Jun 3, 2020
@hendrikmuhs hendrikmuhs requested a review from jloleysens June 3, 2020 14:53
@hendrikmuhs hendrikmuhs requested a review from a team as a code owner June 3, 2020 14:53
@elasticmachine
Copy link
Contributor

Pinging @elastic/es-ui (Team:Elasticsearch UI)

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

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

Copy link
Contributor

@jloleysens jloleysens left a comment

Choose a reason for hiding this comment

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

Great work @hendrikmuhs ! Thanks for cleaning that up.

I tested locally and everything looks to be in good order.

I noticed that in mappings there is completion support for _parent with a type on it:

PUT an-index
{
  "mappings": {
    "_parent": {
      "type": ""
    }
  }
}

I could not see this field in the ES docs for 7.7, do you know whether it should be removed too? No need to block merging on this :)

@hendrikmuhs
Copy link
Author

I noticed that in mappings there is completion support for _parent with a type on it:

PUT an-index
{
  "mappings": {
    "_parent": {
      "type": ""
    }
  }
}

I could not see this field in the ES docs for 7.7, do you know whether it should be removed too? No need to block merging on this :)

I think this type corresponds to a data type, not the deprecated/removed doc type.

@hendrikmuhs hendrikmuhs changed the title [Console] remove completion for type query for filter queries and aggregations [Console]remove completion for type for filter queries and aggs Jun 4, 2020
@hendrikmuhs hendrikmuhs merged commit 50013bf into elastic:master Jun 4, 2020
@hendrikmuhs hendrikmuhs deleted the remove-completion-for-type branch June 4, 2020 12:30
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jun 4, 2020
* master: (26 commits)
  [Console]remove completion for type for filter queries and aggs (elastic#68103)
  [ML] Transforms: Filter aggregation support (elastic#67591)
  [ES UI Shared] Monaco XJSON (elastic#67485)
  [Index Management] Add data streams functionality to indices tab (elastic#67940)
  [Discover] Fix renaming of saved search not displayed in breadcrumb (elastic#67577)
  [SECURITY] Rename siem plugin to security_solution (elastic#67902)
  [Uptime] Fix Telemetry Api flaky test (elastic#67358)
  [Data plugin] Add configuration property to enable / disable autocomplete (elastic#67847)
  remove scripts. prettire update has been done (elastic#68130)
  Closes elastic#68055 by detecting the local Kibana version and using that as (elastic#68198)
  [apm] docs: add deployment annotation example (elastic#67408)
  [ML] Extend population preview chart to show actual and typical value (elastic#67569)
  Refactor index management client integration tests for scalability (elastic#67917)
  Add generator function that creates multiple alerts (elastic#67713)
  chore(NA): remove config arg from os packages (elastic#67871)
  [Reporting] Move code out of Legacy (elastic#67904)
  [Metrics UI] Add overrides to Snapshot API to support alert previews (elastic#68125)
  [Security] [Cases] Manage timeline UI API (elastic#67719)
  [ENDPOINT][INGEST]Task/endpoint ingest update (elastic#67234)
  Fix code coverage for jest, upload merged reports (elastic#68149)
  ...
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jun 8, 2020
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 68103 or prevent reminders by adding the backport:skip label.

@cjcenizal
Copy link
Contributor

Here are my thoughts in response to Hendrik's very good question.

I see the current form of Console acting as an unopinionated window into the Elasticsearch APIs. I think it exists to support accessing these APIs with tools like autocomplete and XJSON. I think we should consider UX enhancements like warnings (e.g. #39491), but it seems against the spirit of Console to hide aspects of Elasticsearch, even if they are deprecated. I think someone who tries to access type would be surprised to find it missing from autocomplete, and possibly file a bug report. If we truly don't want users to use parts of Elasticsearch's API, I think the solution we'd reach for is to remove those parts from the API.

@cjcenizal cjcenizal added backport:skip This PR does not require backporting and removed backport missing Added to PRs automatically when the are determined to be missing a backport. labels Jun 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip This PR does not require backporting Feature:Console Dev Tools Console Feature Feature:Dev Tools release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more t// v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants