Fixes #38542 - Delay params for import_uploads action #11430

ofedoren · 2025-07-01T15:34:55Z

What are the changes introduced in this pull request?

This PR tries to remove pain points of touching database during initialization times. There are might be other places, so maintainers, please point me to them, so we can get rid of them.

Considerations taken when implementing this change?

We kinda have a bad habit of calling the DB during initialization time, which leads to weird and hard to debug errors. The errors may or may not appear, making them even uglier.

I've tried to find a way how to workaround it and it seems moving such code into after_initialize or to_prepare blocks doesn't always work. Rails doesn't guarantee when the DB is ready. We could establish the connection manually somewhere, but it seems like an even worse solution.

Given that, I think we need to treat each case differently depending on what's the root of the problem.

What are the testing steps for this pull request?

I don't think we cover params with tests, needs manual API docs verification.

Summary by Sourcery

Consolidate dynamic import parameter definitions for the import_uploads action into an Apipie concern loaded after initialization, and switch the content_type parameter to use a callable_enum for dynamically generated values.

Enhancements:

Extract the generic import_attributes parameters from the controller into a new DynamicParams::Repositories concern
Include the DynamicParams concern into the RepositoriesController via an after_initialize hook
Change the content_type parameter to use a callable_enum for on-the-fly enumeration of uploadable content types

sourcery-ai · 2025-07-01T15:35:00Z

Reviewer's Guide

Extracts dynamic repository import parameters from the controller into a dedicated Apipie DSL concern loaded on initialization, and modernizes the content_type parameter to use a callable enum for up‐to‐date labels.

File-Level Changes

Change	Details	Files
Modernize content_type parameter to use callable enum	Replaced static list of labels with :callable_enum wrapping a lambda Kept the same label source (RepositoryTypeManager.uploadable_content_types) Retained requirement as optional and existing description	`app/controllers/katello/api/v2/repositories_controller.rb`
Refactor import_uploads dynamic parameters into an Apipie concern	Created DynamicParams::Repositories concern with update_api(:import_uploads) block Moved generic import_attributes loop from controller into the new concern Registered the concern in an initializer via after_initialize include	`app/controllers/katello/concerns/api/v2/dynamic_params/repositories.rb` `config/initializers/dynamic_params.rb` `app/controllers/katello/api/v2/repositories_controller.rb`

Tips and commands

Interacting with Sourcery

Trigger a new review: Comment @sourcery-ai review on the pull request.
Continue discussions: Reply directly to Sourcery's review comments.
Generate a GitHub issue from a review comment: Ask Sourcery to create an
issue from a review comment by replying to it. You can also reply to a
review comment with @sourcery-ai issue to create an issue from it.
Generate a pull request title: Write @sourcery-ai anywhere in the pull
request title to generate a title at any time. You can also comment
@sourcery-ai title on the pull request to (re-)generate the title at any time.
Generate a pull request summary: Write @sourcery-ai summary anywhere in
the pull request body to generate a PR summary at any time exactly where you
want it. You can also comment @sourcery-ai summary on the pull request to
(re-)generate the summary at any time.
Generate reviewer's guide: Comment @sourcery-ai guide on the pull
request to (re-)generate the reviewer's guide at any time.
Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
pull request to resolve all Sourcery comments. Useful if you've already
addressed all the comments and don't want to see them anymore.
Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
request to dismiss all existing Sourcery reviews. Especially useful if you
want to start fresh with a new review - don't forget to comment
@sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

Enable or disable review features such as the Sourcery-generated pull request
summary, the reviewer's guide, and others.
Change the review language.
Add, remove or edit custom review instructions.
Adjust other review settings.

Getting Help

Contact our support team for questions or feedback.
Visit our documentation for detailed guides and information.
Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

sourcery-ai

Hey @ofedoren - I've reviewed your changes and they look great!

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

ofedoren · 2025-07-03T14:50:07Z

app/controllers/katello/api/v2/repositories_controller.rb

-    repo_wrap_params = RootRepository.attribute_names + generic_repo_wrap_params
-
-    wrap_parameters :repository, :include => repo_wrap_params


These are not necessary, but seems nicer to delegate atribute_names call to Rails instead of calling it explicitly here. I'd love to get rid of calling database related stuff during initialization time.

ofedoren · 2025-07-03T14:51:06Z

app/controllers/katello/api/v2/repositories_controller.rb

@@ -493,7 +491,7 @@ def upload_content
    param :async, :bool, desc: N_("Do not wait for the ImportUpload action to finish. Default: false")
    param 'publish_repository', :bool, :desc => N_("Whether or not to regenerate the repository on disk. Default: true")
    param 'sync_capsule', :bool, :desc => N_("Whether or not to sync an external capsule after upload. Default: true")
-    param :content_type, RepositoryTypeManager.uploadable_content_types(false).map(&:label), :required => false, :desc => N_("content type ('deb', 'docker_manifest', 'file', 'ostree_ref', 'rpm', 'srpm')")


We introduced callable_enum in Foreman some time ago just for such cases: theforeman/foreman#10494

ofedoren · 2025-07-03T14:52:04Z

config/initializers/dynamic_params.rb

@@ -0,0 +1,3 @@
+Rails.application.config.after_initialize do
+  Katello::Api::V2::RepositoriesController.include Katello::Concerns::Api::V2::DynamicParams::Repositories


This can be done now it different places, e.g. in to_prepare block in engine.rb. This is just for readability? and a default place for similar cases.

ofedoren · 2025-07-03T14:57:17Z

app/controllers/katello/concerns/api/v2/dynamic_params/repositories.rb

+        module Repositories
+          extend ::Apipie::DSL::Concern
+
+          lazy_update_api(:import_uploads) do


lazy_update_api doesn't exist in apipie-rails yet, the PR is open: Apipie/apipie-rails#955

ekohl · 2025-07-03T15:09:40Z

app/controllers/katello/concerns/api/v2/dynamic_params/repositories.rb

+            ::Katello::RepositoryTypeManager.generic_repository_types.each_pair do |_, repo_type|
+              repo_type.import_attributes.each do |import_attribute|
+                param import_attribute.api_param, import_attribute.type,
+                    :desc => N_(import_attribute.description)


This was already there, but N_() doesn't work with variables so

Suggested change

:desc => N_(import_attribute.description)

:desc => import_attribute.description

ekohl · 2025-07-03T15:11:14Z

app/controllers/katello/api/v2/repositories_controller.rb

-        param import_attribute.api_param, import_attribute.type,
-            :desc => N_(import_attribute.description)
-      end
-    end


I'm thinking out loud now. Would it be possible to extend the DSL with a method so you still keep the API definition in one place? For example lazy_api (or better naming):

Suggested change

end

lazy_api do

Katello::RepositoryTypeManager.generic_repository_types.each_value do |repo_type|

repo_type.import_attributes.each do |import_attribute|

param import_attribute.api_param, import_attribute.type, desc: import_attribute.description

end

end

end

I thought about that, but this would introduce a new pattern (not that this is bad, but this new method will be available for every controller which use apipie mixin potentially introducing more conflicts).

Current PR to apipie-rails rather extends existing pattern. Personally I don't care, but it's up to maintainers/reviewers/users to decide.

pr-processor bot added the Not yet reviewed label Jul 1, 2025

ofedoren mentioned this pull request Jul 1, 2025

Patch Katello controllers to enable access by smart proxy theforeman/foreman_rh_cloud#1003

Merged

sourcery-ai bot reviewed Jul 1, 2025

View reviewed changes

pr-processor bot removed the Not yet reviewed label Jul 1, 2025

ofedoren force-pushed the ref-38542-delay-params-eval branch from c3cc7e5 to 654dbca Compare July 2, 2025 10:06

Fixes #38542 - Delay params for import_uploads action

497dc04

ofedoren force-pushed the ref-38542-delay-params-eval branch from 654dbca to 497dc04 Compare July 3, 2025 14:45

ofedoren mentioned this pull request Jul 3, 2025

Add lazy_update_api support Apipie/apipie-rails#955

Open

ofedoren commented Jul 3, 2025

View reviewed changes

ekohl reviewed Jul 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fixes #38542 - Delay params for import_uploads action #11430

Fixes #38542 - Delay params for import_uploads action #11430

Uh oh!

ofedoren commented Jul 1, 2025 •

edited

Loading

Uh oh!

sourcery-ai bot commented Jul 1, 2025 •

edited

Loading

Interacting with Sourcery

Customizing Your Experience

Getting Help

Uh oh!

sourcery-ai bot left a comment

Uh oh!

ofedoren Jul 3, 2025

Uh oh!

ofedoren Jul 3, 2025

Uh oh!

ofedoren Jul 3, 2025

Uh oh!

ofedoren Jul 3, 2025

Uh oh!

ekohl Jul 3, 2025

Uh oh!

ekohl Jul 3, 2025

Uh oh!

ofedoren Jul 3, 2025

Uh oh!

Uh oh!

		repo_wrap_params = RootRepository.attribute_names + generic_repo_wrap_params

		wrap_parameters :repository, :include => repo_wrap_params

		@@ -0,0 +1,3 @@
		Rails.application.config.after_initialize do
		Katello::Api::V2::RepositoriesController.include Katello::Concerns::Api::V2::DynamicParams::Repositories

	:desc => N_(import_attribute.description)
	:desc => import_attribute.description

-    end
+    lazy_api do
+      Katello::RepositoryTypeManager.generic_repository_types.each_value do |repo_type|
+        repo_type.import_attributes.each do |import_attribute|
+          param import_attribute.api_param, import_attribute.type, desc: import_attribute.description
+        end
+      end
+    end

Fixes #38542 - Delay params for import_uploads action #11430

Are you sure you want to change the base?

Fixes #38542 - Delay params for import_uploads action #11430

Uh oh!

Conversation

ofedoren commented Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What are the changes introduced in this pull request?

Considerations taken when implementing this change?

What are the testing steps for this pull request?

Summary by Sourcery

Uh oh!

sourcery-ai bot commented Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewer's Guide

File-Level Changes

Interacting with Sourcery

Customizing Your Experience

Getting Help

Uh oh!

sourcery-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

ofedoren Jul 3, 2025

Choose a reason for hiding this comment

Uh oh!

ofedoren Jul 3, 2025

Choose a reason for hiding this comment

Uh oh!

ofedoren Jul 3, 2025

Choose a reason for hiding this comment

Uh oh!

ofedoren Jul 3, 2025

Choose a reason for hiding this comment

Uh oh!

ekohl Jul 3, 2025

Choose a reason for hiding this comment

Uh oh!

ekohl Jul 3, 2025

Choose a reason for hiding this comment

Uh oh!

ofedoren Jul 3, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ofedoren commented Jul 1, 2025 •

edited

Loading

sourcery-ai bot commented Jul 1, 2025 •

edited

Loading