From 95eec106f73a3cf417deef35c7fbc0f146313788 Mon Sep 17 00:00:00 2001
From: Enrique Alcantara <ealcantara@gitlab.com>
Date: Sun, 22 Oct 2023 22:10:56 +0200
Subject: [PATCH 1/7] Code review feedback

Add limit to settings_finder query
---
 app/finders/vs_code/settings/settings_finder.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/finders/vs_code/settings/settings_finder.rb b/app/finders/vs_code/settings/settings_finder.rb
index 459ccdbe566f26..54edc947ae8e46 100644
--- a/app/finders/vs_code/settings/settings_finder.rb
+++ b/app/finders/vs_code/settings/settings_finder.rb
@@ -12,7 +12,7 @@ def execute
         relation = User.find(current_user.id).vscode_settings
         return relation unless setting_types.present?
 
-        relation.by_setting_type(setting_types)
+        relation.by_setting_type(setting_types).limit(SETTINGS_TYPES.length)
       end
 
       private
-- 
GitLab


From 0eff8c5c39801c065e2bdf76dc12f69dd295ea34 Mon Sep 17 00:00:00 2001
From: Enrique Alcantara <ealcantara@gitlab.com>
Date: Wed, 18 Oct 2023 21:08:31 +0200
Subject: [PATCH 2/7] Add resource references endpoint

Add an endpoint to the VsCode Settings Sync API
that allows obtaining a list of resource references

Changelog: added
---
 .../entities/vs_code_setting_reference.rb     | 25 +++++++++
 .../vs_code/settings/vs_code_settings_sync.rb | 54 ++++++++++++++-----
 lib/vs_code/settings.rb                       | 12 +++++
 spec/lib/vs_code/settings_spec.rb             | 22 ++++++++
 .../settings/vs_code_settings_sync_spec.rb    | 46 +++++++++++++---
 5 files changed, 139 insertions(+), 20 deletions(-)
 create mode 100644 lib/api/vs_code/settings/entities/vs_code_setting_reference.rb
 create mode 100644 spec/lib/vs_code/settings_spec.rb

diff --git a/lib/api/vs_code/settings/entities/vs_code_setting_reference.rb b/lib/api/vs_code/settings/entities/vs_code_setting_reference.rb
new file mode 100644
index 00000000000000..aeae1f0b3cf625
--- /dev/null
+++ b/lib/api/vs_code/settings/entities/vs_code_setting_reference.rb
@@ -0,0 +1,25 @@
+# frozen_string_literal: true
+
+module API
+  module VsCode
+    module Settings
+      module Entities
+        class VsCodeSettingReference < Grape::Entity
+          include ::API::Helpers::RelatedResourcesHelpers
+
+          expose :url do |setting|
+            expose_path(api_v4_vscode_settings_sync_v1_resource_path(
+              resource_name: setting[:setting_type],
+              id: setting[:uuid]
+            ))
+          end
+          expose :created do |setting|
+            updated_at = setting[:updated_at]
+
+            updated_at.present? ? updated_at.to_i : nil
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/lib/api/vs_code/settings/vs_code_settings_sync.rb b/lib/api/vs_code/settings/vs_code_settings_sync.rb
index dc22496e3801a7..6e04e643915f92 100644
--- a/lib/api/vs_code/settings/vs_code_settings_sync.rb
+++ b/lib/api/vs_code/settings/vs_code_settings_sync.rb
@@ -8,6 +8,21 @@ class VsCodeSettingsSync < ::API::Base
 
         feature_category :web_ide
 
+        helpers do
+          def empty_response
+            status :no_content
+            header :etag, NO_CONTENT_ETAG
+            body false
+          end
+
+          def find_settings
+            setting_types = [params[:resource_name]]
+            settings = SettingsFinder.new(current_user, setting_types).execute
+
+            ::VsCode::Settings.with_default_machine(settings, setting_types)
+          end
+        end
+
         before do
           authenticate!
 
@@ -24,7 +39,8 @@ class VsCodeSettingsSync < ::API::Base
               tags %w[vscode]
             end
             get '/v1/manifest' do
-              settings = SettingsFinder.new(current_user, SETTINGS_TYPES).execute
+              settings = SettingsFinder.new(current_user, []).execute
+
               presenter = VsCodeManifestPresenter.new(settings)
 
               present presenter, with: Entities::VsCodeManifest
@@ -41,27 +57,37 @@ class VsCodeSettingsSync < ::API::Base
             get '/v1/resource/:resource_name/:id' do
               authenticate!
 
-              setting_name = params[:resource_name]
-              setting = nil
+              settings = find_settings
 
-              if params[:resource_name] == 'machines'
-                setting = DEFAULT_MACHINE
-              else
-                settings = SettingsFinder.new(current_user, [setting_name]).execute
-                setting = settings.first if settings.present?
-              end
-
-              if setting.nil?
-                status :no_content
-                header :etag, NO_CONTENT_ETAG
-                body false
+              if settings.blank?
+                empty_response
               else
+                setting = settings.first
                 header :etag, setting[:uuid]
                 presenter = VsCodeSettingPresenter.new setting
                 present presenter, with: Entities::VsCodeSetting
               end
             end
 
+            desc 'Get a list of references to one or more vscode setting resources' do
+              success [Entities::VsCodeSettingReference]
+              tags %w[vscode]
+            end
+            params do
+              requires :resource_name, type: String, desc: 'Name of the resource such as settings'
+            end
+            get '/v1/resource/:resource_name' do
+              authenticate!
+
+              settings = find_settings
+
+              if settings.blank?
+                empty_response
+              else
+                present settings, with: Entities::VsCodeSettingReference
+              end
+            end
+
             desc 'Update a specific setting'
             params do
               requires :resource_name, type: String, desc: 'Name of the resource such as settings'
diff --git a/lib/vs_code/settings.rb b/lib/vs_code/settings.rb
index 30b91ebb16fada..a4ce1bfa3985ae 100644
--- a/lib/vs_code/settings.rb
+++ b/lib/vs_code/settings.rb
@@ -2,6 +2,8 @@
 
 module VsCode
   module Settings
+    extend self
+
     DEFAULT_MACHINE = {
       id: 1,
       uuid: "3aa16b0f-652e-4850-8429-a00190dac6aa",
@@ -18,5 +20,15 @@ module Settings
     SETTINGS_TYPES = %w[settings extensions globalState machines keybindings snippets tasks].freeze
     DEFAULT_SESSION = "1"
     NO_CONTENT_ETAG = "0"
+
+    # Adds the DEFAULT_MACHINE setting to a setting list
+    # if the setting_types list argument contains the
+    # literal "machines" or it is an empty list.
+    def with_default_machine(settings_relation, setting_types = [])
+      settings_list = settings_relation.to_a
+      settings_list << DEFAULT_MACHINE if setting_types.include?('machines')
+
+      settings_list
+    end
   end
 end
diff --git a/spec/lib/vs_code/settings_spec.rb b/spec/lib/vs_code/settings_spec.rb
new file mode 100644
index 00000000000000..613081d381ec3c
--- /dev/null
+++ b/spec/lib/vs_code/settings_spec.rb
@@ -0,0 +1,22 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe VsCode::Settings, feature_category: :web_ide do
+  describe 'with_default_machine' do
+    context 'when setting_types parameter includes machines type' do
+      it 'returns a settings list that includes the DEFAULT MACHINE' do
+        expect(described_class.with_default_machine([], ['machines'])).to eq(
+          [VsCode::Settings::DEFAULT_MACHINE]
+        )
+      end
+    end
+
+    context 'when setting_types parameter does not include machines type' do
+      it 'returns a settings list that without the default machine' do
+        expect(described_class.with_default_machine([], [])).to eq([])
+        expect(described_class.with_default_machine([], ['settings'])).to eq([])
+      end
+    end
+  end
+end
diff --git a/spec/requests/api/vs_code/settings/vs_code_settings_sync_spec.rb b/spec/requests/api/vs_code/settings/vs_code_settings_sync_spec.rb
index 1055a8efded865..c4c841cc3b4c77 100644
--- a/spec/requests/api/vs_code/settings/vs_code_settings_sync_spec.rb
+++ b/spec/requests/api/vs_code/settings/vs_code_settings_sync_spec.rb
@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 RSpec.describe API::VsCode::Settings::VsCodeSettingsSync, :aggregate_failures, factory_default: :keep, feature_category: :web_ide do
+  include GrapePathHelpers::NamedRouteMatcher
+
   let_it_be(:user) { create_default(:user) }
   let_it_be(:user_token) { create(:personal_access_token) }
 
@@ -21,6 +23,15 @@
     end
   end
 
+  shared_examples "returns 204 no content and no content Etag" do
+    it 'returns 204 no content and no content ETag header' do
+      get api(path, personal_access_token: user_token)
+
+      expect(response).to have_gitlab_http_status(:no_content)
+      expect(response.header['ETag']).to eq(::VsCode::Settings::NO_CONTENT_ETAG)
+    end
+  end
+
   describe 'GET /vscode/settings_sync/v1/manifest' do
     let(:path) { "/vscode/settings_sync/v1/manifest" }
 
@@ -81,12 +92,7 @@
     it_behaves_like "returns unauthorized when not authenticated"
 
     context 'when settings with that type are not present' do
-      it 'returns 204 no content and no content ETag header' do
-        get api(path, personal_access_token: user_token)
-
-        expect(response).to have_gitlab_http_status(:no_content)
-        expect(response.header['ETag']).to eq(::VsCode::Settings::NO_CONTENT_ETAG)
-      end
+      it_behaves_like "returns 204 no content and no content Etag"
     end
 
     context 'when settings with that type are present' do
@@ -102,6 +108,34 @@
     end
   end
 
+  describe 'GET /vscode/settings_sync/v1/resource/:resource_name/' do
+    let(:path) { "/vscode/settings_sync/v1/resource/settings/" }
+
+    it_behaves_like "returns 20x when authenticated", :no_content
+    it_behaves_like "returns unauthorized when not authenticated"
+
+    context 'when settings with that type are not present' do
+      it_behaves_like "returns 204 no content and no content Etag"
+    end
+
+    context 'when settings with that type are present' do
+      let_it_be(:settings) { create(:vscode_setting, content: '{ "key": "value" }') }
+
+      it 'returns settings with the correct json content' do
+        get api(path, personal_access_token: user_token)
+
+        resource_ref = expose_path(api_v4_vscode_settings_sync_v1_resource_path(
+          resource_name: settings[:setting_type],
+          id: settings[:uuid]
+        ))
+
+        expect(json_response.length).to eq(1)
+        expect(json_response.first['url']).to eq(resource_ref)
+        expect(json_response.first['created']).to eq(settings.updated_at.to_i)
+      end
+    end
+  end
+
   describe 'POST /vscode/settings_sync/v1/resource/:resource_name' do
     let(:path) { "/vscode/settings_sync/v1/resource/settings" }
 
-- 
GitLab


From e6bbfda38bdf413c02df5458e2cd847f54310509 Mon Sep 17 00:00:00 2001
From: Enrique Alcantara <ealcantara@gitlab.com>
Date: Fri, 20 Oct 2023 14:55:17 +0200
Subject: [PATCH 3/7] Code review feedback

- Remove unnecessary authenticate! statements
- Return empty array on resource reference collection when
there are not vs_code_setting records for a given setting type
- Properly document API responses
- Return 400 bad request when invalid resource name is received
- Document unused :id parameter
---
 .../vs_code/settings/vs_code_settings_sync.rb | 64 +++++++++++++------
 lib/vs_code/settings.rb                       |  5 +-
 .../settings/vs_code_settings_sync_spec.rb    | 34 ++++++++--
 3 files changed, 73 insertions(+), 30 deletions(-)

diff --git a/lib/api/vs_code/settings/vs_code_settings_sync.rb b/lib/api/vs_code/settings/vs_code_settings_sync.rb
index 6e04e643915f92..63ef8edddb3739 100644
--- a/lib/api/vs_code/settings/vs_code_settings_sync.rb
+++ b/lib/api/vs_code/settings/vs_code_settings_sync.rb
@@ -9,14 +9,18 @@ class VsCodeSettingsSync < ::API::Base
         feature_category :web_ide
 
         helpers do
-          def empty_response
-            status :no_content
-            header :etag, NO_CONTENT_ETAG
-            body false
+          def invalid_resource_name!
+            bad_request! unless SETTINGS_TYPES.include?(params[:resource_name])
           end
 
           def find_settings
             setting_types = [params[:resource_name]]
+            # NOTE: There can only be one database record per setting type per user.
+            # This constraint is the reason behind not paginating these results in
+            # the REST API.
+            #
+            # This constraint is enforced in
+            # app/services/vs_code/settings/create_or_update_service.rb
             settings = SettingsFinder.new(current_user, setting_types).execute
 
             ::VsCode::Settings.with_default_machine(settings, setting_types)
@@ -36,18 +40,27 @@ def find_settings
 
             desc 'Get the settings manifest for Settings Sync' do
               success [Entities::VsCodeManifest]
+              failure [
+                { code: 401, message: '401 Unauthorized' }
+              ]
               tags %w[vscode]
             end
             get '/v1/manifest' do
-              settings = SettingsFinder.new(current_user, []).execute
-
+              settings = SettingsFinder.new(current_user, SETTINGS_TYPES).execute
               presenter = VsCodeManifestPresenter.new(settings)
 
               present presenter, with: Entities::VsCodeManifest
             end
 
             desc 'Get a specific setting resource' do
-              success [Entities::VsCodeSetting]
+              success [
+                Entities::VsCodeSetting,
+                { code: 204, message: 'No content' }
+              ]
+              failure [
+                { code: 400, message: '400 bad request' },
+                { code: 401, message: '401 Unauthorized' }
+              ]
               tags %w[vscode]
             end
             params do
@@ -55,13 +68,20 @@ def find_settings
               requires :id, type: String, desc: 'ID of the resource to retrieve'
             end
             get '/v1/resource/:resource_name/:id' do
-              authenticate!
-
               settings = find_settings
 
+              invalid_resource_name!
+
               if settings.blank?
-                empty_response
+                status :no_content
+                header :etag, NO_CONTENT_ETAG
+                body false
               else
+                # This endpoint does not use the :id parameter
+                # because the first iteration of this API only
+                # supports storing a single record of a given setting_type.
+                # We can rely on obtaining the first record of the setting
+                # result.
                 setting = settings.first
                 header :etag, setting[:uuid]
                 presenter = VsCodeSettingPresenter.new setting
@@ -71,30 +91,34 @@ def find_settings
 
             desc 'Get a list of references to one or more vscode setting resources' do
               success [Entities::VsCodeSettingReference]
+              failure [
+                { code: 400, message: '400 bad request' },
+                { code: 401, message: '401 Unauthorized' }
+              ]
               tags %w[vscode]
             end
             params do
               requires :resource_name, type: String, desc: 'Name of the resource such as settings'
             end
             get '/v1/resource/:resource_name' do
-              authenticate!
-
               settings = find_settings
 
-              if settings.blank?
-                empty_response
-              else
-                present settings, with: Entities::VsCodeSettingReference
-              end
+              invalid_resource_name!
+
+              present settings, with: Entities::VsCodeSettingReference
             end
 
-            desc 'Update a specific setting'
+            desc 'Update a specific setting' do
+              success [{ code: 200, message: 'OK' }]
+              failure [
+                { code: 400, message: 'Bad request' },
+                { code: 401, message: '401 Unauthorized' }
+              ]
+            end
             params do
               requires :resource_name, type: String, desc: 'Name of the resource such as settings'
             end
             post '/v1/resource/:resource_name' do
-              authenticate!
-
               response = CreateOrUpdateService.new(current_user: current_user, params: {
                 content: params[:content],
                 version: params[:version],
diff --git a/lib/vs_code/settings.rb b/lib/vs_code/settings.rb
index a4ce1bfa3985ae..324098350974d2 100644
--- a/lib/vs_code/settings.rb
+++ b/lib/vs_code/settings.rb
@@ -21,9 +21,8 @@ module Settings
     DEFAULT_SESSION = "1"
     NO_CONTENT_ETAG = "0"
 
-    # Adds the DEFAULT_MACHINE setting to a setting list
-    # if the setting_types list argument contains the
-    # literal "machines" or it is an empty list.
+    # Adds the DEFAULT_MACHINE setting to a vs_code_setting list
+    # if the setting_types list argument contains the literal "machines".
     def with_default_machine(settings_relation, setting_types = [])
       settings_list = settings_relation.to_a
       settings_list << DEFAULT_MACHINE if setting_types.include?('machines')
diff --git a/spec/requests/api/vs_code/settings/vs_code_settings_sync_spec.rb b/spec/requests/api/vs_code/settings/vs_code_settings_sync_spec.rb
index c4c841cc3b4c77..5e2451ad2cc5a9 100644
--- a/spec/requests/api/vs_code/settings/vs_code_settings_sync_spec.rb
+++ b/spec/requests/api/vs_code/settings/vs_code_settings_sync_spec.rb
@@ -23,12 +23,11 @@
     end
   end
 
-  shared_examples "returns 204 no content and no content Etag" do
-    it 'returns 204 no content and no content ETag header' do
+  shared_examples "returns 400" do
+    it 'returns 400' do
       get api(path, personal_access_token: user_token)
 
-      expect(response).to have_gitlab_http_status(:no_content)
-      expect(response.header['ETag']).to eq(::VsCode::Settings::NO_CONTENT_ETAG)
+      expect(response).to have_gitlab_http_status(:bad_request)
     end
   end
 
@@ -91,8 +90,19 @@
     it_behaves_like "returns 20x when authenticated", :no_content
     it_behaves_like "returns unauthorized when not authenticated"
 
+    context "when resource type is invalid" do
+      let(:path) { "/vscode/settings_sync/v1/resource/foo/1" }
+
+      it_behaves_like "returns 400"
+    end
+
     context 'when settings with that type are not present' do
-      it_behaves_like "returns 204 no content and no content Etag"
+      it 'returns 204 no content and no content ETag header' do
+        get api(path, personal_access_token: user_token)
+
+        expect(response).to have_gitlab_http_status(:no_content)
+        expect(response.header['ETag']).to eq(::VsCode::Settings::NO_CONTENT_ETAG)
+      end
     end
 
     context 'when settings with that type are present' do
@@ -111,11 +121,21 @@
   describe 'GET /vscode/settings_sync/v1/resource/:resource_name/' do
     let(:path) { "/vscode/settings_sync/v1/resource/settings/" }
 
-    it_behaves_like "returns 20x when authenticated", :no_content
+    context "when resource type is invalid" do
+      let(:path) { "/vscode/settings_sync/v1/resource/foo" }
+
+      it_behaves_like "returns 400"
+    end
+
     it_behaves_like "returns unauthorized when not authenticated"
+    it_behaves_like "returns 20x when authenticated", :ok
 
     context 'when settings with that type are not present' do
-      it_behaves_like "returns 204 no content and no content Etag"
+      it "returns empty array response" do
+        get api(path, personal_access_token: user_token)
+
+        expect(json_response.length).to eq(0)
+      end
     end
 
     context 'when settings with that type are present' do
-- 
GitLab


From 17f1c2546c850c935bb1f20490621bff17b45e29 Mon Sep 17 00:00:00 2001
From: Enrique Alcantara <ealcantara@gitlab.com>
Date: Sun, 22 Oct 2023 22:10:56 +0200
Subject: [PATCH 4/7] Code review feedback

Add limit to settings_finder query
Simplify returning the default machine on resources that
require it
---
 .../vs_code/settings/vs_code_setting.rb       |  4 +++-
 .../vs_code/settings/vs_code_settings_sync.rb | 21 +++++-------------
 lib/vs_code/settings.rb                       | 11 ----------
 .../vs_code/settings/settings_finder_spec.rb  |  4 ++--
 spec/lib/vs_code/settings_spec.rb             | 22 -------------------
 5 files changed, 11 insertions(+), 51 deletions(-)
 delete mode 100644 spec/lib/vs_code/settings_spec.rb

diff --git a/app/models/vs_code/settings/vs_code_setting.rb b/app/models/vs_code/settings/vs_code_setting.rb
index e55d958d2b4e5b..cced8a06b557c5 100644
--- a/app/models/vs_code/settings/vs_code_setting.rb
+++ b/app/models/vs_code/settings/vs_code_setting.rb
@@ -5,7 +5,9 @@ module Settings
     class VsCodeSetting < ApplicationRecord
       belongs_to :user, inverse_of: :vscode_settings
 
-      validates :setting_type, presence: true
+      validates :setting_type, presence: true,
+        inclusion: { in: SETTINGS_TYPES },
+        uniqueness: { scope: :user_id, message: 'One setting type per user' }
       validates :content, presence: true
 
       scope :by_setting_type, ->(setting_type) { where(setting_type: setting_type) }
diff --git a/lib/api/vs_code/settings/vs_code_settings_sync.rb b/lib/api/vs_code/settings/vs_code_settings_sync.rb
index 63ef8edddb3739..4b2bd358553f47 100644
--- a/lib/api/vs_code/settings/vs_code_settings_sync.rb
+++ b/lib/api/vs_code/settings/vs_code_settings_sync.rb
@@ -10,25 +10,20 @@ class VsCodeSettingsSync < ::API::Base
 
         helpers do
           def invalid_resource_name!
-            bad_request! unless SETTINGS_TYPES.include?(params[:resource_name])
+            bad_request! unless SETTINGS_TYPES.include?(params[:resource_name]) ||
+              params.exclude?(:resource_name)
           end
 
           def find_settings
-            setting_types = [params[:resource_name]]
-            # NOTE: There can only be one database record per setting type per user.
-            # This constraint is the reason behind not paginating these results in
-            # the REST API.
-            #
-            # This constraint is enforced in
-            # app/services/vs_code/settings/create_or_update_service.rb
-            settings = SettingsFinder.new(current_user, setting_types).execute
-
-            ::VsCode::Settings.with_default_machine(settings, setting_types)
+            return [DEFAULT_MACHINE] if params[:resource_name] == DEFAULT_MACHINE[:setting_type]
+
+            SettingsFinder.new(current_user, [params[:resource_name]]).execute
           end
         end
 
         before do
           authenticate!
+          invalid_resource_name!
 
           header 'Access-Control-Expose-Headers', 'etag'
         end
@@ -70,8 +65,6 @@ def find_settings
             get '/v1/resource/:resource_name/:id' do
               settings = find_settings
 
-              invalid_resource_name!
-
               if settings.blank?
                 status :no_content
                 header :etag, NO_CONTENT_ETAG
@@ -103,8 +96,6 @@ def find_settings
             get '/v1/resource/:resource_name' do
               settings = find_settings
 
-              invalid_resource_name!
-
               present settings, with: Entities::VsCodeSettingReference
             end
 
diff --git a/lib/vs_code/settings.rb b/lib/vs_code/settings.rb
index 324098350974d2..30b91ebb16fada 100644
--- a/lib/vs_code/settings.rb
+++ b/lib/vs_code/settings.rb
@@ -2,8 +2,6 @@
 
 module VsCode
   module Settings
-    extend self
-
     DEFAULT_MACHINE = {
       id: 1,
       uuid: "3aa16b0f-652e-4850-8429-a00190dac6aa",
@@ -20,14 +18,5 @@ module Settings
     SETTINGS_TYPES = %w[settings extensions globalState machines keybindings snippets tasks].freeze
     DEFAULT_SESSION = "1"
     NO_CONTENT_ETAG = "0"
-
-    # Adds the DEFAULT_MACHINE setting to a vs_code_setting list
-    # if the setting_types list argument contains the literal "machines".
-    def with_default_machine(settings_relation, setting_types = [])
-      settings_list = settings_relation.to_a
-      settings_list << DEFAULT_MACHINE if setting_types.include?('machines')
-
-      settings_list
-    end
   end
 end
diff --git a/spec/finders/vs_code/settings/settings_finder_spec.rb b/spec/finders/vs_code/settings/settings_finder_spec.rb
index b7b4308bbbd154..19372a10f1c976 100644
--- a/spec/finders/vs_code/settings/settings_finder_spec.rb
+++ b/spec/finders/vs_code/settings/settings_finder_spec.rb
@@ -53,9 +53,9 @@
         end
 
         it 'returns multiple records when more than one setting exists' do
-          create(:vscode_setting, user: user, setting_type: 'profile')
+          create(:vscode_setting, user: user, setting_type: 'globalState')
 
-          result = described_class.new(user, %w[settings profile]).execute
+          result = described_class.new(user, %w[settings globalState]).execute
           expect(result.length).to eq(2)
         end
       end
diff --git a/spec/lib/vs_code/settings_spec.rb b/spec/lib/vs_code/settings_spec.rb
deleted file mode 100644
index 613081d381ec3c..00000000000000
--- a/spec/lib/vs_code/settings_spec.rb
+++ /dev/null
@@ -1,22 +0,0 @@
-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe VsCode::Settings, feature_category: :web_ide do
-  describe 'with_default_machine' do
-    context 'when setting_types parameter includes machines type' do
-      it 'returns a settings list that includes the DEFAULT MACHINE' do
-        expect(described_class.with_default_machine([], ['machines'])).to eq(
-          [VsCode::Settings::DEFAULT_MACHINE]
-        )
-      end
-    end
-
-    context 'when setting_types parameter does not include machines type' do
-      it 'returns a settings list that without the default machine' do
-        expect(described_class.with_default_machine([], [])).to eq([])
-        expect(described_class.with_default_machine([], ['settings'])).to eq([])
-      end
-    end
-  end
-end
-- 
GitLab


From a3c96c8edfcec431146dfeab2c9d6a724e29f80a Mon Sep 17 00:00:00 2001
From: Enrique Alcantara <ealcantara@gitlab.com>
Date: Tue, 24 Oct 2023 17:28:09 +0200
Subject: [PATCH 5/7] Code review feedback

Add index to ensure a unique setting_type
on the vs_code_settings table
---
 ..._unique_setting_type_on_vs_code_settings.rb | 18 ++++++++++++++++++
 db/schema_migrations/20231024151916            |  1 +
 db/structure.sql                               |  4 ++--
 3 files changed, 21 insertions(+), 2 deletions(-)
 create mode 100644 db/migrate/20231024151916_add_index_unique_setting_type_on_vs_code_settings.rb
 create mode 100644 db/schema_migrations/20231024151916

diff --git a/db/migrate/20231024151916_add_index_unique_setting_type_on_vs_code_settings.rb b/db/migrate/20231024151916_add_index_unique_setting_type_on_vs_code_settings.rb
new file mode 100644
index 00000000000000..6eb340862990c6
--- /dev/null
+++ b/db/migrate/20231024151916_add_index_unique_setting_type_on_vs_code_settings.rb
@@ -0,0 +1,18 @@
+# frozen_string_literal: true
+
+class AddIndexUniqueSettingTypeOnVsCodeSettings < Gitlab::Database::Migration[2.1]
+  disable_ddl_transaction!
+
+  INDEX_NAME = 'unique_user_id_and_setting_type'
+  PREVIOUS_INDEX_NAME = 'index_vs_code_settings_on_user_id'
+
+  def up
+    remove_concurrent_index_by_name :vs_code_settings, name: PREVIOUS_INDEX_NAME
+    add_concurrent_index :vs_code_settings, [:user_id, :setting_type], name: INDEX_NAME, unique: true
+  end
+
+  def down
+    remove_concurrent_index_by_name :vs_code_settings, name: INDEX_NAME
+    add_concurrent_index :vs_code_settings, [:user_id], name: PREVIOUS_INDEX_NAME
+  end
+end
diff --git a/db/schema_migrations/20231024151916 b/db/schema_migrations/20231024151916
new file mode 100644
index 00000000000000..1333c1f3b8238b
--- /dev/null
+++ b/db/schema_migrations/20231024151916
@@ -0,0 +1 @@
+b316a07e7f307aea53dd9cac257c75ac58ff2b4deeace4e454ec933bd4039761
\ No newline at end of file
diff --git a/db/structure.sql b/db/structure.sql
index 4748dcb16ef3bb..bead50c1c50f2a 100644
--- a/db/structure.sql
+++ b/db/structure.sql
@@ -34676,8 +34676,6 @@ CREATE UNIQUE INDEX index_verification_codes_on_phone_and_visitor_id_code ON ONL
 
 COMMENT ON INDEX index_verification_codes_on_phone_and_visitor_id_code IS 'JiHu-specific index';
 
-CREATE INDEX index_vs_code_settings_on_user_id ON vs_code_settings USING btree (user_id);
-
 CREATE UNIQUE INDEX index_vuln_findings_on_uuid_including_vuln_id_1 ON vulnerability_occurrences USING btree (uuid) INCLUDE (vulnerability_id);
 
 CREATE UNIQUE INDEX index_vuln_historical_statistics_on_project_id_and_date ON vulnerability_historical_statistics USING btree (project_id, date);
@@ -35138,6 +35136,8 @@ CREATE UNIQUE INDEX unique_streaming_event_type_filters_destination_id ON audit_
 
 CREATE UNIQUE INDEX unique_streaming_instance_event_type_filters_destination_id ON audit_events_streaming_instance_event_type_filters USING btree (instance_external_audit_event_destination_id, audit_event_type);
 
+CREATE UNIQUE INDEX unique_user_id_and_setting_type ON vs_code_settings USING btree (user_id, setting_type);
+
 CREATE UNIQUE INDEX unique_vuln_merge_request_link_vuln_id_and_mr_id ON vulnerability_merge_request_links USING btree (vulnerability_id, merge_request_id);
 
 CREATE UNIQUE INDEX unique_zoekt_shards_uuid ON zoekt_shards USING btree (uuid);
-- 
GitLab


From b5663a94b4257f3727dcf554f58deddd29a3480d Mon Sep 17 00:00:00 2001
From: Enrique Alcantara <ealcantara@gitlab.com>
Date: Tue, 24 Oct 2023 18:54:37 +0200
Subject: [PATCH 6/7] Code review feedback

Use grape param values validator
Remove unnecessary subject statement
Add model uniqueness test
---
 app/models/vs_code/settings/vs_code_setting.rb  |  2 +-
 .../vs_code/settings/vs_code_settings_sync.rb   | 17 +++++++----------
 .../vs_code/settings/settings_finder_spec.rb    |  4 ----
 .../vs_code/settings/vs_code_setting_spec.rb    |  4 ++++
 4 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/app/models/vs_code/settings/vs_code_setting.rb b/app/models/vs_code/settings/vs_code_setting.rb
index cced8a06b557c5..1401ce820454ed 100644
--- a/app/models/vs_code/settings/vs_code_setting.rb
+++ b/app/models/vs_code/settings/vs_code_setting.rb
@@ -7,7 +7,7 @@ class VsCodeSetting < ApplicationRecord
 
       validates :setting_type, presence: true,
         inclusion: { in: SETTINGS_TYPES },
-        uniqueness: { scope: :user_id, message: 'One setting type per user' }
+        uniqueness: { scope: :user_id }
       validates :content, presence: true
 
       scope :by_setting_type, ->(setting_type) { where(setting_type: setting_type) }
diff --git a/lib/api/vs_code/settings/vs_code_settings_sync.rb b/lib/api/vs_code/settings/vs_code_settings_sync.rb
index 4b2bd358553f47..dead28f2f832d1 100644
--- a/lib/api/vs_code/settings/vs_code_settings_sync.rb
+++ b/lib/api/vs_code/settings/vs_code_settings_sync.rb
@@ -9,11 +9,6 @@ class VsCodeSettingsSync < ::API::Base
         feature_category :web_ide
 
         helpers do
-          def invalid_resource_name!
-            bad_request! unless SETTINGS_TYPES.include?(params[:resource_name]) ||
-              params.exclude?(:resource_name)
-          end
-
           def find_settings
             return [DEFAULT_MACHINE] if params[:resource_name] == DEFAULT_MACHINE[:setting_type]
 
@@ -23,7 +18,6 @@ def find_settings
 
         before do
           authenticate!
-          invalid_resource_name!
 
           header 'Access-Control-Expose-Headers', 'etag'
         end
@@ -59,7 +53,8 @@ def find_settings
               tags %w[vscode]
             end
             params do
-              requires :resource_name, type: String, desc: 'Name of the resource such as settings'
+              requires :resource_name, type: String, desc: 'Name of the resource such as settings',
+                values: SETTINGS_TYPES
               requires :id, type: String, desc: 'ID of the resource to retrieve'
             end
             get '/v1/resource/:resource_name/:id' do
@@ -91,7 +86,8 @@ def find_settings
               tags %w[vscode]
             end
             params do
-              requires :resource_name, type: String, desc: 'Name of the resource such as settings'
+              requires :resource_name, type: String, desc: 'Name of the resource such as settings',
+                values: SETTINGS_TYPES
             end
             get '/v1/resource/:resource_name' do
               settings = find_settings
@@ -99,7 +95,7 @@ def find_settings
               present settings, with: Entities::VsCodeSettingReference
             end
 
-            desc 'Update a specific setting' do
+            desc 'Creates or updates a specific setting' do
               success [{ code: 200, message: 'OK' }]
               failure [
                 { code: 400, message: 'Bad request' },
@@ -107,7 +103,8 @@ def find_settings
               ]
             end
             params do
-              requires :resource_name, type: String, desc: 'Name of the resource such as settings'
+              requires :resource_name, type: String, desc: 'Name of the resource such as settings',
+                values: SETTINGS_TYPES
             end
             post '/v1/resource/:resource_name' do
               response = CreateOrUpdateService.new(current_user: current_user, params: {
diff --git a/spec/finders/vs_code/settings/settings_finder_spec.rb b/spec/finders/vs_code/settings/settings_finder_spec.rb
index 19372a10f1c976..fa24f5d0aec1f7 100644
--- a/spec/finders/vs_code/settings/settings_finder_spec.rb
+++ b/spec/finders/vs_code/settings/settings_finder_spec.rb
@@ -34,8 +34,6 @@
       let_it_be(:setting) { create(:vscode_setting, user: user) }
 
       context 'when user has no settings with that type' do
-        subject { finder.execute }
-
         it 'returns an empty array' do
           finder = described_class.new(user, ['profile'])
           expect(finder.execute).to eq([])
@@ -43,8 +41,6 @@
       end
 
       context 'when user does have settings with the type' do
-        subject { finder.execute }
-
         it 'returns the record when a single setting exists' do
           result = described_class.new(user, ['settings']).execute
           expect(result.length).to eq(1)
diff --git a/spec/models/vs_code/settings/vs_code_setting_spec.rb b/spec/models/vs_code/settings/vs_code_setting_spec.rb
index d22cc8158771ee..36e8f61a3ba022 100644
--- a/spec/models/vs_code/settings/vs_code_setting_spec.rb
+++ b/spec/models/vs_code/settings/vs_code_setting_spec.rb
@@ -11,6 +11,10 @@
     it { is_expected.to validate_presence_of(:content) }
   end
 
+  describe 'validates the uniqueness of attributes' do
+    it { is_expected.to validate_uniqueness_of(:setting_type).scoped_to([:user_id]) }
+  end
+
   describe 'relationship validation' do
     it { is_expected.to belong_to(:user) }
   end
-- 
GitLab


From effd00b3e103cb489016186ce0c4c42785c8fbf2 Mon Sep 17 00:00:00 2001
From: Enrique Alcantara <ealcantara@gitlab.com>
Date: Fri, 27 Oct 2023 12:23:38 +0200
Subject: [PATCH 7/7] Code review feedback

- Remote unnecessary limit in finder
- Use literal values in tests
- Add model validation
- Add machine-specific created field test
---
 .../vs_code/settings/settings_finder.rb       |  2 +-
 .../entities/vs_code_setting_reference.rb     |  4 +---
 .../vs_code/settings/vs_code_setting_spec.rb  |  4 ++++
 .../settings/vs_code_settings_sync_spec.rb    | 19 +++++++++++++++----
 4 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/app/finders/vs_code/settings/settings_finder.rb b/app/finders/vs_code/settings/settings_finder.rb
index 54edc947ae8e46..459ccdbe566f26 100644
--- a/app/finders/vs_code/settings/settings_finder.rb
+++ b/app/finders/vs_code/settings/settings_finder.rb
@@ -12,7 +12,7 @@ def execute
         relation = User.find(current_user.id).vscode_settings
         return relation unless setting_types.present?
 
-        relation.by_setting_type(setting_types).limit(SETTINGS_TYPES.length)
+        relation.by_setting_type(setting_types)
       end
 
       private
diff --git a/lib/api/vs_code/settings/entities/vs_code_setting_reference.rb b/lib/api/vs_code/settings/entities/vs_code_setting_reference.rb
index aeae1f0b3cf625..38af85dc0c7612 100644
--- a/lib/api/vs_code/settings/entities/vs_code_setting_reference.rb
+++ b/lib/api/vs_code/settings/entities/vs_code_setting_reference.rb
@@ -14,9 +14,7 @@ class VsCodeSettingReference < Grape::Entity
             ))
           end
           expose :created do |setting|
-            updated_at = setting[:updated_at]
-
-            updated_at.present? ? updated_at.to_i : nil
+            setting[:updated_at]&.to_i
           end
         end
       end
diff --git a/spec/models/vs_code/settings/vs_code_setting_spec.rb b/spec/models/vs_code/settings/vs_code_setting_spec.rb
index 36e8f61a3ba022..d61e89dc54b5cb 100644
--- a/spec/models/vs_code/settings/vs_code_setting_spec.rb
+++ b/spec/models/vs_code/settings/vs_code_setting_spec.rb
@@ -19,6 +19,10 @@
     it { is_expected.to belong_to(:user) }
   end
 
+  describe 'settings type validation' do
+    it { is_expected.to validate_inclusion_of(:setting_type).in_array(VsCode::Settings::SETTINGS_TYPES) }
+  end
+
   describe '.by_setting_type' do
     subject { described_class.by_setting_type('settings') }
 
diff --git a/spec/requests/api/vs_code/settings/vs_code_settings_sync_spec.rb b/spec/requests/api/vs_code/settings/vs_code_settings_sync_spec.rb
index 5e2451ad2cc5a9..c2c75619f36ba3 100644
--- a/spec/requests/api/vs_code/settings/vs_code_settings_sync_spec.rb
+++ b/spec/requests/api/vs_code/settings/vs_code_settings_sync_spec.rb
@@ -144,16 +144,27 @@
       it 'returns settings with the correct json content' do
         get api(path, personal_access_token: user_token)
 
-        resource_ref = expose_path(api_v4_vscode_settings_sync_v1_resource_path(
-          resource_name: settings[:setting_type],
-          id: settings[:uuid]
-        ))
+        setting_type = settings[:setting_type]
+        uuid = settings[:uuid]
+
+        resource_ref = "/api/v4/vscode/settings_sync/v1/resource/#{setting_type}/#{uuid}"
 
         expect(json_response.length).to eq(1)
         expect(json_response.first['url']).to eq(resource_ref)
         expect(json_response.first['created']).to eq(settings.updated_at.to_i)
       end
     end
+
+    context 'when setting type is machine' do
+      let(:path) { "/vscode/settings_sync/v1/resource/machines/" }
+
+      it 'created field is nil' do
+        get api(path, personal_access_token: user_token)
+
+        expect(json_response.length).to eq(1)
+        expect(json_response.first['created']).to be_nil
+      end
+    end
   end
 
   describe 'POST /vscode/settings_sync/v1/resource/:resource_name' do
-- 
GitLab