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

[Index template] Fix editor should support mappings types #55804

Conversation

sebelga
Copy link
Contributor

@sebelga sebelga commented Jan 24, 2020

This PR adds support for mappings type in the Template UI (create/cone/edit).

Through the UI it is not possible to create a new template with a type, but it is possible that a template being edited has a custom type attached.

Fix #55645

Note for the reviewer

Activate the "Hide whitespace changes"

Screen Shot 2020-01-24 at 13 59 47

How to test

  • Create a template with a custom type in the dev console
PUT _template/my_index_template?include_type_name
{
  "index_patterns": [
    "my_index"
  ],
  "mappings": {
    "my_custom_type": {
      "properties": {
        "name": {
          "type": "keyword"
        }
      }
    }
  }
}
  • Then navigate to the Template UI and edit that template.
  • Got to the "Mappings" and add a field
  • Got to the review tab and make sure that the request still contains the custom type ("my_custom_type")

Screen Shot 2020-01-24 at 13 33 34

  • Save the template. No error should appear and you should be brought back to the template list
  • The mappings of the template should have the custom type

Screen Shot 2020-01-24 at 13 34 00

Indices created with the template

Create an index that matches the template

POST my_index/_doc
{
  "name": "a" 
}
  • Go to the Index Management --> Indices tab
  • Click on the "my_index" index
  • Navigate to the "Mappings" tab
  • The custom type should appear

Screen Shot 2020-01-24 at 13 34 17

Note for 7.5 backport

As the fix to support types requires the files from the lib folder of the mappings editor, we will need to manually cherry-pick on the backport PR the commits that don't modify those files in this PR, and then copy over the needed files manually in a separate commit.

Release note

We fixed a bug introduced in 7.4 when we shipped the index template UI. The template editor did not take into account custom type defined for the mappings when fetching and editing an index template. The mapping type ended up being converted to the default _doc losing the ability to query documents on the custom type.

@sebelga sebelga added bug Fixes for quality problems that affect the customer experience Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.6.0 labels Jan 24, 2020
@elasticmachine
Copy link
Contributor

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

@sebelga sebelga added Feature:Index Management Index and index templates UI release_note:skip Skip the PR/issue when compiling release notes labels Jan 24, 2020
Copy link
Contributor

@cjcenizal cjcenizal 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 Seb! I found a bug, though. If you use the "Load JSON" modal to load in mappings that contain a type, the UI resets to the default state. This is particularly strange if you're looking at mappings that already contain a type. You could copy the mappings from the Review tab and paste it into the "Load JSON" modal and you'd expect it to show you the mappings you already have.

image

return typedMappings;
};

export const doesMappingsHasType = (mappings: GenericObject = {}): boolean =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I think the name doMappingsHaveType or even isTypedMappings would be more grammatically correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For sure 😊

@sebelga
Copy link
Contributor Author

sebelga commented Jan 28, 2020

Thanks for the review @cjcenizal ! Great catch with the "Load JSON". I fixed it, can you have another look?

Custom type should be detected when loading the JSON and kept as is. If multi-type are detected, the unparsed (not validated) JSON is forwarded as the UI takes care of displaying a warning.

Custom type can be a mappings configuration

{
  "dynamic": { // Custom type is a mappings configuration
    "_source": {
      "enabled": true,
      "includes": [],
      "excludes": []
    },
    "_meta": {},
    "_routing": {
      "required": false
    },
    "dynamic": true,
    "numeric_detection": false,
    "date_detection": true,
    "properties": {
      "cool": {
        "type": "text"
      },
      "name": {
        "type": "keyword"
      }
    }
  }
}

but can't be "properties"

{
  "properties": {
    "_source": {
      "enabled": true,
      "includes": [],
      "excludes": []
    },
    "_meta": {},
    "_routing": {
      "required": false
    },
    "dynamic": true,
    "numeric_detection": false,
    "date_detection": true,
    "properties": {
      "cool": {
        "type": "text"
      },
      "name": {
        "type": "keyword"
      }
    }
  }
}

Validation lib

I added some comments in the code in the mappings_validator.ts to use another lib for validation instead of io-ts. It works OK in most cases but not to prevent unknown properties on objects. I've had great success with Avj, which works both on the server and in the browser.

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

I tested the following conditions:

  • Removing the custom type from an index template
  • Adding a custom type to an index template
  • Loading multiple custom types
  • Loading JSON with all advanced options customized

Everything looks great!

I noticed that the text for the warning callout no longer makes sense, since custom types are supported:

image

How about we tweak the copy to say "Multiple mapping types detected" - "The mappings for this template uses multiple types, which are not supported."

image

}

if (hasCustomType) {
customType = Object.keys(unparsed)[0];
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nit: In the case of isMultiTypeMappings === true, then customType loses meaning because it's not the only custom type. Maybe we should change the condition to hasCustomType && !isMultiTypeMappings so that customType is left undefined if there are multiple type mappings?

}

// Custom type can't be "properties", ES will not treat it as such
// as it is reserved for fields definition
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️ Great comments!

* It is extremely verbose, and as we can see below, there isn't a way to make the validation fail on unknown properties.
* We need the hack below to return errors when unknown properties are detected.
*
* TODO: I propose to refactor and use Ajv (https://ajv.js.org/) that has a much simpler implementation for the validation we need to do.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move this out of comments and into an issue? That will allow for team discussion and prevent this from being lost.

Copy link
Contributor

Choose a reason for hiding this comment

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

CC @jloleysens curious to hear your thoughts on this too since I believe you introduced io-ts.

@@ -235,6 +261,15 @@ export const validateMappingsConfiguration = (
});
}

if (Boolean(unknownConfigurationParameters.length)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nit: unknownConfigurationParameters.length > 0 evaluates to a boolean too, and I think also more clearly expresses our intention here (similar comment for the condition on line 268).

@sebelga
Copy link
Contributor Author

sebelga commented Jan 29, 2020

Thanks for the review @cjcenizal ! I addressed your suggestions on the PR. I will merge and backport to 7.6 when CI is green.

I created an issue to move to avj validator: #56272

@sebelga sebelga merged commit 9aa4324 into elastic:7.x Jan 29, 2020
@sebelga sebelga deleted the fix/Index-template-editor-should-support-mappings-types branch January 29, 2020 07:05
sebelga added a commit to sebelga/kibana that referenced this pull request Jan 29, 2020
jloleysens added a commit to jloleysens/kibana that referenced this pull request Jan 29, 2020
…55831

* '7.x' of github.com:elastic/kibana: (78 commits)
  Re-enable watcher FireFox functional test (elastic#56112) (elastic#56294)
  [Metrics UI] Fixing title truncation in Metrics Explorer (elastic#55917) (elastic#56248)
  [APM] x-axis labels on Error occurrences chart are incorrect based on Kibana timezone (elastic#55686) (elastic#56288)
  Migrate saved_object_save_as_checkbox directive to timelion (elastic#56114) (elastic#56286)
  [APM] Treat error.exception.stacktrace.line as optional (elastic#55733) (elastic#55840)
  Remove alerts and actions from feature catalogue (elastic#56140) (elastic#56208)
  Migrate UI capabilities to use new platform APIs (elastic#56070) (elastic#56207)
  [ML] Add functional tests for analytics UI: creation addition and regression/outlier results (elastic#56059) (elastic#56191)
  [SIEM] Overview page feedback (elastic#56261) (elastic#56276)
  [NP Cleanup] Remove ui/public/inspector (elastic#55677) (elastic#56271)
  [Index template] Fix editor should support mappings types (elastic#55804)
  fixes map index message (elastic#56104) (elastic#56194)
  [SIEM] [TIMELINE] Only add endpoint logo when on event.module === endgame (elastic#56263) (elastic#56269)
  [SIEM] Fix filters on Hosts and Network page (elastic#56234) (elastic#56267)
  [SIEM] Adds ability to infer the newsfeed.enabled setting (elastic#56236) (elastic#56265)
  [SIEM][Detection Engine] critical blocker for updated rules (elastic#56259)
  [SIEM] Put the notice for rules in comment block (elastic#56123) (elastic#56246)
  [SIEM][Detection Engine] critical blocker, fixes ordering issue that causes rules to not run the first time (elastic#56256)
  [Reporting/NP] Migration of Reporting Security dependency (elastic#56046) (elastic#56198)
  [SIEM] Add link to endpoint app through reference.url (elastic#56211) (elastic#56250)
  ...

# Conflicts:
#	x-pack/plugins/watcher/public/plugin.ts
@sebelga sebelga added release_note:fix and removed release_note:skip Skip the PR/issue when compiling release notes labels Feb 24, 2020
@kibanamachine
Copy link
Contributor

💔 Build Failed

History

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:Index Management Index and index templates UI release_note:fix Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants