Skip to content

Fixes #38653 - Fix settings show endpoint #10644

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

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

arvind4501
Copy link

@arvind4501 arvind4501 commented Aug 12, 2025

  • There was issue related to the api, in settings when we try to find a setting by id and if there is no setting with that id, it gives nil(which causes issue on the output) rather than raising exception.

hammer settings info --id 1
Before:-

output:- undefined method `name' for nil:NilClass

After Fix:-

output:- Resource setting not found by id '1'

Copy link
Contributor

@stejskalleos stejskalleos left a comment

Choose a reason for hiding this comment

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

Can we also add tests?

Copy link
Contributor

@stejskalleos stejskalleos left a comment

Choose a reason for hiding this comment

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

🍏 LGTM

Before

$ curl -su admin:changeme "https://foreman.local.lan/api/settings/666"                                                                              
	{
  "error": {"message":"undefined method `name' for nil:NilClass"}
}

After

~  $ curl -su admin:changeme "https://foreman.local.lan/api/settings/666"
{
  "error": {"message":"Resource setting not found by id '666'"}
}

@arvind4501 please squash the commits, after that we should have green CI and I'll merge it.

- There was issue related to the api, in settings when we try to find a
setting by id and if there is no setting with that id, it gives nil
rather than raising exception.

hammer settings info --id 1
Before:-

output:- undefined method `name' for nil:NilClass

After Fix:-

output:- Resource setting not found by id '1'
@arvind4501
Copy link
Author

Hey @stejskalleos , i have squashed the commits and updated branch with latest develop. thanks

@nacoool
Copy link

nacoool commented Aug 18, 2025

Working as expected, QE ACK

@ShimShtein
Copy link
Member

Before I merge it, what was the original call stack of the failure? I see that by default we should go to

raise ActiveRecord::RecordNotFound if scope.empty?
, so it makes me wonder why do I need to override the default behavior.

@arvind4501
Copy link
Author

Before I merge it, what was the original call stack of the failure? I see that by default we should go to

raise ActiveRecord::RecordNotFound if scope.empty?

, so it makes me wonder why do I need to override the default behavior.

Hey @ShimShtein , raise ActiveRecord::RecordNotFound if scope.empty? would not raise the error because the scope is not empty. we have scope defined in the api/v2/settings_controller.rb as
def resource_scope(...) Foreman.settings end

so the find_resource method in find_common.rb returns nil which causes issue in the api/v2/settings/show.json.rabl
and here is the trace of call stack

16:37:22 rails.1   | 2025-08-18T16:37:22 [I|app|49d84dd7] Processing by Api::V2::SettingsController#show as JSON
16:37:22 rails.1   | 2025-08-18T16:37:22 [I|app|49d84dd7]   Parameters: {"apiv"=>"v2", "id"=>"1", "setting"=>{}}
16:37:23 rails.1   | 2025-08-18T16:37:23 [D|app|49d84dd7] Authenticated user admin against INTERNAL authentication source
16:37:23 rails.1   | 2025-08-18T16:37:23 [D|app|49d84dd7] Post-login processing for admin
16:37:23 rails.1   | 2025-08-18T16:37:23 [I|app|49d84dd7] Authorized user admin(Admin User)
16:37:23 rails.1   | 2025-08-18T16:37:23 [D|app|49d84dd7] Post-login processing for admin
16:37:23 rails.1   | 2025-08-18T16:37:23 [D|tax|49d84dd7] Current location set to none
16:37:23 rails.1   | 2025-08-18T16:37:23 [D|tax|49d84dd7] Current organization set to none
16:37:23 rails.1   | 2025-08-18T16:37:23 [D|tax|49d84dd7] Current location set to none
16:37:23 rails.1   | 2025-08-18T16:37:23 [D|tax|49d84dd7] Current organization set to none
16:37:23 rails.1   | 2025-08-18T16:37:23 [D|app|49d84dd7]   Rendering api/v2/settings/show.json.rabl
16:37:23 rails.1   | 2025-08-18T16:37:23 [I|app|49d84dd7]   Rendered api/v2/settings/show.json.rabl (Duration: 9.5ms | Allocations: 902)
16:37:23 rails.1   | 2025-08-18T16:37:23 [W|app|49d84dd7] Action failed
16:37:23 rails.1   | 2025-08-18T16:37:23 [I|app|49d84dd7] Backtrace for 'Action failed' error (ActionView::Template::Error): undefined method `name' for nil:NilClass
16:37:23 rails.1   |  49d84dd7 | (eval):2:in `cached_source_3598127875320106324'
16:37:23 rails.1   |  49d84dd7 | /home/ajangir/.rbenv/versions/3.0.2/gemsets/foreman/gems/rabl-0.17.0/lib/rabl/engine.rb:443:in `eval_source'
16:37:23 rails.1   |  49d84dd7 | /home/ajangir/.rbenv/versions/3.0.2/gemsets/foreman/gems/rabl-0.17.0/lib/rabl/engine.rb:38:in `apply'
16:37:23 rails.1   |  49d84dd7 | /home/ajangir/apps/foreman/app/views/api/v2/settings/show.json.rabl:2:in `_app_views_api_v__settings_show_json_rabl__4205319975101823973_341360'
16:37:23 rails.1   |  49d84dd7 | /home/ajangir/.rbenv/versions/3.0.2/gemsets/foreman/gems/actionview-7.0.8.7/lib/action_view/base.rb:244:in `public_send'
16:37:23 rails.1   |  49d84dd7 | /home/ajangir/.rbenv/versions/3.0.2/gemsets/foreman/gems/actionview-7.0.8.7/lib/action_view/base.rb:244:in `_run'
16:37:23 rails.1   |  49d84dd7 | /home/ajangir/.rbenv/versions/3.0.2/gemsets/foreman/gems/actionview-7.0.8.7/lib/action_view/template.rb:157:in `block in render'
16:37:23 rails.1   |  49d84dd7 | /home/ajangir/.rbenv/versions/3.0.2/gemsets/foreman/gems/activesupport-7.0.8.7/lib/active_support/notifications.rb:208:in `instrument'
16:37:23 rails.1   |  49d84dd7 | /home/ajangir/.rbenv/versions/3.0.2/gemsets/foreman/gems/actionview-7.0.8.7/lib/action_view/template.rb:361:in `instrument_render_template'
16:37:23 rails.1   |  49d84dd7 | /home/ajangir/.rbenv/versions/3.0.2/gemsets/foreman/gems/actionview-7.0.8.7/lib/action_view/template.rb:155:in `render'
16:37:23 rails.1   |  49d84dd7 | /home/ajangir/.rbenv/versions/3.0.2/gemsets/foreman/gems/actionview-7.0.8.7/lib/action_view/renderer/template_renderer.rb:65:in `block (2 levels) in render_template'
16:37:23 rails.1   |  49d84dd7 | /home/ajangir/.rbenv/versions/3.0.2/gemsets/foreman/gems/activesupport-7.0.8.7/lib/active_support/notifications.rb:206:in `block in instrument'
16:37:23 rails.1   |  49d84dd7 | /home/ajangir/.rbenv/versions/3.0.2/gemsets/foreman/gems/activesupport-7.0.8.7/lib/active_support/notifications/instrumenter.rb:24:in ```


Thanks

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.

4 participants