Merge branch '396476_add_linter_for_codeowners' into 'master'

Add a basic linter for CODEOWNERS file

See merge request !117682



Merged-by: Mehmet Emin INAC <minac@gitlab.com>
Approved-by: Mehmet Emin INAC <minac@gitlab.com>
Approved-by: Joe Woodward <jwoodward@gitlab.com>
Reviewed-by: Vasilii Iakliushin <viakliushin@gitlab.com>
Reviewed-by: Mehmet Emin INAC <minac@gitlab.com>
Co-authored-by: Vasilii Iakliushin <viakliushin@gitlab.com>

ee/lib/gitlab/code_owners/error.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module CodeOwners
+    class Error
+      MISSING_ENTRY_OWNER = :missing_entry_owner
+      MISSING_SECTION_NAME = :missing_section_name
+      INVALID_APPROVAL_REQUIREMENT = :invalid_approval_requirement
+      INVALID_SECTION_FORMAT = :invalid_section_format
+
+      def initialize(message:, line_number:, path:)
+        @message = message
+        @line_number = line_number
+        @path = path
+      end
+
+      attr_reader :message, :line_number, :path
+
+      def ==(other)
+        return true if equal?(other)
+
+        message == other.message &&
+          line_number == other.line_number &&
+          path == other.path
+      end
+    end
+  end
+end

ee/lib/gitlab/code_owners/file.rb

@@ -11,8 +11,11 @@ class File
 
       def initialize(blob)
         @blob = blob
+        @errors = []
       end
 
+      attr_reader :errors
+
       def parsed_data
         @parsed_data ||= get_parsed_data
       end
@@ -68,6 +71,14 @@ def entries_for_path(path)
         matches
       end
 
+      def valid?
+        parsed_data
+
+        errors.none?
+      end
+
+      private
+
       def data
         return "" if @blob.nil? || @blob.binary?
 
@@ -80,31 +91,41 @@ def get_parsed_data
           current_section.name => {}
         }
 
-        data.lines.each do |line|
+        data.lines.each.with_index(1) do |line, line_number|
           line = line.strip
 
           next if skip?(line)
 
+          section_parser = SectionParser.new(line, parsed_sectional_data)
+          parsed_section = section_parser.execute
+
+          # Report errors even if the section is successfully parsed
+          unless section_parser.valid?
+            section_parser.errors.each { |error| add_error(error, line_number) }
+          end
+
           # Detect section headers and consider next lines in the file as part ot the section.
-          if (parsed_section = Section.parse(line, parsed_sectional_data))
+          if parsed_section
             current_section = parsed_section
             parsed_sectional_data[current_section.name] ||= {}
 
             next
           end
 
-          parse_entry(line, parsed_sectional_data, current_section)
+          parse_entry(line, parsed_sectional_data, current_section, line_number)
         end
 
         parsed_sectional_data
       end
 
-      def parse_entry(line, parsed, section)
+      def parse_entry(line, parsed, section, line_number)
         pattern, _separator, entry_owners = line.partition(/(?<!\\)\s+/)
         normalized_pattern = normalize_pattern(pattern)
 
         owners = entry_owners.presence || section.default_owners
 
+        add_error(Error::MISSING_ENTRY_OWNER, line_number) if owners.blank?
+
         parsed[section.name][normalized_pattern] = Entry.new(
           pattern,
           owners,
@@ -139,6 +160,10 @@ def normalize_pattern(pattern)
       def path_matches?(pattern, path)
         ::File.fnmatch?(pattern, path, FNMATCH_FLAGS)
       end
+
+      def add_error(message, line_number)
+        errors << Error.new(message: message, line_number: line_number, path: path)
+      end
     end
   end
 end

ee/lib/gitlab/code_owners/section.rb

@@ -5,35 +5,6 @@ module CodeOwners
     class Section
       DEFAULT = 'codeowners'
 
-      REGEX = {
-        optional: /(?<optional>\^)?/,
-        name: /\[(?<name>.*?)\]/,
-        approvals: /(?:\[(?<approvals>\d*?)\])?/,
-        default_owners: /(?<default_owners>\s+[@\w_.\-\/\s+]*)?/ # rubocop: disable Style/RegexpLiteralMixedPreserve
-      }.freeze
-
-      HEADER_REGEX = /^#{REGEX.values_at(:optional, :name, :approvals, :default_owners).join}/
-
-      def self.parse(line, sectional_data)
-        match = line.match(HEADER_REGEX)
-        return unless match
-
-        new(
-          name: find_section_name(match[:name], sectional_data),
-          optional: match[:optional].present?,
-          approvals: match[:approvals].to_i,
-          default_owners: match[:default_owners]
-        )
-      end
-
-      def self.find_section_name(name, sectional_data)
-        section_headers = sectional_data.keys
-
-        return name if section_headers.last == DEFAULT
-
-        section_headers.find { |k| k.casecmp?(name) } || name
-      end
-
       attr_reader :name, :optional, :approvals, :default_owners
 
       def initialize(name:, optional: false, approvals: 0, default_owners: nil)

ee/lib/gitlab/code_owners/section_parser.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module CodeOwners
+    class SectionParser
+      REGEX = {
+        optional: /(?<optional>\^)?/,
+        name: /\[(?<name>.*?)\]/,
+        approvals: /(?:\[(?<approvals>\d*?)\])?/,
+        default_owners: /(?<default_owners>\s+[@\w_.\-\/\s+]*)?/, # rubocop: disable Style/RegexpLiteralMixedPreserve
+        invalid_name: /\[[^\]]+?/
+      }.freeze
+
+      HEADER_REGEX = /^#{REGEX.values_at(:optional, :name, :approvals, :default_owners).join}/
+      REGEX_INVALID_SECTION = /^#{REGEX.values_at(:optional, :invalid_name).join}$/
+
+      def initialize(line, sectional_data)
+        @line = line
+        @sectional_data = sectional_data
+        @errors = []
+      end
+
+      attr_reader :errors
+
+      def execute
+        section = fetch_section
+
+        if section.present?
+          errors << Error::MISSING_SECTION_NAME if section.name.blank?
+          errors << Error::INVALID_APPROVAL_REQUIREMENT if section.optional && section.approvals > 0
+
+          return section
+        end
+
+        errors << Error::INVALID_SECTION_FORMAT if invalid_section?
+
+        nil
+      end
+
+      def valid?
+        errors.none?
+      end
+
+      private
+
+      attr_reader :line, :sectional_data
+
+      def fetch_section
+        match = line.match(HEADER_REGEX)
+        return unless match
+
+        Section.new(
+          name: find_section_name(match[:name]),
+          optional: match[:optional].present?,
+          approvals: match[:approvals].to_i,
+          default_owners: match[:default_owners]
+        )
+      end
+
+      def find_section_name(name)
+        section_headers = sectional_data.keys
+
+        return name if section_headers.last == Section::DEFAULT
+
+        section_headers.find { |k| k.casecmp?(name) } || name
+      end
+
+      def invalid_section?
+        line.match?(REGEX_INVALID_SECTION)
+      end
+    end
+  end
+end

ee/spec/lib/gitlab/code_owners/file_spec.rb

@@ -440,4 +440,42 @@ def owner_line(pattern)
       it_behaves_like "returns expected matches"
     end
   end
+
+  describe '#valid?' do
+    context 'when codeowners file is correct' do
+      it 'does not detect errors' do
+        expect(file.valid?).to eq(true)
+        expect(file.errors).to be_empty
+      end
+    end
+
+    context 'when codeowners file has errors' do
+      let(:file_content) do
+        <<~CONTENT
+        *.rb
+
+        []
+        *_spec.rb @gl-test
+
+        ^[Optional][5]
+        *.txt @user
+
+        [Invalid section
+        CONTENT
+      end
+
+      it 'detects syntax errors' do
+        expect(file.valid?).to eq(false)
+
+        expect(file.errors).to match_array(
+          [
+            Gitlab::CodeOwners::Error.new(message: :missing_entry_owner, line_number: 1, path: 'CODEOWNERS'),
+            Gitlab::CodeOwners::Error.new(message: :missing_section_name, line_number: 3, path: 'CODEOWNERS'),
+            Gitlab::CodeOwners::Error.new(message: :invalid_approval_requirement, line_number: 6, path: 'CODEOWNERS'),
+            Gitlab::CodeOwners::Error.new(message: :invalid_section_format, line_number: 9, path: 'CODEOWNERS')
+          ]
+        )
+      end
+    end
+  end
 end

ee/spec/lib/gitlab/code_owners/section_spec.rb → ee/spec/lib/gitlab/code_owners/section_parser_spec.rb

@@ -2,13 +2,15 @@
 
 require 'spec_helper'
 
-RSpec.describe Gitlab::CodeOwners::Section, feature_category: :source_code_management do
+RSpec.describe Gitlab::CodeOwners::SectionParser, feature_category: :source_code_management do
   using RSpec::Parameterized::TableSyntax
 
+  subject(:parser) { described_class.new(line, sectional_data) }
+
   let(:sectional_data) { {} }
 
-  describe '.parse' do
-    subject(:section) { described_class.parse(line, sectional_data) }
+  describe '#execute' do
+    subject(:section) { parser.execute }
 
     context 'when line is not a section header' do
       let(:line) { 'foo' }
@@ -17,22 +19,23 @@
     end
 
     context 'when line is a section header' do
-      where(:line, :name, :optional, :approvals, :default_owners, :sectional_data) do
-        '[Doc]'               | 'Doc' | false | 0 | ''          | {}
-        '[Doc]'               | 'doc' | false | 0 | ''          | { 'doc' => {} }
-        '[Doc]'               | 'Doc' | false | 0 | ''          | { 'foo' => {} }
-        '^[Doc]'              | 'Doc' | true  | 0 | ''          | {}
-        '[Doc][1]'            | 'Doc' | false | 1 | ''          | {}
-        '^[Doc][1]'           | 'Doc' | true  | 1 | ''          | {}
-        '^[Doc][1] @doc'      | 'Doc' | true  | 1 | '@doc'      | {}
-        '^[Doc][1] @doc @dev' | 'Doc' | true  | 1 | '@doc @dev' | {}
-        '^[Doc][1] @gl/doc-1' | 'Doc' | true  | 1 | '@gl/doc-1' | {}
-        '[Doc][1] @doc'       | 'Doc' | false | 1 | '@doc'      | {}
-        '[Doc] @doc'          | 'Doc' | false | 0 | '@doc'      | {}
-        '^[Doc] @doc'         | 'Doc' | true  | 0 | '@doc'      | {}
-        '[Doc] @doc @rrr.dev @dev' | 'Doc' | false | 0 | '@doc @rrr.dev @dev' | {}
-        '^[Doc] @doc @rrr.dev @dev' | 'Doc' | true | 0 | '@doc @rrr.dev @dev' | {}
-        '[Doc][2] @doc @rrr.dev @dev' | 'Doc' | false | 2 | '@doc @rrr.dev @dev' | {}
+      where(:line, :name, :optional, :approvals, :default_owners, :sectional_data, :errors) do
+        '[]'                  | ''    | false | 0 | ''          | {}              | [:missing_section_name]
+        '[Doc]'               | 'Doc' | false | 0 | ''          | {}              | []
+        '[Doc]'               | 'doc' | false | 0 | ''          | { 'doc' => {} } | []
+        '[Doc]'               | 'Doc' | false | 0 | ''          | { 'foo' => {} } | []
+        '^[Doc]'              | 'Doc' | true  | 0 | ''          | {}              | []
+        '[Doc][1]'            | 'Doc' | false | 1 | ''          | {}              | []
+        '^[Doc][1]'           | 'Doc' | true  | 1 | ''          | {}              | [:invalid_approval_requirement]
+        '^[Doc][1] @doc'      | 'Doc' | true  | 1 | '@doc'      | {}              | [:invalid_approval_requirement]
+        '^[Doc][1] @doc @dev' | 'Doc' | true  | 1 | '@doc @dev' | {}              | [:invalid_approval_requirement]
+        '^[Doc][1] @gl/doc-1' | 'Doc' | true  | 1 | '@gl/doc-1' | {}              | [:invalid_approval_requirement]
+        '[Doc][1] @doc'       | 'Doc' | false | 1 | '@doc'      | {}              | []
+        '[Doc] @doc'          | 'Doc' | false | 0 | '@doc'      | {}              | []
+        '^[Doc] @doc'         | 'Doc' | true  | 0 | '@doc'      | {}              | []
+        '[Doc] @doc @rrr.dev @dev' | 'Doc' | false | 0 | '@doc @rrr.dev @dev' | {} | []
+        '^[Doc] @doc @rrr.dev @dev' | 'Doc' | true | 0 | '@doc @rrr.dev @dev' | {} | []
+        '[Doc][2] @doc @rrr.dev @dev' | 'Doc' | false | 2 | '@doc @rrr.dev @dev' | {} | []
       end
 
       with_them do
@@ -42,6 +45,29 @@
           expect(section.optional).to eq(optional)
           expect(section.approvals).to eq(approvals)
           expect(section.default_owners).to eq(default_owners)
+
+          if errors.any?
+            expect(parser.valid?).to be_falsey
+            expect(parser.errors).to match_array(errors)
+          else
+            expect(parser.valid?).to be_truthy
+          end
+        end
+      end
+    end
+
+    context 'when section header is invalid' do
+      where(:line, :status, :errors) do
+        '^[Invalid' | false | [:invalid_section_format]
+        '[Invalid'  | false | [:invalid_section_format]
+      end
+
+      with_them do
+        it 'validates section correctness' do
+          expect(section).to be_nil
+
+          expect(parser.valid?).to eq(status)
+          expect(parser.errors).to match_array(errors)
         end
       end
     end