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

Fail early on import, and don't prompt to overwrite when failing #11059

Merged
merged 3 commits into from Apr 10, 2017

Conversation

w33ble
Copy link
Contributor

@w33ble w33ble commented Apr 5, 2017

Closes #5694 and #11057

  • When JSON.parse fails, show error and exit early
  • When importing a non-array, show error and fail early

To test, try importing this JSON file (from #5694).

Also try importing a file that is not valid json, by, say, removing a quote from the file.

we expect to get an array of objects, so show an error message if we find something that is not an array
@ycombinator
Copy link
Contributor

ycombinator commented Apr 6, 2017

I tried both cases: importing a non-JSON file and importing a JSON file with a syntax error (I tried twice: first, by removing a double quote, and second, by removing a comma). In all cases I get the same error message: "Saved Objects: The file could not be processed.".

Is it possible to produce the new error message you introduced in this PR: "JSON format is invalid and can not be imported."?

@ycombinator ycombinator self-requested a review April 6, 2017 14:07
@w33ble
Copy link
Contributor Author

w33ble commented Apr 6, 2017

I tried both cases: importing a non-JSON file and importing a JSON file with a syntax error

Those are actually the same case. Both make JSON.parse throw.

You should get the new error when importing a valid JSON file where the root element is not an array, like the example I linked to in the description.

@ycombinator
Copy link
Contributor

ycombinator commented Apr 6, 2017

Ah, I think I understand now. The "JSON format" in the new error message introduced in this PR refers to the structure of the JSON that Kibana expects for saved objects JSON.

I think perhaps we could change that message to something like "Saved objects file format is invalid and cannot be imported"? The fact that the file contains JSON seems like an implementation detail that the user shouldn't need to care about.

@w33ble
Copy link
Contributor Author

w33ble commented Apr 6, 2017

I like it. PR updated.

@epixa epixa added v5.3.2 and removed v5.3.1 labels Apr 6, 2017
Copy link
Contributor

@ycombinator ycombinator left a comment

Choose a reason for hiding this comment

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

LGTM.

@BigFunger BigFunger self-requested a review April 10, 2017 14:51
Copy link
Contributor

@BigFunger BigFunger left a comment

Choose a reason for hiding this comment

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

LGTM

@w33ble w33ble merged commit fb7be62 into elastic:master Apr 10, 2017
w33ble added a commit that referenced this pull request Apr 10, 2017
)

* stop importing if JSON parsing fails

* fail early if improper JSON

we expect to get an array of objects, so show an error message if we find something that is not an array

* clarify import error message
w33ble added a commit that referenced this pull request Apr 10, 2017
)

* stop importing if JSON parsing fails

* fail early if improper JSON

we expect to get an array of objects, so show an error message if we find something that is not an array

* clarify import error message
@w33ble
Copy link
Contributor Author

w33ble commented Apr 10, 2017

5.x 84d0d65
5.3 20b88b6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants