Skip to content

Follow-up from "Update DismissPolicyViolations mutation with dismissal_types and comment"

The following discussion from !204903 (merged) should be addressed:


Thread: Refactor group_violations_by_policy

@imam_h started a discussion: (+2 comments)

Hi @Andyschoenen , could you please do an initial review of this MR? Thank you.
@sashi_kumar : Thanks! The changes in this MR LGTM 👍


Comment by @sashi_kumar
But I've a couple of concerns with group_violations_by_policy:

  • group_by_security_policy_id is currently a scope, but it is not composable. Should that be a class method?
  • group_by_security_policy_id preloads approval_policy_rules even though for_security_policies does a join. Maybe we can combine to avoid the extra query to load approval_policy_rules.

Maybe, we can create a single class method like:

def self.group_by_security_policy_id(security_policies)
  joins(:approval_policy_rule)
    .where(approval_policy_rule: { security_policy: security_policies })
    .includes(:approval_policy_rule)
    .group_by { |v| v.approval_policy_rule&.security_policy_id }
end

This will generate this query

SQL
    SELECT
        "scan_result_policy_violations"."id" AS t0_r0,
        "scan_result_policy_violations"."scan_result_policy_id" AS t0_r1,
        "scan_result_policy_violations"."merge_request_id" AS t0_r2,
        "scan_result_policy_violations"."project_id" AS t0_r3,
        "scan_result_policy_violations"."created_at" AS t0_r4,
        "scan_result_policy_violations"."updated_at" AS t0_r5,
        "scan_result_policy_violations"."violation_data" AS t0_r6,
        "scan_result_policy_violations"."approval_policy_rule_id" AS t0_r7,
        "scan_result_policy_violations"."status" AS t0_r8,
        "approval_policy_rule"."id" AS t1_r0,
        "approval_policy_rule"."security_policy_id" AS t1_r1,
        "approval_policy_rule"."created_at" AS t1_r2,
        "approval_policy_rule"."updated_at" AS t1_r3,
        "approval_policy_rule"."rule_index" AS t1_r4,
        "approval_policy_rule"."type" AS t1_r5,
        "approval_policy_rule"."content" AS t1_r6,
        "approval_policy_rule"."security_policy_management_project_id" AS t1_r7 
    FROM
        "scan_result_policy_violations" 
    INNER JOIN
        "approval_policy_rules" "approval_policy_rule" 
            ON "approval_policy_rule"."id" = "scan_result_policy_violations"."approval_policy_rule_id" 
    WHERE
        "scan_result_policy_violations"."merge_request_id" = 292 
        AND "approval_policy_rule"."security_policy_id" IN (
            SELECT
                "security_policies"."id" 
            FROM
                "security_policies" 
            WHERE
                "security_policies"."id" IN (
                    97, 98, 99, 100, 101, 102, 103, 104, 105, 111
                )
                AND (
                    content->>'enforcement_type' = 'warn'
                )
        )

Let me know your thoughts 🏓

Edited by 🤖 GitLab Bot 🤖