Dynamic data like database queries in apipie

ekohl · November 8, 2024, 11:50am

This is to gather some thoughts around this topic.

The background is that we use apipie to describe our REST API. It adds descriptions and (optional) validation.

In many places we use the EnumValidator. IMHO that validator is great because it also generates a nice description to the user of valid values. One common problem is that the data is actually dynamic where the EnumValidator will store a copy of the concrete list.

In practice this means the application starts up and loads the data. Concrete examples that recently popped up:

github.com

theforeman/foreman/blob/9d0f388296a845ebcba944e9fc04570a42f5ceea/app/controllers/api/v2/template_inputs_controller.rb#L40


      
              param :required, :bool, :allow_nil => true, :desc => N_('Input is required')
              param :advanced, :bool, :allow_nil => true, :desc => N_('Input is advanced')
              param :input_type, Foreman.input_types_registry.input_types.keys, :required => true, :desc => N_('Input type')
              Foreman.input_types_registry.input_types.each do |key, input_type_class|
                input_type_class.api_params_for_input_group(self)
              end
              param :options, Array, :required => false, :desc => N_('Selectable values for user inputs')
              param :default, String, :required => false, :desc => N_('Default value for user input')
              param :hidden_value, :bool, :required => false, :desc => N_('The value contains sensitive information and shouldn not be normally visible, useful e.g. for passwords')
              param :value_type, TemplateInput::VALUE_TYPE, :required => false, :desc => N_('Value type, defaults to plain')
              param :resource_type, Permission.resources, :required => false, :desc => N_('For values of type search, this is the resource the value searches in')
            end
          end
          
          api :POST, '/templates/:template_id/template_inputs/', N_('Create a template input')
          param :template_id, :identifier, :required => true
          param_group :template_input, :as => :create
          def create
            @template_input = resource_class.new(template_input_params.merge(:template_id => @nested_obj.id))
            process_response @template_input.save
          end

Here for the resource_type parameter it calls Permission.resources, which is this method:

github.com

theforeman/foreman/blob/9d0f388296a845ebcba944e9fc04570a42f5ceea/app/models/permission.rb#L14-L20


      
          def self.resources
            # We don't run seeds.rb initializer in test environment, so at the first call of this method in TemplateInputsController
            # during code loading @all_resources will be an empty array, and don't get populated with the actual resources from db later
            # In dev and prod environments, the seeds.rb initializer is run, so @all_resources will be populated with the actual resources
            @all_resources = Permission.distinct.order(:resource_type).pluck(:resource_type).compact if @all_resources.empty?
            @all_resources
          end

We saw the failure there when the test database wasn’t migrated (fixed), but it does mean that during class loading it already calls the database even if it might not need the controllers (such as a rake task). That can slow it down.

In this particular case the data is cached anyway, so if you would change the DB entries it doesn’t make a difference, but there’s another case where it should be dynamic:

github.com

Katello/katello/blob/da40587680c2e05d53752196d157da34653e475d/app/controllers/katello/api/v2/generic_content_units_controller.rb#L3-L6


      
          resource_description do
            name 'Content Units'
            param :content_type, String, desc: N_("Possible values: #{Katello::RepositoryTypeManager.generic_content_types.join(", ")}"), required: true
          end

Here we can actually see multiple issues. First of all, it’s marked for translation (N_()) but with a dynamic string. That can never work and should be N_("Possible values: %s") % Katello::RepositoryTypeManager.generic_content_types.join(", ") for it to have a chance, but I don’t think we can really make it translatable.

The more interesting part is that it displays the values, but those are loaded on application start up. It doesn’t perform any validation so if the content types change, it’ll still display the old value(s) while accepting the new value(s). You can question the usefulness of the help here.

That it runs on start up can actually trigger interesting failure modes. Bug #37977: Katello prevents foreman server startup when Pulp is not reachable - Katello - Foreman observes that the method call can result in the application reaching out to Pulp and Foreman refuses to start up when Pulp is unavailable.

So we can observe reliability, correctness and performance aren’t what they should be. Question is, what can we do?

I think EnumValidator today should only be used with true static lists, but perhaps we can enhance it to become dynamic. I opened Support passing a callable to EnumValidator by ekohl · Pull Request #946 · Apipie/apipie-rails · GitHub to do this, but failed to realize it then overlaps with ProcValidator.

So now I’m opening the topic to a wider audience: what should we do here?

Shimon_Shtein · November 12, 2024, 10:48am

I agree that access to external systems during class loading is a bad practice and we should avoid it.

I think we have two different use cases here:

Validate static metadata that is hard to obtain. e.g. it’s from the DB/Pulp
Validate a dynamic list of values

In both cases we can’t obtain the list during the class loading and we need to postpone it. I would suggest to postpone it to the latest possible stage - to the actual request for the apipie data. I think we can achieve both if we introduce a concept of examples to the ProcValidator - upon demand a method will be called that will return a list of items that will pass the validation. Since the method will be called on demand, it will be done quite late in the process - which is good for our cause. And since it’s only examples the system will not commit to this specific list, and will be able to change it depending on the actual object validation logic.

So for use case 1, we will get the list of possible values once on first request and cache it forever.
And for use case 2 we can retreive a list that will be correct to the specific point of time, but for the next request we will need to retreve it again.

How does this sound?

ekohl · November 12, 2024, 11:36am

I’m trying to think about the performance. If they’re expensive to retrieve then today it’s not an issue because the cost is paid once on start up. On the other hand, that slows down start up and also means it can end being invalid.

I wonder if we could somehow figure out how much time is spent in apipie loading and if we can easily identify the slow or dynamic parts. See how big of an issue this actually is.