Skip to content

Commit

Permalink
fix(APIv2): RHINENG-10379 introduce weak attributes for systems
Browse files Browse the repository at this point in the history
  • Loading branch information
skateman authored and romanblanco committed Jun 11, 2024
1 parent 05e2f95 commit f7b08b9
Show file tree
Hide file tree
Showing 6 changed files with 58 additions and 10 deletions.
4 changes: 4 additions & 0 deletions .rubocop.yml
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,10 @@ Metrics/ClassLength:
Exclude:
- 'test/**/*'
- 'spec/**/*'
CountAsOne:
- 'array'
- 'heredoc'
- 'method_call'

Metrics/ModuleLength:
Exclude:
Expand Down
15 changes: 13 additions & 2 deletions app/controllers/concerns/v2/resolver.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ def expand_resource
scope = join_parents(pundit_scope, permitted_params[:parents])
# Join with the additional 1:1 relationships required by the serializer, select only the
# dependencies that are really necessary for the rendering.
join_aggregated(join_associated(scope)).select(*select_fields)
join_aggregated(join_weak(join_associated(scope))).select(*select_fields)
end

# Reduce through all the associations of the `relation` and join+scope them or return the
Expand All @@ -38,6 +38,13 @@ def join_associated(relation)
relation.where.associated(*associations)
end

# Left-outer join with the optional (weak) dependencies required for computing weak attributes
def join_weak(relation)
weak_dependencies.keys.reduce(relation) do |scope, association|
already_joined(scope).include?(association) ? scope : scope.left_outer_joins(association)
end
end

# Self-join with the requested aggregations built using 1:n associations, also select
# the aggregated/evaluated/aliased fields from the self-joined subquery.
def join_aggregated(relation)
Expand Down Expand Up @@ -72,7 +79,7 @@ def subquery_fragment(association, aliases)
# so it is understood by SQL. Furthermore, alias any field that is coming from a joined
# table to avoid any possible hash key collision.
def select_fields
dependencies.flat_map do |(association, fields)|
dependencies.merge(weak_dependencies).flat_map do |(association, fields)|
fields.map do |field|
if association
"#{association}.#{field} AS #{association}__#{field}"
Expand All @@ -98,6 +105,10 @@ def aggregations
@aggregations ||= serializer.aggregations(permitted_params[:parents], resource.one_to_many)
end

def weak_dependencies
@weak_dependencies ||= serializer.weak_dependencies(permitted_params[:parents])
end

# List all the joined (direct and indirect) associations of a given scope
def already_joined(scope)
scope.try(:joins_values).to_a.flat_map do |association|
Expand Down
14 changes: 13 additions & 1 deletion app/models/v2/system.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ class System < ApplicationRecord
has_many :report_systems, class_name: 'V2::ReportSystem', dependent: nil
has_many :policies, class_name: 'V2::Policy', through: :policy_systems
has_many :reports, class_name: 'V2::Report', through: :report_systems
has_many :test_results, -> { where(arel_table[:report_id].eq(V2::Report.arel_table.alias(:reports)[:id])) },
has_many :v2_test_results, -> { where(arel_table[:report_id].eq(V2::Report.arel_table.alias(:reports)[:id])) },
class_name: 'V2::TestResult', dependent: :destroy, inverse_of: :system
has_many :rule_results, class_name: 'V2::RuleResult', through: :test_results

Expand Down Expand Up @@ -109,6 +109,18 @@ def os_minor_version
attributes['os_minor_version'] || try(:system_profile)&.dig('operating_system', 'minor')
end

def compliant
attributes['v2_test_results__score'].try(:>=, attributes['reports__compliance_threshold'])
end

def last_scanned
attributes['v2_test_results__end_time']
end

def failed_rule_count
attributes['v2_test_results__failed_rule_count']
end

def self.arel_inventory_groups(groups, key)
jsons = groups.map { |group| [{ key => group }].to_json.dump }

Expand Down
22 changes: 22 additions & 0 deletions app/serializers/v2/application_serializer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,14 @@ def aggregations(parents, to_many)
end
end

# Return the hash of dependencies for optional (weak) attributes in the same format as the `dependencies`
# excluding the `nil` key as own fields are never optional.
def weak_dependencies(parents)
@weak_attributes&.values.to_a.each_with_object({}) do |(deps, fields), hsh|
merge_dependencies(hsh, fields, false) if deps.all? { |dep| parents.try(:include?, dep) }
end
end

# Panko's default way of skipping certain attributes is to construct a hash that contains a list of fields
# that should be omitted. This method automatically receives a context and a scope argument, where we use
# the context to pass the `params[:joined]` that are necessary to determine if the required tables are joined.
Expand Down Expand Up @@ -62,6 +70,16 @@ def derived_attribute(name, *arr, **hsh)
@derived_attributes[name] = hsh.merge(nil => arr)
end

# This method allows the definition of loosely-coupled attributes that are derived from left-outer-joined
# models. This means that if there is nothing to join with, their final value would be `nil`.
def weak_attribute(name, *parents, **hsh)
attributes name
delegate name, to: :@object

@weak_attributes ||= {}
@weak_attributes[name] = [parents, hsh]
end

# This method allows the definition attributes that are aggregated from any left-outer-joined has_many
# `association`. The attribute is forwarded to an `aggregate_#{name}` method in the model that should
# exist as an attribute when calling the serializer. The result of the aggregation `function` should
Expand All @@ -80,15 +98,19 @@ def aggregated_attribute(name, association, function)
# Derived attributes should be excluded if the required associations are not joined with the current scope,
# i.e. there are no joined parents available. Aggregated attributes should be skipped when its dependencies
# have been already joined to the current scope, i.e. there is an overlap with parents.
# rubocop:disable Metrics/AbcSize
def to_be_excluded?(field, context)
@derived_attributes ||= {}
@aggregated_attributes ||= {}
@weak_attributes ||= {}

[
@derived_attributes.key?(field) && !meets_dependency?(@derived_attributes[field].keys, context[:joined]),
@weak_attributes.key?(field) && !meets_dependency?(@weak_attributes[field].first, context[:joined]),
@aggregated_attributes.key?(field) && meets_dependency?(@aggregated_attributes[field].keys, context[:joined])
].any?
end
# rubocop:enable Metrics/AbcSize

# Reduces the "method fields" of the serializer to an array using a passed block
def reduce_method_fields(initial, &block)
Expand Down
7 changes: 3 additions & 4 deletions app/serializers/v2/system_serializer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,9 @@ class SystemSerializer < V2::ApplicationSerializer
derived_attribute :os_major_version, V2::System::OS_MAJOR_VERSION
derived_attribute :os_minor_version, V2::System::OS_MINOR_VERSION

# TODO: these attributes do not work yet
derived_attribute :compliant, policy: [:compliance_threshold], test_result: [:score]
derived_attribute :last_scanned, test_result: [:end_time]
derived_attribute :failed_rule_count, test_result: [:failed_rule_count]
weak_attribute :compliant, :reports, reports: [:compliance_threshold], v2_test_results: [:score]
weak_attribute :last_scanned, :reports, v2_test_results: [:end_time]
weak_attribute :failed_rule_count, :reports, v2_test_results: [:failed_rule_count]

aggregated_attribute :policies, :policies, -> { V2::System::POLICIES }
end
Expand Down
6 changes: 3 additions & 3 deletions spec/controllers/v2/systems_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -610,9 +610,9 @@
insights_id: :insights_id,
tags: :tags,
policies: -> { policies.map { |policy| { id: policy.id, title: policy.title } } },
# compliant: :compliant,
# last_scanned: :last_scanned,
# failed_rule_count: :failed_rule_count,
compliant: :compliant,
last_scanned: :last_scanned,
failed_rule_count: :failed_rule_count,
os_major_version: -> { system_profile&.dig('operating_system', 'major') },
os_minor_version: -> { system_profile&.dig('operating_system', 'minor') }
}
Expand Down

0 comments on commit f7b08b9

Please sign in to comment.