Merge branch 'add-sse-config-file-3' into 'master'

Implement SSE config file processing

See merge request gitlab-org/gitlab!41957

app/controllers/projects/static_site_editor_controller.rb

@@ -27,7 +27,12 @@ class Projects::StaticSiteEditorController < Projects::ApplicationController
     if service_response.success?
       @data = service_response.payload
     else
-      respond_422
+      # TODO: For now, if the service returns any error, the user is redirected
+      #       to the root project page with the error message displayed as an alert.
+      #       See https://gitlab.com/gitlab-org/gitlab/-/issues/213285#note_414808004
+      #       for discussion of plans to handle this via a page owned by the Static Site Editor.
+      flash[:alert] = service_response.message
+      redirect_to project_path(project)
     end
   end
 

app/services/static_site_editor/config_service.rb

@@ -4,18 +4,38 @@ module StaticSiteEditor
   class ConfigService < ::BaseContainerService
     ValidationError = Class.new(StandardError)
 
-    def execute
+    def initialize(container:, current_user: nil, params: {})
+      super
+
       @project = container
+      @repository = project.repository
+      @ref = params.fetch(:ref)
+    end
+
+    def execute
       check_access!
 
+      file_config = load_file_config!
+      file_data = file_config.to_hash_with_defaults
+      generated_data = load_generated_config.data
+
+      check_for_duplicate_keys!(generated_data, file_data)
+      data = merged_data(generated_data, file_data)
+
       ServiceResponse.success(payload: data)
     rescue ValidationError => e
       ServiceResponse.error(message: e.message)
+    rescue => e
+      Gitlab::ErrorTracking.track_and_raise_exception(e)
     end
 
     private
 
-    attr_reader :project
+    attr_reader :project, :repository, :ref
+
+    def static_site_editor_config_file
+      '.gitlab/static-site-editor.yml'
+    end
 
     def check_access!
       unless can?(current_user, :download_code, project)
@@ -23,27 +43,43 @@ module StaticSiteEditor
       end
     end
 
-    def data
-      check_for_duplicate_keys!
-      generated_data.merge(file_data)
+    def load_file_config!
+      yaml = yaml_from_repo.presence || '{}'
+      file_config = Gitlab::StaticSiteEditor::Config::FileConfig.new(yaml)
+
+      unless file_config.valid?
+        raise ValidationError, file_config.errors.first
+      end
+
+      file_config
+    rescue Gitlab::StaticSiteEditor::Config::FileConfig::ConfigError => e
+      raise ValidationError, e.message
     end
 
-    def generated_data
-      @generated_data ||= Gitlab::StaticSiteEditor::Config::GeneratedConfig.new(
+    def load_generated_config
+      Gitlab::StaticSiteEditor::Config::GeneratedConfig.new(
         project.repository,
-        params.fetch(:ref),
+        ref,
         params.fetch(:path),
         params[:return_url]
-      ).data
-    end
-
-    def file_data
-      @file_data ||= Gitlab::StaticSiteEditor::Config::FileConfig.new.data
+      )
     end
 
-    def check_for_duplicate_keys!
+    def check_for_duplicate_keys!(generated_data, file_data)
       duplicate_keys = generated_data.keys & file_data.keys
       raise ValidationError.new("Duplicate key(s) '#{duplicate_keys}' found.") if duplicate_keys.present?
     end
+
+    def merged_data(generated_data, file_data)
+      generated_data.merge(file_data)
+    end
+
+    def yaml_from_repo
+      project.repository.blob_data_at(ref, static_site_editor_config_file)
+    rescue GRPC::NotFound
+      # Return nil in the case of a GRPC::NotFound exception, so the default config will be used.
+      # Allow any other unexpected exception will be tracked and re-raised.
+      nil
+    end
   end
 end

changelogs/unreleased/add-sse-config-file-3.yml

+---
+title: Introduce '.gitlab/static-site-editor.yml' config file, with support for 'static_site_generator'
+  entry.
+merge_request: 41957
+author:
+type: added

lib/gitlab/static_site_editor/config/file_config.rb

@@ -3,11 +3,38 @@
 module Gitlab
   module StaticSiteEditor
     module Config
+      #
+      # Base GitLab Static Site Editor Configuration facade
+      #
       class FileConfig
-        def data
-          {
-            static_site_generator: 'middleman'
-          }
+        ConfigError = Class.new(StandardError)
+
+        def initialize(yaml)
+          content_hash = content_hash(yaml)
+          @global = Entry::Global.new(content_hash)
+          @global.compose!
+        rescue Gitlab::Config::Loader::FormatError => e
+          raise FileConfig::ConfigError, e.message
+        end
+
+        def valid?
+          @global.valid?
+        end
+
+        def errors
+          @global.errors
+        end
+
+        def to_hash_with_defaults
+          # NOTE: The current approach of simply mapping all the descendents' keys and values ('config')
+          #       into a flat hash may need to be enhanced as we add more complex, non-scalar entries.
+          @global.descendants.map { |descendant| [descendant.key, descendant.config] }.to_h
+        end
+
+        private
+
+        def content_hash(yaml)
+          Gitlab::Config::Loader::Yaml.new(yaml).load!
         end
       end
     end

lib/gitlab/static_site_editor/config/file_config/entry/global.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module StaticSiteEditor
+    module Config
+      class FileConfig
+        module Entry
+          ##
+          # This class represents a global entry - root Entry for entire
+          # GitLab StaticSiteEditor Configuration file.
+          #
+          class Global < ::Gitlab::Config::Entry::Node
+            include ::Gitlab::Config::Entry::Configurable
+            include ::Gitlab::Config::Entry::Attributable
+
+            ALLOWED_KEYS = %i[static_site_generator].freeze
+
+            attributes ALLOWED_KEYS
+
+            validations do
+              validates :config, allowed_keys: ALLOWED_KEYS
+            end
+
+            entry :static_site_generator, Entry::StaticSiteGenerator,
+                  description: 'Configuration of the Static Site Editor static site generator.'
+          end
+        end
+      end
+    end
+  end
+end

lib/gitlab/static_site_editor/config/file_config/entry/static_site_generator.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module StaticSiteEditor
+    module Config
+      class FileConfig
+        module Entry
+          ##
+          # Entry that represents the static site generator tool/framework.
+          #
+          class StaticSiteGenerator < ::Gitlab::Config::Entry::Node
+            include ::Gitlab::Config::Entry::Validatable
+
+            validations do
+              validates :config, type: String, inclusion: { in: %w[middleman], message: "should be 'middleman'" }
+            end
+
+            def self.default
+              'middleman'
+            end
+          end
+        end
+      end
+    end
+  end
+end

spec/controllers/projects/static_site_editor_controller_spec.rb

@@ -8,6 +8,8 @@ RSpec.describe Projects::StaticSiteEditorController do
   let(:data) { instance_double(Hash) }
 
   describe 'GET show' do
+    render_views
+
     let(:default_params) do
       {
         namespace_id: project.namespace,
@@ -82,8 +84,9 @@ RSpec.describe Projects::StaticSiteEditorController do
           context 'when invalid config file' do
             let(:service_response) { ServiceResponse.error(message: 'invalid') }
 
-            it 'returns 422' do
-              expect(response).to have_gitlab_http_status(:unprocessable_entity)
+            it 'redirects to project page and flashes error message' do
+              expect(response).to redirect_to(project_path(project))
+              expect(response).to set_flash[:alert].to('invalid')
             end
           end
         end

spec/lib/gitlab/static_site_editor/config/file_config/entry/global_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::StaticSiteEditor::Config::FileConfig::Entry::Global do
+  let(:global) { described_class.new(hash) }
+  let(:default_static_site_generator_value) { 'middleman' }
+
+  shared_examples_for 'valid default configuration' do
+    describe '#compose!' do
+      before do
+        global.compose!
+      end
+
+      it 'creates nodes hash' do
+        expect(global.descendants).to be_an Array
+      end
+
+      it 'creates node object for each entry' do
+        expect(global.descendants.count).to eq 1
+      end
+
+      it 'creates node object using valid class' do
+        expect(global.descendants.first)
+          .to be_an_instance_of expected_node_object_class
+      end
+
+      it 'sets correct description for nodes' do
+        expect(global.descendants.first.description)
+          .to eq 'Configuration of the Static Site Editor static site generator.'
+      end
+
+      describe '#leaf?' do
+        it 'is not leaf' do
+          expect(global).not_to be_leaf
+        end
+      end
+    end
+
+    context 'when not composed' do
+      describe '#static_site_generator_value' do
+        it 'returns nil' do
+          expect(global.static_site_generator_value).to be nil
+        end
+      end
+
+      describe '#leaf?' do
+        it 'is leaf' do
+          expect(global).to be_leaf
+        end
+      end
+    end
+
+    context 'when composed' do
+      before do
+        global.compose!
+      end
+
+      describe '#errors' do
+        it 'has no errors' do
+          expect(global.errors).to be_empty
+        end
+      end
+
+      describe '#static_site_generator_value' do
+        it 'returns correct values' do
+          expect(global.static_site_generator_value).to eq(default_static_site_generator_value)
+        end
+      end
+    end
+  end
+
+  describe '.nodes' do
+    it 'returns a hash' do
+      expect(described_class.nodes).to be_a(Hash)
+    end
+
+    context 'when filtering all the entry/node names' do
+      it 'contains the expected node names' do
+        expect(described_class.nodes.keys)
+          .to match_array(%i[static_site_generator])
+      end
+    end
+  end
+
+  context 'when configuration is valid' do
+    context 'when some entries defined' do
+      let(:expected_node_object_class) { Gitlab::StaticSiteEditor::Config::FileConfig::Entry::StaticSiteGenerator }
+      let(:hash) do
+        { static_site_generator: default_static_site_generator_value }
+      end
+
+      it_behaves_like 'valid default configuration'
+    end
+  end
+
+  context 'when value is an empty hash' do
+    let(:expected_node_object_class) { Gitlab::Config::Entry::Unspecified }
+    let(:hash) { {} }
+
+    it_behaves_like 'valid default configuration'
+  end
+
+  context 'when configuration is not valid' do
+    before do
+      global.compose!
+    end
+
+    context 'when static_site_generator is invalid' do
+      let(:hash) do
+        { static_site_generator: { not_a_string: true } }
+      end
+
+      describe '#errors' do
+        it 'reports errors' do
+          expect(global.errors)
+            .to include 'static_site_generator config should be a string'
+        end
+      end
+    end
+
+    context 'when there is an invalid key' do
+      let(:hash) do
+        { invalid_key: true }
+      end
+
+      describe '#errors' do
+        it 'reports errors' do
+          expect(global.errors)
+            .to include 'global config contains unknown keys: invalid_key'
+        end
+      end
+    end
+  end
+
+  context 'when value is not a hash' do
+    let(:hash) { [] }
+
+    describe '#valid?' do
+      it 'is not valid' do
+        expect(global).not_to be_valid
+      end
+    end
+
+    describe '#errors' do
+      it 'returns error about invalid type' do
+        expect(global.errors.first).to match /should be a hash/
+      end
+    end
+  end
+
+  describe '#specified?' do
+    it 'is concrete entry that is defined' do
+      expect(global.specified?).to be true
+    end
+  end
+
+  describe '#[]' do
+    before do
+      global.compose!
+    end
+
+    let(:hash) do
+      { static_site_generator: default_static_site_generator_value }
+    end
+
+    context 'when entry exists' do
+      it 'returns correct entry' do
+        expect(global[:static_site_generator])
+          .to be_an_instance_of Gitlab::StaticSiteEditor::Config::FileConfig::Entry::StaticSiteGenerator
+        expect(global[:static_site_generator].value).to eq default_static_site_generator_value
+      end
+    end
+
+    context 'when entry does not exist' do
+      it 'always return unspecified node' do
+        expect(global[:some][:unknown][:node])
+          .not_to be_specified
+      end
+    end
+  end
+end

spec/lib/gitlab/static_site_editor/config/file_config/entry/static_site_generator_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::StaticSiteEditor::Config::FileConfig::Entry::StaticSiteGenerator do
+  let(:static_site_generator) { described_class.new(config) }
+
+  describe 'validations' do
+    context 'when value is valid' do
+      let(:config) { 'middleman' }
+
+      describe '#value' do
+        it 'returns a static_site_generator key' do
+          expect(static_site_generator.value).to eq config
+        end
+      end
+
+      describe '#valid?' do
+        it 'is valid' do
+          expect(static_site_generator).to be_valid
+        end
+      end
+    end
+
+    context 'when value is invalid' do
+      let(:config) { 'not-a-valid-generator' }
+
+      describe '#valid?' do
+        it 'is not valid' do
+          expect(static_site_generator).not_to be_valid
+        end
+      end
+    end
+
+    context 'when value has a wrong type' do
+      let(:config) { { not_a_string: true } }
+
+      it 'reports errors about wrong type' do
+        expect(static_site_generator.errors)
+          .to include 'static site generator config should be a string'
+      end
+    end
+  end
+
+  describe '.default' do
+    it 'returns default static_site_generator' do
+      expect(described_class.default).to eq 'middleman'
+    end
+  end
+end

spec/lib/gitlab/static_site_editor/config/file_config_spec.rb

@@ -3,13 +3,93 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::StaticSiteEditor::Config::FileConfig do
-  subject(:config) { described_class.new }
+  let(:config) do
+    described_class.new(yml)
+  end
+
+  context 'when config is valid' do
+    context 'when config has valid values' do
+      let(:yml) do
+        <<-EOS
+        static_site_generator: middleman
+        EOS
+      end
+
+      describe '#to_hash_with_defaults' do
+        it 'returns hash created from string' do
+          hash = {
+            static_site_generator: 'middleman'
+          }
+
+          expect(config.to_hash_with_defaults).to eq hash
+        end
+      end
+
+      describe '#valid?' do
+        it 'is valid' do
+          expect(config).to be_valid
+        end
+
+        it 'has no errors' do
+          expect(config.errors).to be_empty
+        end
+      end
+    end
+  end
+
+  context 'when a config entry has an empty value' do
+    let(:yml) { 'static_site_generator: ' }
+
+    describe '#to_hash_with_defaults' do
+      it 'returns default values' do
+        hash = {
+          static_site_generator: 'middleman'
+        }
+
+        expect(config.to_hash_with_defaults).to eq hash
+      end
+    end
+
+    describe '#valid?' do
+      it 'is valid' do
+        expect(config).to be_valid
+      end
+
+      it 'has no errors' do
+        expect(config.errors).to be_empty
+      end
+    end
+  end
+
+  context 'when config is invalid' do
+    context 'when yml is incorrect' do
+      let(:yml) { '// invalid' }
+
+      describe '.new' do
+        it 'raises error' do
+          expect { config }.to raise_error(described_class::ConfigError, /Invalid configuration format/)
+        end
+      end
+    end
+
+    context 'when config value exists but is not a valid value' do
+      let(:yml) { 'static_site_generator: "unsupported-generator"' }
+
+      describe '#valid?' do
+        it 'is not valid' do
+          expect(config).not_to be_valid
+        end
 
-  describe '#data' do
-    subject { config.data }
+        it 'has errors' do
+          expect(config.errors).not_to be_empty
+        end
+      end
 
-    it 'returns hardcoded data for now' do
-      is_expected.to match(static_site_generator: 'middleman')
+      describe '#errors' do
+        it 'returns an array of strings' do
+          expect(config.errors).to all(be_an_instance_of(String))
+        end
+      end
     end
   end
 end

spec/services/static_site_editor/config_service_spec.rb

@@ -7,8 +7,8 @@ RSpec.describe StaticSiteEditor::ConfigService do
   let_it_be(:user) { create(:user) }
 
   # params
-  let(:ref) { double(:ref) }
-  let(:path) { double(:path) }
+  let(:ref) { 'master' }
+  let(:path) { 'README.md' }
   let(:return_url) { double(:return_url) }
 
   # stub data
@@ -42,22 +42,84 @@ RSpec.describe StaticSiteEditor::ConfigService do
         allow_next_instance_of(Gitlab::StaticSiteEditor::Config::GeneratedConfig) do |config|
           allow(config).to receive(:data) { generated_data }
         end
+      end
+
+      context 'when reading file from repo fails with an unexpected error' do
+        let(:unexpected_error) { RuntimeError.new('some unexpected error') }
 
-        allow_next_instance_of(Gitlab::StaticSiteEditor::Config::FileConfig) do |config|
-          allow(config).to receive(:data) { file_data }
+        before do
+          allow(project.repository).to receive(:blob_data_at).and_raise(unexpected_error)
+        end
+
+        it 'returns an error response' do
+          expect(Gitlab::ErrorTracking).to receive(:track_and_raise_exception).with(unexpected_error).and_call_original
+          expect { execute }.to raise_error(unexpected_error)
         end
       end
 
-      it 'returns merged generated data and config file data' do
-        expect(execute).to be_success
-        expect(execute.payload).to eq(generated: true, file: true)
+      context 'when file is missing' do
+        before do
+          allow(project.repository).to receive(:blob_data_at).and_raise(GRPC::NotFound)
+          expect_next_instance_of(Gitlab::StaticSiteEditor::Config::FileConfig, '{}') do |config|
+            allow(config).to receive(:valid?) { true }
+            allow(config).to receive(:to_hash_with_defaults) { file_data }
+          end
+        end
+
+        it 'returns default config' do
+          expect(execute).to be_success
+          expect(execute.payload).to eq(generated: true, file: true)
+        end
       end
 
-      it 'returns an error if any keys would be overwritten by the merge' do
-        generated_data[:duplicate_key] = true
-        file_data[:duplicate_key] = true
-        expect(execute).to be_error
-        expect(execute.message).to match(/duplicate key.*duplicate_key.*found/i)
+      context 'when file is present' do
+        before do
+          allow(project.repository).to receive(:blob_data_at).with(ref, anything) do
+            config_content
+          end
+        end
+
+        context 'and configuration is not valid' do
+          let(:config_content) { 'invalid content' }
+
+          before do
+            expect_next_instance_of(Gitlab::StaticSiteEditor::Config::FileConfig, config_content) do |config|
+              error = 'error'
+              allow(config).to receive_message_chain('errors.first') { error }
+              allow(config).to receive(:valid?) { false }
+            end
+          end
+
+          it 'returns an error' do
+            expect(execute).to be_error
+            expect(execute.message).to eq('Invalid configuration format')
+          end
+        end
+
+        context 'and configuration is valid' do
+          # NOTE: This has to be a valid config, even though it is mocked, because
+          #       `expect_next_instance_of` executes the constructor logic.
+          let(:config_content) { 'static_site_generator: middleman' }
+
+          before do
+            expect_next_instance_of(Gitlab::StaticSiteEditor::Config::FileConfig, config_content) do |config|
+              allow(config).to receive(:valid?) { true }
+              allow(config).to receive(:to_hash_with_defaults) { file_data }
+            end
+          end
+
+          it 'returns merged generated data and config file data' do
+            expect(execute).to be_success
+            expect(execute.payload).to eq(generated: true, file: true)
+          end
+
+          it 'returns an error if any keys would be overwritten by the merge' do
+            generated_data[:duplicate_key] = true
+            file_data[:duplicate_key] = true
+            expect(execute).to be_error
+            expect(execute.message).to match(/duplicate key.*duplicate_key.*found/i)
+          end
+        end
       end
     end
   end