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

Use snake case for saved objects import/export API #54123

Closed
5 tasks
rudolf opened this issue Jan 7, 2020 · 6 comments
Closed
5 tasks

Use snake case for saved objects import/export API #54123

rudolf opened this issue Jan 7, 2020 · 6 comments
Labels
Breaking Change Feature:New Platform Feature:Saved Objects stalled Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@rudolf
Copy link
Contributor

rudolf commented Jan 7, 2020

As per #52284 (comment) we should stick to the convention for snake case API's. Therefore, before marking the import/export API as "stable" we should fix the following camel case fields:

Import API changes

  • Request
    • createNewCopies to create_new_copies
  • Response
    • successCount to success_count
    • successResults to success_results
    • A few fields in the successResults[] array are also not snake_case:
      • SavedObjectsImportSuccess.destinationId to destination_id
      • SavedObjectsImportSuccess.createNewCopy to create_new_copy
    • Some of the fields on the errors array need updating as well:
      • SavedObjectsImportActionRequiredWarning.actionPath to action_path
      • SavedObjectsImportActionRequiredWarning.buttonLabel to button_label
      • SavedObjectsImportConflictError.destinationId to destination_id
      • SavedObjectsImportUnknownError.statusCode to status_code
      • SavedObjectsImportAmbiguousConflictError.destinations.updatedAt to updated_at

Export API changes

  • Request
    • hasReference to has_reference
    • includeReferencesDeep to include_references_deep
    • excludeExportDetails to exclude_export_details
  • Response
    • none, response is just a string containing the ndjson file

We should not be making any changes to the actual file format of the ndjson file.

Related APIs that should also change as they contain some of the same fields listed above:

  • _resolve_import_errors
  • _copy_saved_objects

Implementation Scope

This should probably be done in a multi-phased approach:

  • Add support for the new snake_case request parameters
    • In 7.x we need to continue accepting the old camelCase parameters as well
    • Validation logic will need to be duplicated or updated to accept either one or the other options
  • Add support for the new snake_case response fields
    • These should be duplicated in 7.x so that we're returning both the old and new names
  • Update the Import/Export UI code to use the new snake_case parameters and response fields
  • Add deprecation log when camelCase request parameters are supplied & update documentation
  • Remove support for the snake_case request parameters and response fields in master/8.0 branches (including documentation)
@rudolf rudolf added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform Feature:Saved Objects labels Jan 7, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform (Team:Platform)

@lukeelmers
Copy link
Member

We decided that, since this is a frequently used API, we will defer making the actual breaking change in 8.0, and instead address the other items in the scope (introducing new snake case fields, marking camelCase ones as deprecated).

Actual removal of the deprecated fields will be treated as a follow-up task, and timing for that is TBD.

@lukeelmers
Copy link
Member

I have a draft PR in progress for this and, unsurprisingly, the scope keeps growing:

  • There's also core's /_resolve_import_errors, which we should probably update for consistency. I'm looking at this now.
  • The spaces /_copy_saved_objects route uses these same request params (destinationId, createNewCopy, ignoreMissingReferences) as it uses import/export under the hood, so probably we should update it as well for consistency?
    • WDYT @jportner? Would like to get your two cents before I start updating everybody's favorite security & spaces integration tests :trollface:
  • The design of the SO import warnings API relies on the actionPath and buttonLabel being passed straight through from the REST API, so I had to make some adjustments to the types & response handling to ensure warnings can still be registered in camelCase, even if they are returned by the api in snake_case
  • There's the issue of the contracts for the importer and exporter. For example, SavedObjectsImportResponse is used for both the http route and the importer on the plugin contract, so in order to keep these changes contained to the routes as I did with the import warnings, I need to look into disentangling the two.

@jportner
Copy link
Contributor

jportner commented Sep 1, 2021

The spaces /_copy_saved_objects route uses these same request params (destinationId, createNewCopy, ignoreMissingReferences) as it uses import/export under the hood, so probably we should update it as well for consistency?

  • WDYT @jportner? Would like to get your two cents before I start updating everybody's favorite security & spaces integration tests :trollface:

Yes, if you decide to go down this route, we should update the CTS routes accordingly 👍

@lukeelmers
Copy link
Member

Based on our team discussion today, we will go ahead and hold on making these changes pending the outcome of our discussions around handling multi-space support in SO import/export, as explained in #91615 (comment)

As there is a likelihood we will opt to create new import/export APIs and eventually deprecate and remove the existing ones, it wouldn't really make sense to invest the time to deprecate & change these field names if the whole API could end up being deprecated in the future.

I'll go ahead and mark this as stalled and close my in-progress PR. I'll leave the issue open for now, and once we have a clear path forward on the other work, we can close it once we are certain it is no longer necessary.

@rudolf
Copy link
Contributor Author

rudolf commented Jul 18, 2024

Closing as the cost for ourselves and customers to adopt the snake case params would just not be worth the small benefit that consistency brings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Feature:New Platform Feature:Saved Objects stalled Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
Status: Done (7.13)
Development

Successfully merging a pull request may close this issue.

5 participants