Fix invalid Reporting integration test data

[jportner]

Starting in 7.10, an invalid saved object was introduced into the Reporting API integration test data via #76998:

Excerpt of `x-pack/test/functional/es_archives/reporting/ecommerce_kibana_spaces/data.json.gz` that is currently present in the 7.10, 7.11, and 7.12 branches

{
  "type": "doc",
  "value": {
    "id": "config:8.0.0",
    "index": ".kibana_1",
    "source": {
      "config": {
        "buildNum": 9007199254740991,
        "dateFormat:tz": "UTC",
        "defaultIndex": "aac3e500-f2c7-11ea-8250-fb138aa491e7"
      },
      "migrationVersion": {
        "config": "7.9.0"
      },
      "namespace": "default",
      "references": [
      ],
      "type": "config",
      "updated_at": "2020-08-26T22:46:48.711Z"
    }
  }
}

Starting in 7.13, this object was updated to include "migrationVersion": { "config": "7.13.0" } via #92620 (and the .json.gz file was replaced with a .json file):

Excerpt of `x-pack/test/functional/es_archives/reporting/ecommerce_kibana_spaces/data.json` that is currently present in the 7.x and master branches

{
  "type": "doc",
  "value": {
    "id": "config:8.0.0",
    "index": ".kibana_1",
    "source": {
      "config": {
        "buildNum": 9007199254740991,
        "dateFormat:tz": "UTC",
        "defaultIndex": "aac3e500-f2c7-11ea-8250-fb138aa491e7"
      },
      "migrationVersion": {
        "config": "7.13.0"
      },
      "namespace": "default",
      "references": [
      ],
      "type": "config",
      "updated_at": "2020-08-26T22:46:48.711Z"
    }
  }
}

Both of these objects are invalid because they include "namespace": "default", which is not a valid attribute for the object's namespace field (it should be undefined to indicate that the object is in the Default space). My best guess for how this occurred is that the test data was exported from Kibana in the first PR linked above, and it was manually modified when being added to the .data.json.gz file.

This invalid object results in the following behavior in the SavedObjectsSerializer:

• rawToSavedObject(): successfully deserializes the document into a saved object with "namespace": "default"
• isRawSavedObject(): returns false, because the namespace prefix "default" is not found in the object's raw ID

However, isRawSavedObject() is only used during migrations, and the object in question had an up-to-date migrationVersion, therefore it never triggered a migration.

We only ran into this test failure because, up until now, the latest migrationVersion for a config object was "7.9.0". However, #93409 introduced a new migration for "7.12.0", which caused the invalid saved object to be migrated during CI in the 7.12 branch, resulting in a test failure.

So we have a few open questions:

1. How do we best prevent this from happening again? I think our best course of action may be to change rawToSavedObject() to first check the outcome of isRawSavedObject(), and throw an error if the document in question is not a valid raw saved object
2. What should we do about migration inconsistencies during CI, if anything?

CC @lizozom @rudolf

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: a467717
• Storybooks Preview

Metrics [docs]

✅ unchanged

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

[rudolf]

Thanks for investigating and fixing this!

How do we best prevent this from happening again? I think our best course of action may be to change rawToSavedObject() to first check the outcome of isRawSavedObject(), and throw an error if the document in question is not a valid raw saved object

I agree that it would make more sense to let rawToSavedObject throw the CorruptSavedObjectError. It's still possible for a test to create a corrupt fixture which goes undetected if that saved object fixture is never loaded in the test, though that's less common.

What should we do about migration inconsistencies during CI, if anything?

I think the root cause here is that fixtures are edited by hand and then esArchiver writes these directly to the .kibana index bypassing all validation. So the long term fix would be to move all tests over to kbnServer.importExport #92526

[jportner]

Thanks for investigating and fixing this!

😄

I agree that it would make more sense to let rawToSavedObject throw the CorruptSavedObjectError. It's still possible for a test to create a corrupt fixture which goes undetected if that saved object fixture is never loaded in the test, though that's less common.

OK, I'm happy to add that -- do you think we should add it in this PR, or open a separate one that is only backported to 7.x (to be released with 7.13.0)? I'm leaning towards the latter, as it's mostly a preventative measure moving forward. It seems there's a small risk that it could break any malformed-but-currently-functioning objects that exist in clusters, so it's reasonable to add that fix into the next minor, would you agree?

I think the root cause here is that fixtures are edited by hand and then esArchiver writes these directly to the .kibana index bypassing all validation. So the long term fix would be to move all tests over to kbnServer.importExport #92526

👍

[lizozom]

@jportner anything preventing us from merging this?

[rudolf]

I'm leaning towards the latter, as it's mostly a preventative measure moving forward. It seems there's a small risk that it could break any malformed-but-currently-functioning objects that exist in clusters, so it's reasonable to add that fix into the next minor, would you agree?

Agreed.