Separate set_value_from_env_or_default in two methods and always set default option value (#4627)

Co-authored-by: Marco Costa <[email protected]>

Author: vpellan

lib/datadog/appsec/configuration/settings.rb

@@ -259,11 +259,13 @@ def self.add_settings!(base)
                       APPSEC_VALID_TRACK_USER_EVENTS_ENABLED_VALUES.include?(env_value.strip.downcase)
                     end
                   end
-                  o.after_set do
-                    Core.log_deprecation(key: :appsec_track_user_events_enabled) do
-                      'The appsec.track_user_events.enabled setting has been deprecated for removal. ' \
-                      'Please remove it from your Datadog.configure block and use ' \
-                      'appsec.auto_user_instrumentation.mode instead.'
+                  o.after_set do |_, _, precedence|
+                    unless precedence == Datadog::Core::Configuration::Option::Precedence::DEFAULT
+                      Core.log_deprecation(key: :appsec_track_user_events_enabled) do
+                        'The appsec.track_user_events.enabled setting is deprecated. ' \
+                        'Please remove it from your Datadog.configure block and use ' \
+                        'appsec.auto_user_instrumentation.mode instead.'
+                      end
                     end
                   end
                 end
@@ -287,11 +289,13 @@ def self.add_settings!(base)
                       SAFE_TRACK_USER_EVENTS_MODE
                     end
                   end
-                  o.after_set do
-                    Core.log_deprecation(key: :appsec_track_user_events_mode) do
-                      'The appsec.track_user_events.mode setting has been deprecated for removal. ' \
-                      'Please remove it from your Datadog.configure block and use ' \
-                      'appsec.auto_user_instrumentation.mode instead.'
+                  o.after_set do |_, _, precedence|
+                    unless precedence == Datadog::Core::Configuration::Option::Precedence::DEFAULT
+                      Core.log_deprecation(key: :appsec_track_user_events_mode) do
+                        'The appsec.track_user_events.mode setting is deprecated. ' \
+                        'Please remove it from your Datadog.configure block and use ' \
+                        'appsec.auto_user_instrumentation.mode instead.'
+                      end
                     end
                   end
                 end

lib/datadog/core/configuration/option.rb

@@ -120,11 +120,15 @@ def unset(precedence)
         end
 
         def get
-          if @is_set
-            @value
-          else
-            set_value_from_env_or_default
+          unless @is_set
+            # Ensures that both the default value and the environment value are set.
+            # This approach handles scenarios where an environment value is unset
+            # by falling back to the default value consistently.
+            set_default_value
+            set_env_value
           end
+
+          @value
         end
 
         def reset
@@ -258,7 +262,8 @@ def validate(type, value)
           when NilClass
             true # No validation is performed when option is typeless
           else
-            raise ArgumentError, "The option #{@definition.name} is using an unsupported type option `#{@definition.type}`"
+            raise InvalidDefinitionError,
+              "The option #{@definition.name} is using an unsupported type option `#{@definition.type}`"
           end
         end
 
@@ -271,7 +276,7 @@ def internal_set(value, precedence, resolved_env)
             @resolved_env = resolved_env
             # Store original value to ensure we can always safely call `#internal_set`
             # when restoring a value from `@value_per_precedence`, and we are only running `definition.setter`
-            # on the original value, not on a valud that has already been processed by `definition.setter`.
+            # on the original value, not on a value that has already been processed by `definition.setter`.
             @value_per_precedence[precedence] = value
             context_exec(v, old_value, precedence, &definition.after_set) if definition.after_set
           end
@@ -285,9 +290,12 @@ def context_eval(&block)
           @context.instance_eval(&block)
         end
 
-        def set_value_from_env_or_default
+        def set_default_value
+          set(default_value, precedence: Precedence::DEFAULT)
+        end
+
+        def set_env_value
           value = nil
-          precedence = nil
           resolved_env = nil
 
           if definition.env
@@ -296,24 +304,20 @@ def set_value_from_env_or_default
 
               resolved_env = env
               value = coerce_env_variable(ENV[env])
-              precedence = Precedence::ENVIRONMENT
               break
             end
           end
 
           if value.nil? && definition.deprecated_env && ENV[definition.deprecated_env]
             resolved_env = definition.deprecated_env
             value = coerce_env_variable(ENV[definition.deprecated_env])
-            precedence = Precedence::ENVIRONMENT
 
             Datadog::Core.log_deprecation do
               "#{definition.deprecated_env} environment variable is deprecated, use #{definition.env} instead."
             end
           end
 
-          option_value = value.nil? ? default_value : value
-
-          set(option_value, precedence: precedence || Precedence::DEFAULT, resolved_env: resolved_env)
+          set(value, precedence: Precedence::ENVIRONMENT, resolved_env: resolved_env) unless value.nil?
         rescue ArgumentError
           raise ArgumentError,
             "Expected environment variable #{resolved_env} to be a #{@definition.type}, " \

sig/datadog/core/configuration/option.rbs

@@ -51,7 +51,9 @@ module Datadog
 
         def context_eval: () { () -> untyped } -> untyped
 
-        def set_value_from_env_or_default: () -> untyped
+        def set_default_value: () -> void
+
+        def set_env_value: () -> void
       end
     end
   end

spec/datadog/appsec/configuration/settings_spec.rb

@@ -486,7 +486,7 @@ def patcher
 
             it 'writes the deprication message' do
               expect(Datadog::Core).to receive(:log_deprecation) do |_, &block|
-                expect(block.call).to match(/setting has been deprecated for removal/)
+                expect(block.call).to match(/setting is deprecated/)
               end
               expect(enabled).to eq(true)
             end
@@ -580,7 +580,7 @@ def patcher
 
           it 'writes the deprication message' do
             expect(Datadog::Core).to receive(:log_deprecation) do |_, &block|
-              expect(block.call).to match(/setting has been deprecated for removal/)
+              expect(block.call).to match(/setting is deprecated/)
             end
 
             set_appsec_track_user_events_mode

spec/datadog/core/configuration/option_spec.rb

@@ -36,8 +36,9 @@
   # to make sure specs pass when comparing result ex. expect(result).to be value
   # we ensure that frozen_or_dup returns the same instance
   before do
-    allow(Datadog::Core::Utils::SafeDup).to receive(:frozen_or_dup) do |args, _block|
-      args
+    # |args, _block| is not working with arrays
+    allow(Datadog::Core::Utils::SafeDup).to receive(:frozen_or_dup) do |*args, &_block|
+      args.first
     end
   end
 
@@ -316,11 +317,11 @@
       end
 
       context 'when type is defined' do
-        context 'type is invalid value' do
+        context 'type is invalid' do
           let(:type) { :nullable_string }
           let(:value) { 'Hello' }
           it 'raise exception' do
-            expect { set }.to raise_exception(ArgumentError)
+            expect { set }.to raise_exception(Datadog::Core::Configuration::Option::InvalidDefinitionError)
           end
         end
 
@@ -689,9 +690,11 @@
     subject(:get) { option.get }
 
     shared_examples_for 'env coercion' do
+      # As we now always set default value, we also need to change default to corresponding type
       context 'when type is defined' do
         context ':hash' do
           let(:type) { :hash }
+          let(:default) { {} }
 
           context 'value with commas' do
             let(:env_value) { 'key1:value1,key2:value2' }
@@ -712,6 +715,7 @@
 
         context ':int' do
           let(:type) { :int }
+          let(:default) { 0 }
           let(:env_value) { '1234' }
 
           it 'coerce value' do
@@ -742,6 +746,7 @@
 
         context ':float' do
           let(:type) { :float }
+          let(:default) { 0.0 }
           let(:env_value) { '12.34' }
 
           it 'coerce value' do
@@ -758,7 +763,7 @@
 
         context ':array' do
           let(:type) { :array }
-
+          let(:default) { [] }
           context 'value with commas' do
             let(:env_value) { '12,34' }
 
@@ -778,7 +783,7 @@
 
         context ':bool' do
           let(:type) { :bool }
-
+          let(:default) { false }
           context 'with value 1' do
             let(:env_value) { '1' }
 
@@ -806,6 +811,7 @@
 
         context 'invalid type' do
           let(:type) { :invalid_type }
+          let(:default) { '0' }
           let(:env_value) { '1' }
 
           it 'raise exception' do
@@ -823,10 +829,7 @@
       end
 
       it 'passes the env variable value to the env_parser' do
-        expect(context).to receive(:instance_exec) do |*args, &block|
-          expect(args.first).to eq(env_value)
-          expect(block).to eq env_parser
-        end
+        expect(context).to receive(:instance_exec).with(env_value, &env_parser)
 
         get
       end
@@ -865,6 +868,12 @@
           expect(option.send(:precedence_set)).to eq described_class::Precedence::ENVIRONMENT
         end
 
+        it 'falls back to default when unsetting env' do
+          option.get
+          option.unset(described_class::Precedence::ENVIRONMENT)
+          expect(option.get).to eq default
+        end
+
         it_behaves_like 'env coercion'
         it_behaves_like 'with env_parser'
       end