From 8d499d85f9b7935a488ebaf4695b9fdfb9f26702 Mon Sep 17 00:00:00 2001 From: Gerardo Navarro Date: Sun, 14 Dec 2025 19:18:16 +0000 Subject: [PATCH 1/2] Fix elasticsearch_url setter to accept array input The getter returns an array of URIs, but the setter only accepted strings. This caused API round-trip failures (GET then PUT) with a 500 error due to ActiveSupport's Array#split behavior. The setter now handles both String and Array inputs while maintaining backward compatibility. Changelog: fixed EE: true --- doc/api/settings.md | 2 +- ee/app/models/ee/application_setting.rb | 11 ++++- ee/spec/models/application_setting_spec.rb | 55 +++++++++++++--------- 3 files changed, 43 insertions(+), 25 deletions(-) diff --git a/doc/api/settings.md b/doc/api/settings.md index 0a755fe104d5ec..3d599ff3447080 100644 --- a/doc/api/settings.md +++ b/doc/api/settings.md @@ -568,7 +568,7 @@ to configure other related settings. These requirements are | `elasticsearch_namespace_ids` | array of integers | no | The namespaces to index via Elasticsearch if `elasticsearch_limit_indexing` is enabled. Premium and Ultimate only. | | `elasticsearch_project_ids` | array of integers | no | The projects to index via Elasticsearch if `elasticsearch_limit_indexing` is enabled. Premium and Ultimate only. | | `elasticsearch_search` | boolean | no | Enable Elasticsearch search. Premium and Ultimate only. | -| `elasticsearch_url` | string | no | The URL to use for connecting to Elasticsearch. Use a comma-separated list to support cluster (for example, `http://localhost:9200, http://localhost:9201"`). Premium and Ultimate only. | +| `elasticsearch_url` | string or array of strings | no | The URL to use for connecting to Elasticsearch. Use a comma-separated list or an array to support cluster (for example, `http://localhost:9200, http://localhost:9201` or `["http://localhost:9200", "http://localhost:9201"]`). Premium and Ultimate only. | | `elasticsearch_username` | string | no | The `username` of your Elasticsearch instance. Premium and Ultimate only. | | `elasticsearch_password` | string | no | The password of your Elasticsearch instance. Premium and Ultimate only. | | `elasticsearch_prefix` | string | no | Custom prefix for Elasticsearch index names. Defaults to `gitlab`. Must be 1-100 characters, contain only lowercase alphanumeric characters, hyphens, and underscores, and cannot start or end with a hyphen or underscore. Premium and Ultimate only. | diff --git a/ee/app/models/ee/application_setting.rb b/ee/app/models/ee/application_setting.rb index 25b113db46c2f4..7c94faab5660e9 100644 --- a/ee/app/models/ee/application_setting.rb +++ b/ee/app/models/ee/application_setting.rb @@ -618,7 +618,16 @@ def elasticsearch_url end def elasticsearch_url=(values) - cleaned = values.split(',').map { |url| url.strip.gsub(%r{/*\z}, '') } + urls = case values + when String + values.split(',') + when Array + values.flat_map { |v| v.to_s.split(',') } + else + [] + end + + cleaned = urls.map { |url| url.strip.gsub(%r{/*\z}, '') }.reject(&:blank?) write_attribute(:elasticsearch_url, cleaned.join(',')) end diff --git a/ee/spec/models/application_setting_spec.rb b/ee/spec/models/application_setting_spec.rb index 1b54705ca97210..ce00657157f843 100644 --- a/ee/spec/models/application_setting_spec.rb +++ b/ee/spec/models/application_setting_spec.rb @@ -1699,32 +1699,41 @@ def expect_is_es_licensed end describe '#elasticsearch_url', feature_category: :global_search do - it 'presents a single URL as a one-element array' do - setting.elasticsearch_url = 'http://example.com' - - expect(setting.elasticsearch_url).to match_array([URI.parse('http://example.com')]) + using RSpec::Parameterized::TableSyntax + + where(:input_value, :expected_urls) do + 'http://example.com' | [URI.parse('http://example.com')] + 'http://example.com,https://invalid.invalid:9200' | [ + URI.parse('http://example.com'), URI.parse('https://invalid.invalid:9200') + ] + ' http://example.com, https://invalid.invalid:9200 ' | [ + URI.parse('http://example.com'), URI.parse('https://invalid.invalid:9200') + ] + 'http://example.com/, https://example.com:9200/, https://example.com:9200/prefix//' | [ + URI.parse('http://example.com'), URI.parse('https://example.com:9200'), URI.parse('https://example.com:9200/prefix') + ] + + # Array inputs + ['http://example.com'] | [URI.parse('http://example.com')] + ['http://example.com', + 'https://other.example.com:9200'] | [URI.parse('http://example.com'), URI.parse('https://other.example.com:9200')] + ['http://example.com, https://other.example.com:9200'] | [ + URI.parse('http://example.com'), URI.parse('https://other.example.com:9200') + ] + [' http://example.com ', + ' https://other.example.com:9200 '] | [URI.parse('http://example.com'), URI.parse('https://other.example.com:9200')] + + # Edge cases + [] | [] + [URI.parse('http://example.com')] | [URI.parse('http://example.com')] end - it 'presents multiple URLs as a many-element array' do - setting.elasticsearch_url = 'http://example.com,https://invalid.invalid:9200' - - expect(setting.elasticsearch_url).to match_array([URI.parse('http://example.com'), URI.parse('https://invalid.invalid:9200')]) - end - - it 'strips whitespace from around URLs' do - setting.elasticsearch_url = ' http://example.com, https://invalid.invalid:9200 ' - - expect(setting.elasticsearch_url).to match_array([URI.parse('http://example.com'), URI.parse('https://invalid.invalid:9200')]) - end - - it 'strips trailing slashes from URLs' do - setting.elasticsearch_url = 'http://example.com/, https://example.com:9200/, https://example.com:9200/prefix//' + with_them do + it 'correctly parses the array input' do + setting.elasticsearch_url = input_value - expect(setting.elasticsearch_url).to match_array([ - URI.parse('http://example.com'), - URI.parse('https://example.com:9200'), - URI.parse('https://example.com:9200/prefix') - ]) + expect(setting.elasticsearch_url).to match_array(expected_urls) + end end end -- GitLab From a298fb5601ca913023c14eab76d3773d776b4d8e Mon Sep 17 00:00:00 2001 From: Gerardo Navarro Date: Tue, 16 Dec 2025 16:03:00 +0000 Subject: [PATCH 2/2] Add more test cases to increase test coverage - Failed ci job, see https://gitlab.com/gitlab-org/gitlab/-/jobs/12439090781 --- ee/spec/models/application_setting_spec.rb | 29 +++++++++------------- 1 file changed, 12 insertions(+), 17 deletions(-) diff --git a/ee/spec/models/application_setting_spec.rb b/ee/spec/models/application_setting_spec.rb index ce00657157f843..26b1c300be19d9 100644 --- a/ee/spec/models/application_setting_spec.rb +++ b/ee/spec/models/application_setting_spec.rb @@ -1701,32 +1701,27 @@ def expect_is_es_licensed describe '#elasticsearch_url', feature_category: :global_search do using RSpec::Parameterized::TableSyntax + # rubocop:disable Layout/LineLength -- Keep one-line table syntax in this case where(:input_value, :expected_urls) do 'http://example.com' | [URI.parse('http://example.com')] - 'http://example.com,https://invalid.invalid:9200' | [ - URI.parse('http://example.com'), URI.parse('https://invalid.invalid:9200') - ] - ' http://example.com, https://invalid.invalid:9200 ' | [ - URI.parse('http://example.com'), URI.parse('https://invalid.invalid:9200') - ] - 'http://example.com/, https://example.com:9200/, https://example.com:9200/prefix//' | [ - URI.parse('http://example.com'), URI.parse('https://example.com:9200'), URI.parse('https://example.com:9200/prefix') - ] + 'http://example.com,https://invalid.invalid:9200' | [URI.parse('http://example.com'), URI.parse('https://invalid.invalid:9200')] + ' http://example.com, https://invalid.invalid:9200 ' | [URI.parse('http://example.com'), URI.parse('https://invalid.invalid:9200')] + 'http://example.com/, https://example.com:9200/, https://example.com:9200/prefix//' | [URI.parse('http://example.com'), URI.parse('https://example.com:9200'), URI.parse('https://example.com:9200/prefix')] # Array inputs - ['http://example.com'] | [URI.parse('http://example.com')] - ['http://example.com', - 'https://other.example.com:9200'] | [URI.parse('http://example.com'), URI.parse('https://other.example.com:9200')] - ['http://example.com, https://other.example.com:9200'] | [ - URI.parse('http://example.com'), URI.parse('https://other.example.com:9200') - ] - [' http://example.com ', - ' https://other.example.com:9200 '] | [URI.parse('http://example.com'), URI.parse('https://other.example.com:9200')] + ['http://example.com'] | [URI.parse('http://example.com')] + ['http://example.com', 'https://other.example.com:9200'] | [URI.parse('http://example.com'), URI.parse('https://other.example.com:9200')] + ['http://example.com, https://other.example.com:9200'] | [URI.parse('http://example.com'), URI.parse('https://other.example.com:9200')] + [' http://example.com ', ' https://other.example.com:9200 '] | [URI.parse('http://example.com'), URI.parse('https://other.example.com:9200')] # Edge cases [] | [] [URI.parse('http://example.com')] | [URI.parse('http://example.com')] + '' | [] + nil | [] + 123 | [] end + # rubocop:enable Layout/LineLength with_them do it 'correctly parses the array input' do -- GitLab