Ambiguous Timestamp Handling - #92

what I like here that we're keeping the dst handling information close to the timezone, what I do not like is that we extend the TZ string I know it's unlikely but what happens if users request a different behavior or smt else needs to be extended in the time-zone.

another thing to consider, since time is ambiguous twice a year I wonder if users would ever want to have different behavior (once use dst, second time fallback to non dst time) - a bit far fetched, why anyone would want that, but we should keep our options open.

ways forward seems to be:

1. have a separate configuration option e.g. timezone_dst_enabled_on_overlap => true/false
   or smt of a ambiguous_timezone_local_time_handling => use_dst|use_non_dst|error
2. since the flag relates to time zone maybe extend the option to accepts a string_or_hash e.g.

input {
  jdbc {
    jdbc_connection_string => "..."
    jdbc_default_timezone => {
      name => 'America/Chicago',
      
      ambiguous_local_time_handling => dst
      # or
      dst_enabled_on_overlap => true
      
    }
    use_column_value => true
    # ...
  }
}

3. extend the jdbc_default_timezone with custom parsing [] (current PR status e.g. America/Chicago[dst_enabled_on_overlap:true])

I attempted and ruled out the first option (separate config entirely) because it is just too separate and users aren't likely to find it until after they have run into a DST-transition issue.

I have not yet made a serious attempt at the second (string_or_hash). On the surface I like it, but I believe it would benefit from a validator hack to ensure we keep error messages from malformed instructions close to the user. I will make an attempt at this, and determine if it is cleared in practice.

This PR implements the third option and attempts to remain open to future extension:

• Named timezones are effectively constrained to slashes (/), ASCII letters (a-z and A-Z), dashes (-), dots (.), and underscores (_); by defining extensions enclosed in square brackets, we are immune to upstream changes to the timezonedb
• Our implementation only uses two hard-coded extensions in order to avoid writing a complex parser, but would easily accept multiple semicolon-separated extensions.
• After many attempts at wording, I decided to stick with ruby stdlib's naming dst_enabled_on_overlap, because this extension only handles DST-related ambiguity (and not ambiguity caused by political-upheaval).

Whether we do the second or third approach, adding a validator brings errors closer to the user and is easy enough:

    module JDBCTimezoneSpecValidator
      def validate_value(value, validator_name)
        return super(value, validator_name) unless validator_name == :jdbc_timezone_spec

        return true, value if value.kind_of?(::TZInfo::Timezone) # multiple coercion passes
        return true, nil if value.nil? || (value.kind_of?(String) && value.empty?)

        [true, TimezoneProxy.load(value)] rescue [false, $!.message]
      end
    end

AS to the implementation of accepting either jdbc_default_timezone => "America/Los_Angeles" or jdbc_default_timezone => { name => "America/Los_Angeles" dst_enabled_on_overlap => true }, that is also straight-forward enough:

    def self.load(spec)
      return spec if spec.kind_of?(::TZInfo::Timezone)

      if spec.kind_of?(Hash)
        extensions = spec.dup
        name = extensions.delete("name") { fail(ArgumentError, "timezone must include 'name' key") }
      else
        name = spec
        extensions = {}
      end

      timezone = ::TZInfo::Timezone.get(name)

      if extensions && extensions.include?("dst_enabled_on_overlap")
        dst_enabled_on_overlap = extensions.delete("dst_enabled_on_overlap")
        case dst_enabled_on_overlap.to_s.downcase
        when 'true'  then timezone = timezone.dup.extend(PeriodForLocalWithDSTPreference::ON)
        when 'false' then timezone = timezone.dup.extend(PeriodForLocalWithDSTPreference::OFF)
        else fail(ArgumentError, "Invalid timezone extension `dst_enabled_on_overlap:#{dst_enabled_on_overlap}`")
        end
      end

      fail(ArgumentError, "Unexpected timezone extension: #{extensions}") unless extensions.empty?

      timezone
    end

Interface-wise, I think the square-bracket approach is a bit more straight-forward because the pipeline syntax for maps isn't always clear (whitespace separation, not commas):

input {
  jdbc {
    jdbc_connection_string => "..."
    jdbc_default_timezone => {
      name => "America/Los_Angeles"
      dst_enabled_on_overlap => true
    }
    use_column_value => true
    # ...
  }
}

VS

input {
  jdbc {
    jdbc_connection_string => "..."
    jdbc_default_timezone => "America/Los_Angeles[dst_enabled_on_overlap:true]"
    use_column_value => true
    # ...
  }
}

thanks for looking into this.
fine by shipping the current approach given that the risk of additional time-zone extensions is low (which would blow off the string e.g. America/Los_Angeles[dst_enabled_on_overlap:true][another_extension_on_dst_switch:+1h]) I have no further concerns.

😍 the assert, would the commented bits be still useful later on?