diff --git a/app/models/vs_code/settings/vs_code_setting.rb b/app/models/vs_code/settings/vs_code_setting.rb index e55d958d2b4e5b697adabf5a5463d5c4ae9e4218..1401ce820454ed287f33a1f544278433475d50ea 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 } validates :content, presence: true scope :by_setting_type, ->(setting_type) { where(setting_type: setting_type) } 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 0000000000000000000000000000000000000000..6eb340862990c6d1c17a8df5f97c0de4e348f181 --- /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 0000000000000000000000000000000000000000..1333c1f3b8238b247a1dd3502c8ee1a8241eb3e2 --- /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 4748dcb16ef3bb8129c2e83128a5665a984d76ce..bead50c1c50f2a94eb23cb45cfa1b076022a2895 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); 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 0000000000000000000000000000000000000000..38af85dc0c76129a405a493294793cb71251ba0a --- /dev/null +++ b/lib/api/vs_code/settings/entities/vs_code_setting_reference.rb @@ -0,0 +1,23 @@ +# 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| + setting[:updated_at]&.to_i + 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 dc22496e3801a7bef8a5600c64c9c019ce48a2e9..dead28f2f832d1dc6cc32ee9639acb0b668e915b 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,14 @@ class VsCodeSettingsSync < ::API::Base feature_category :web_ide + helpers do + def find_settings + return [DEFAULT_MACHINE] if params[:resource_name] == DEFAULT_MACHINE[:setting_type] + + SettingsFinder.new(current_user, [params[:resource_name]]).execute + end + end + before do authenticate! @@ -21,6 +29,9 @@ class VsCodeSettingsSync < ::API::Base 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 @@ -31,44 +42,71 @@ class VsCodeSettingsSync < ::API::Base 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 - 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 - 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? + if settings.blank? 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 present presenter, with: Entities::VsCodeSetting end end - desc 'Update a specific setting' + 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' + 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 + + present settings, with: Entities::VsCodeSettingReference end - post '/v1/resource/:resource_name' do - authenticate! + desc 'Creates or updates 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', + values: SETTINGS_TYPES + end + post '/v1/resource/:resource_name' do response = CreateOrUpdateService.new(current_user: current_user, params: { content: params[:content], version: params[:version], diff --git a/spec/finders/vs_code/settings/settings_finder_spec.rb b/spec/finders/vs_code/settings/settings_finder_spec.rb index b7b4308bbbd1544db29b50c56ea6b6aecaf1a693..fa24f5d0aec1f7157bd0a608d13df47940381029 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) @@ -53,9 +49,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/models/vs_code/settings/vs_code_setting_spec.rb b/spec/models/vs_code/settings/vs_code_setting_spec.rb index d22cc8158771ee73cbd58bac17d17974d7f092cc..d61e89dc54b5cb101e0dbd2d4d543b4d2c0cb67a 100644 --- a/spec/models/vs_code/settings/vs_code_setting_spec.rb +++ b/spec/models/vs_code/settings/vs_code_setting_spec.rb @@ -11,10 +11,18 @@ 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 + 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 1055a8efded8651cdc81be9c9fbbb2a94406c5e5..c2c75619f36ba3dd4c7763658bc26e14a7de0d23 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,14 @@ end end + shared_examples "returns 400" do + it 'returns 400' do + get api(path, personal_access_token: user_token) + + expect(response).to have_gitlab_http_status(:bad_request) + end + end + describe 'GET /vscode/settings_sync/v1/manifest' do let(:path) { "/vscode/settings_sync/v1/manifest" } @@ -80,6 +90,12 @@ 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 'returns 204 no content and no content ETag header' do get api(path, personal_access_token: user_token) @@ -102,6 +118,55 @@ end end + describe 'GET /vscode/settings_sync/v1/resource/:resource_name/' do + let(:path) { "/vscode/settings_sync/v1/resource/settings/" } + + 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 "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 + 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) + + 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 let(:path) { "/vscode/settings_sync/v1/resource/settings" }