Skip to content

Fixes #39373 - Display API validation errors for host updates#1032

Closed
chris1984 wants to merge 1 commit into
Katello:mainfrom
chris1984:update-exceptionhandler
Closed

Fixes #39373 - Display API validation errors for host updates#1032
chris1984 wants to merge 1 commit into
Katello:mainfrom
chris1984:update-exceptionhandler

Conversation

@chris1984
Copy link
Copy Markdown
Member

@chris1984 chris1984 commented May 28, 2026

When updating a host with invalid content view environments, the API returns detailed validation errors in the 'displayMessage' field, but hammer was only showing "Could not update the host" without details.

Root cause:

  • RestClient::UnprocessableEntity (422 status) wasn't mapped to handle_unprocessable_entity in the exception handler
  • The existing handle_unprocessable_entity method extracts displayMessage but was never being called

This commit adds the missing exception mapping, so users see:
Could not update the host:
Validation failed: Host example.com: Cannot add content view
environment to content facet. The host's content source 'proxy.com'
does not sync lifecycle environment 'Library'.

Instead of just:
Could not update the host

Before patch:

hammer -d host update --id 3 --content-view-environments Library/Zoo
Could not update the host.

After patch:

hammer host update --id 3 --content-view-environments Library/Zoo
Could not update the host:
Validation failed: Host host.example.com: Cannot add content view environment to content facet. The host's content source 'capsule.example.com' does not sync lifecycle environment 'Library'.

Requires: theforeman/hammer-cli-foreman#655

Testing Steps:

  1. Create multiple Content Views (e.g., cv_1 & cv_2) and multiple Lifecycle Environments (e.g., 'Library' and 'Dev').

  2. Publish new versions of all Content Views and promote them to the 'Dev' lifecycle environment.

  3. Add only the 'Dev' lifecycle environment to the external Capsule, ensuring that the 'Library' lifecycle environment is not added. Then, synchronize the Capsule server.

  4. Register the client to the Capsule server.

  5. Try to update the hosts content view environments to one not synced on the capsule
    hammer -d host update --id 3 --content-view-environments Library/Zoo

  6. Verify you see the real error and not the generic can't update host error

When updating a host with invalid content view environments, the API
returns detailed validation errors in the 'displayMessage' field, but
hammer was only showing "Could not update the host" without details.

Root cause:
- RestClient::UnprocessableEntity (422 status) wasn't mapped to
  handle_unprocessable_entity in the exception handler
- The existing handle_unprocessable_entity method extracts
  displayMessage but was never being called

This commit adds the missing exception mapping so users see:
  Could not update the host:
    Validation failed: Host example.com: Cannot add content view
    environment to content facet. The host's content source 'proxy.com'
    does not sync lifecycle environment 'Library'.

Instead of just:
  Could not update the host

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

# RestClient exceptions expect the response to be accessible as a string
exception = RestClient::UnprocessableEntity.new
exception.instance_variable_set(:@response, error_response_body)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Correct me if this isn't the case - but I can see some of claude's bad tendencies coming through here. It likes to use methods like send , instance_variable_set, define_singleton_method, etc. which indicate that this is not being tested in the proper way.

I see that the ExceptionHandler superclass has a method called handle_exception which takes an exception as an argument. That's probably the way to go.

https://github.com/theforeman/hammer-cli/blob/1ff11f998b5f09068749259f9b0694285c25bc74/lib/hammer_cli/exception_handler.rb#L27

}.to_json

# RestClient exceptions expect the response to be accessible as a string
exception = RestClient::UnprocessableEntity.new
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does the constructor for this method take a response in its parameters? That'd be a nicer way to go about it

exception.instance_variable_set(:@response, error_response_body)

# Capture the output
output = StringIO.new
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Counterpoint to this test and the one below which are testing the output: your PR didn't change anything with respect to the output rendering. That's part of hammer-cli itself, so I think these tests can be removed as they are outside of the realm of this gem.

If you want to have a test around the displayMessage it should go in your related foreman PR

@jturel
Copy link
Copy Markdown
Member

jturel commented May 29, 2026

I see hammer-cli is already handling a few RestClient exceptions. Perhaps it'd make more sense to move this entire change there?

https://github.com/theforeman/hammer-cli/blob/1ff11f998b5f09068749259f9b0694285c25bc74/lib/hammer_cli/exception_handler.rb#L17-L19

@chris1984
Copy link
Copy Markdown
Member Author

Closing as this is not needed anymore since the change in hammer-cli-foreman is enough. Adding a test over there:

Summary from Claude:
we didn't need to touch hammer-cli at all. The reviewer was pointing out that RestClient exception handling already exists across the stack, and the question was where the fix best fits.

Looking at the chain:

  • hammer-cli already maps ResourceNotFound, Unauthorized, SSLCertificateNotVerified
  • hammer-cli-foreman already maps UnprocessableEntity and has the handle_unprocessable_entity method - it just wasn't checking the displayMessage field

So the one-line fix in hammer-cli-foreman was the right place. The handler was already there and wired up, it just needed to look at the right key in the response. No new mappings, no new handlers, no changes needed in hammer-cli or hammer-cli-katello.

@chris1984 chris1984 closed this May 29, 2026
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.

2 participants