elastic / kibana

Your window into the Elastic Stack
https://www.elastic.co/products/kibana
Other
19.65k stars 8.23k forks source link

Uploading mappings JSON with multiple types causes browser to freeze #107464

Open cjcenizal opened 3 years ago

cjcenizal commented 3 years ago

To reproduce:

  1. Start Kibana on 7.x or on 7.14
  2. Create a new legacy index template in the UI (see Index Management README for steps on how to get the UI to appear)
  3. On the mappings step, upload the below JSON with multiple types. The browser will freeze.

See extract_mappings_definiton.ts for context on this functionality.

{
  "my_custom_type1": {
    "_source": {
      "enabled": false
    },
    "properties": {
      "name1": {
        "type": "keyword"
      }
    }
  },
  "my_custom_type2": {
    "_source": {
      "enabled": false
    },
    "properties": {
      "name1": {
        "type": "keyword"
      }
    }
  }
}
elasticmachine commented 3 years ago

Pinging @elastic/kibana-stack-management (Team:Stack Management)

sabarasaba commented 3 years ago

I spent a few minutes checking this up, there's an onMappingsEditorUpdate function passed to the MappingsEditor that gets called infinitely by several hooks that depend on it and also invoke it. I was also able to replicate this bug for both legacy and non-legacy index templates.

I tracked it down to this useEffect which I believe its not needed, but I'm not entirely sure what was the reasoning behind it. Removing it appears to have no side effects AFAICT: the bug is gone, tests are all green and the UI appears to work as it was intended.

@sebelga I'm not very familiar with this app but I see you worked on this some time ago so perhaps you might be able to see something else I might be missing 🤔

sebelga commented 3 years ago

It would be interesting to understand what cause the useEffect to enter into an infinite loop. Is it the onChange or the value that gets sent a new reference each time? (you can debug it by creating a useRef and compare the old ref with the new).

I'd say that the useEffect had a purpose for multi type mappings but if your manual testing show that it does not then I guess it could be removed 👍