manageiq-api icon indicating copy to clipboard operation
manageiq-api copied to clipboard

Server error (500) returned after editing Catalog item with used name

Open rvsia opened this issue 6 years ago • 10 comments

Services > Catalogs > Catalogs > Configuration > New/Edit

Description

  • When you add a new Catalog with used name it returns 400 error
  • However when you edit some Catalog and change its name to used name it returns 500 error

http://localhost:3000/api/service_catalogs/45

body

{
  "action": "edit",
  "resource": {
    "name": "Aa" // something what is already used
  }
}

response

500 Interval Server Error

{
"error": {
   "kind": "internal_server_error",
    "message": "Validation failed: ServiceTemplateCatalog: Name has already been taken",
    "klass": "ActiveRecord::RecordInvalid"
    }
}

rvsia avatar Apr 08 '19 15:04 rvsia

API Request:    {:requested_at=>"2019-04-12 14:07:20 UTC", :method=>"POST", :url=>"http://localhost:3000/api/service_catalogs/1"}
Authentication: {:type=>"basic", :token=>nil, :x_miq_group=>nil, :user=>"admin"}
Authorization:  {:user=>"admin", :group=>"EvmGroup-super_administrator", :role=>"EvmRole-super_administrator", :tenant=>"My Company"}
Request:        {:method=>:post, :action=>"edit", :fullpath=>"/api/service_catalogs/1", :url=>"http://localhost:3000/api/service_catalogs/1", :base=>"http://localhost:3000", :path=>"/api/service_catalogs/1", :prefix=>"/api", :version=>"4.1.0-pre", :api_prefix=>"http://localhost:3000/api", :collection=>"service_catalogs", :c_suffix=>nil, :collection_id=>"1", :subcollection=>nil, :subcollection_id=>nil}
Parameters:     {"action"=>"update", "controller"=>"api/service_catalogs", "format"=>"json", "body"=>{"action"=>"edit", "resource"=>{"name"=>"Single Service"}}}

ActiveRecord::RecordInvalid: Validation failed: ServiceTemplateCatalog: Name has already been taken

activerecord-5.0.7.2/lib/active_record/validations.rb:78:in `raise_validation_error'
activerecord-5.0.7.2/lib/active_record/validations.rb:50:in `save!'
activerecord-5.0.7.2/lib/active_record/attribute_methods/dirty.rb:30:in `save!'
activerecord-5.0.7.2/lib/active_record/transactions.rb:324:in `block in save!'
activerecord-5.0.7.2/lib/active_record/transactions.rb:395:in `block in with_transaction_returning_status'
activerecord-5.0.7.2/lib/active_record/connection_adapters/abstract/database_statements.rb:230:in `transaction'
activerecord-5.0.7.2/lib/active_record/transactions.rb:211:in `transaction'
activerecord-5.0.7.2/lib/active_record/transactions.rb:392:in `with_transaction_returning_status'
activerecord-5.0.7.2/lib/active_record/transactions.rb:324:in `save!'
activerecord-5.0.7.2/lib/active_record/suppressor.rb:45:in `save!'
activerecord-5.0.7.2/lib/active_record/persistence.rb:288:in `block in update!'
activerecord-5.0.7.2/lib/active_record/transactions.rb:395:in `block in with_transaction_returning_status'
activerecord-5.0.7.2/lib/active_record/connection_adapters/abstract/database_statements.rb:232:in `block in transaction'
activerecord-5.0.7.2/lib/active_record/connection_adapters/abstract/transaction.rb:189:in `within_new_transaction'
activerecord-5.0.7.2/lib/active_record/connection_adapters/abstract/database_statements.rb:232:in `transaction'
activerecord-5.0.7.2/lib/active_record/transactions.rb:211:in `transaction'
activerecord-5.0.7.2/lib/active_record/transactions.rb:392:in `with_transaction_returning_status'
activerecord-5.0.7.2/lib/active_record/persistence.rb:286:in `update!'
manageiq-api-9240a6cb1e59/app/controllers/api/base_controller/generic.rb:53:in `edit_resource'
manageiq-api-9240a6cb1e59/app/controllers/api/base_controller/manager.rb:105:in `update_one_collection'
manageiq-api-9240a6cb1e59/app/controllers/api/base_controller/manager.rb:92:in `get_and_update_one_collection'
manageiq-api-9240a6cb1e59/app/controllers/api/base_controller/manager.rb:15:in `update_collection'
manageiq-api-9240a6cb1e59/app/controllers/api/base_controller.rb:94:in `update'
actionpack-5.0.7.2/lib/action_controller/metal/basic_implicit_render.rb:4:in `send_action'
actionpack-5.0.7.2/lib/abstract_controller/base.rb:188:in `process_action'
actionpack-5.0.7.2/lib/action_controller/metal/rendering.rb:30:in `process_action'
actionpack-5.0.7.2/lib/abstract_controller/callbacks.rb:20:in `block in process_action'
activesupport-5.0.7.2/lib/active_support/callbacks.rb:126:in `call'
activesupport-5.0.7.2/lib/active_support/callbacks.rb:506:in `block (2 levels) in compile'
activesupport-5.0.7.2/lib/active_support/callbacks.rb:455:in `call'
activesupport-5.0.7.2/lib/active_support/callbacks.rb:101:in `__run_callbacks__'
activesupport-5.0.7.2/lib/active_support/callbacks.rb:750:in `_run_process_action_callbacks'
activesupport-5.0.7.2/lib/active_support/callbacks.rb:90:in `run_callbacks'
actionpack-5.0.7.2/lib/abstract_controller/callbacks.rb:19:in `process_action'
actionpack-5.0.7.2/lib/action_controller/metal/rescue.rb:20:in `process_action'
actionpack-5.0.7.2/lib/action_controller/metal/instrumentation.rb:32:in `block in process_action'
activesupport-5.0.7.2/lib/active_support/notifications.rb:164:in `block in instrument'
activesupport-5.0.7.2/lib/active_support/notifications/instrumenter.rb:21:in `instrument'
activesupport-5.0.7.2/lib/active_support/notifications.rb:164:in `instrument'
actionpack-5.0.7.2/lib/action_controller/metal/instrumentation.rb:30:in `process_action'
actionpack-5.0.7.2/lib/action_controller/metal/params_wrapper.rb:248:in `process_action'
activerecord-5.0.7.2/lib/active_record/railties/controller_runtime.rb:18:in `process_action'
actionpack-5.0.7.2/lib/abstract_controller/base.rb:126:in `process'
actionpack-5.0.7.2/lib/action_controller/metal.rb:190:in `dispatch'
actionpack-5.0.7.2/lib/action_controller/metal.rb:262:in `dispatch'
actionpack-5.0.7.2/lib/action_dispatch/routing/route_set.rb:50:in `dispatch'
actionpack-5.0.7.2/lib/action_dispatch/routing/route_set.rb:32:in `serve'
actionpack-5.0.7.2/lib/action_dispatch/journey/router.rb:39:in `block in serve'
actionpack-5.0.7.2/lib/action_dispatch/journey/router.rb:26:in `each'
actionpack-5.0.7.2/lib/action_dispatch/journey/router.rb:26:in `serve'
actionpack-5.0.7.2/lib/action_dispatch/routing/route_set.rb:727:in `call'
manageiq-graphql-38bd898e7eb3/lib/manageiq/graphql/rest_api_proxy.rb:18:in `call'
secure_headers-3.0.3/lib/secure_headers/middleware.rb:10:in `call'
manageiq/lib/request_started_on_middleware.rb:12:in `call'
rack-2.0.7/lib/rack/etag.rb:25:in `call'
rack-2.0.7/lib/rack/conditional_get.rb:38:in `call'
rack-2.0.7/lib/rack/head.rb:12:in `call'
rack-2.0.7/lib/rack/session/abstract/id.rb:232:in `context'
rack-2.0.7/lib/rack/session/abstract/id.rb:226:in `call'
actionpack-5.0.7.2/lib/action_dispatch/middleware/cookies.rb:613:in `call'
actionpack-5.0.7.2/lib/action_dispatch/middleware/callbacks.rb:38:in `block in call'
activesupport-5.0.7.2/lib/active_support/callbacks.rb:97:in `__run_callbacks__'
activesupport-5.0.7.2/lib/active_support/callbacks.rb:750:in `_run_call_callbacks'
activesupport-5.0.7.2/lib/active_support/callbacks.rb:90:in `run_callbacks'
actionpack-5.0.7.2/lib/action_dispatch/middleware/callbacks.rb:36:in `call'
actionpack-5.0.7.2/lib/action_dispatch/middleware/executor.rb:12:in `call'
actionpack-5.0.7.2/lib/action_dispatch/middleware/remote_ip.rb:79:in `call'
actionpack-5.0.7.2/lib/action_dispatch/middleware/debug_exceptions.rb:49:in `call'
actionpack-5.0.7.2/lib/action_dispatch/middleware/show_exceptions.rb:31:in `call'
railties-5.0.7.2/lib/rails/rack/logger.rb:36:in `call_app'
railties-5.0.7.2/lib/rails/rack/logger.rb:26:in `call'
sprockets-rails-3.2.1/lib/sprockets/rails/quiet_assets.rb:13:in `call'
actionpack-5.0.7.2/lib/action_dispatch/middleware/request_id.rb:24:in `call'
rack-2.0.7/lib/rack/method_override.rb:22:in `call'
rack-2.0.7/lib/rack/runtime.rb:22:in `call'
activesupport-5.0.7.2/lib/active_support/cache/strategy/local_cache_middleware.rb:28:in `call'
actionpack-5.0.7.2/lib/action_dispatch/middleware/executor.rb:12:in `call'
actionpack-5.0.7.2/lib/action_dispatch/middleware/static.rb:136:in `call'
rack-2.0.7/lib/rack/sendfile.rb:111:in `call'
railties-5.0.7.2/lib/rails/engine.rb:522:in `call'
puma-3.7.1/lib/puma/configuration.rb:232:in `call'
puma-3.7.1/lib/puma/server.rb:578:in `handle_request'
puma-3.7.1/lib/puma/server.rb:415:in `process_client'
puma-3.7.1/lib/puma/server.rb:275:in `block in run'
puma-3.7.1/lib/puma/thread_pool.rb:120:in `block in spawn_thread'

Response:       {:completed_at=>"2019-04-12 14:07:20 UTC", :size=>"0.163 KBytes", :time_taken=>"0.144 Seconds", :status=>500}

(from api.log)

himdel avatar Apr 12 '19 14:04 himdel

This issue has been automatically marked as stale because it has not been updated for at least 6 months.

If you can still reproduce this issue on the current release or on master, please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions!

miq-bot avatar Oct 14 '19 04:10 miq-bot

was not able to recreate issue in UI, seems like this issue was addressed in UI with form conversion to react in https://github.com/ManageIQ/manageiq-ui-classic/pull/5139

@martinpovolny do we need to fix something on API side for this?

h-kataria avatar Dec 06 '19 22:12 h-kataria

@miq-bot assign @martinpovolny

h-kataria avatar Dec 06 '19 22:12 h-kataria

This is consistent with other places. For example:

lucifer ruby-2.5.5 - [~/Projects/manageiq-api] (master)$ echo '{ "action": "edit", "resource": { "name": "Loic" } }' | api post roles/20

{
  "error": {
    "kind": "internal_server_error",
    "message": "Validation failed: MiqUserRole: Name has already been taken",
    "klass": "ActiveRecord::RecordInvalid"
  }
}

We probably should have a better generic way of handling this.

I am not sure, but maybe the right HTTP code to return would be "422 Unprocessable Entity".

Also the validation error should probably be better structured so that the UI can present it to the user and possibly a translation in the current locale shoud be returned.

Thoughts on this one? @rvsia, @himdel, @abellotti ?

martinpovolny avatar Dec 12 '19 09:12 martinpovolny

:+1:, but looks like we're using "400 Bad Request" in other places.

As for i18n vs the error message, maybe we should use a similar mechanism as notifications (miqFormatNotification) - serve a translatable format string and the parameters separately? (Not sure if the API should be able to tell the current language.)

himdel avatar Dec 12 '19 10:12 himdel

This issue has been automatically marked as stale because it has not been updated for at least 3 months.

If you can still reproduce this issue on the current release or on master, please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions! More information about the ManageIQ triage process can be found in the traige process documentation.

miq-bot avatar Jun 11 '20 22:06 miq-bot

@abellotti Can you verify?

Fryguy avatar Jul 06 '20 18:07 Fryguy

This is still an issue, we should handle these generically and return a 400 like we do with other ActiveRecord exceptions. I'll create a PR.

abellotti avatar Jul 07 '20 16:07 abellotti

This issue has been automatically marked as stale because it has not been updated for at least 3 months.

If you can still reproduce this issue on the current release or on master, please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation.

miq-bot avatar Feb 27 '23 00:02 miq-bot