Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove dynamic input used in regular expression #2867

Merged
merged 7 commits into from
Jun 21, 2023
Merged

Remove dynamic input used in regular expression #2867

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1a51397
Remove dynamic input used in regular expression
marcotc May 19, 2023
abb314c
Extract backport to is own method
marcotc Jun 19, 2023
48536ab
Merge branch 'master' into remove-dynamic-regex
marcotc Jun 19, 2023
ad28578
Add type check
marcotc Jun 19, 2023
af6d0d0
Add guard to ensure rbs has non-nil return type
marcotc Jun 20, 2023
fdfc987
Move backports
marcotc Jun 21, 2023
ba6caf0
Merge branch 'master' into remove-dynamic-regex
marcotc Jun 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions lib/datadog/core/utils/backport.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
# frozen_string_literal: true

module Datadog
module Core
module Utils
# Methods from future versions of Ruby implemented in for older rubies.
#
# This helps keep the project using newer APIs for never rubies and
# facilitates cleaning up when support for an older versions of Ruby is removed.
module Backport
# `String` class backports.
module String
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: As more of a "futurology" comment (e.g. thinking of how this may perhaps evolve in the future), maybe it would make sense to have a more top-level module to contain a bag of backports (e.g. Datadog::Core::Backport) or even per-minimum-Ruby-version-to-avoid-the-backport (Datadog::Core::BackportFrom24) versus having it scoped into Utils > String per-class.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand the suggestion, could you expand the examples a bit more?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes!

My thinking is -- in this PR we create a Datadog::Core::Utils::Backport::String. This seems to be highly specific to, er, strings. So every time we need to do this, we'd need to create backport modules for every class.

Furthermore, when eventually we drop support for a few Ruby versions, we'll need to revisit these to figure out which are still needed.

So my suggestion is, why not have a backport module per Ruby version, that would have all backports for all classes there.

E.g. something like:

module Datadog
  module Core
    # This module is used to provide features from Ruby 2.5+ to older Rubies
    module BackportFrom25
      if ::String.method_defined?(:delete_prefix)
        def string_delete_prefix(string, prefix)
          string.delete_prefix(prefix)
        end
      else
        def string_delete_prefix(string, prefix)
          prefix = prefix.to_s
          if string.start_with?(prefix)
            string[prefix.length..-1] || raise('rbs-guard: String#[] is non-nil as `prefix` is guaranteed present')
          else
            string.dup
          end
        end
      end
    end
  end
end

# in another file
module Datadog
  module Core
    # This module is used to provide features from Ruby 2.3+ to older Rubies
    module BackportFrom23
      if ::Thread.method_defined?(:name=)
        def thread_name=(thread, name)
          thread.name = name
        end
      else
        def thread_name=(thread, name)
          # not supported, no-op
        end
      end
    end
  end
end

This way:

  1. We'd have centralized places to find backports (we can always break them down later if needed)
  2. We know that when we drop support for Ruby 2.1 and 2.2, we can nuke BackportFrom23 and clean up everything that uses it

module_function

if ::String.method_defined?(:delete_prefix)
def delete_prefix(string, prefix)
string.delete_prefix(prefix)
end
else
def delete_prefix(string, prefix)
prefix = prefix.to_s
if string.start_with?(prefix)
string[prefix.length..-1]
else
string.dup
end
end
end
end
end
end
end
end
3 changes: 2 additions & 1 deletion lib/datadog/tracing/contrib/rack/middlewares.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
require 'date'

require_relative '../../../core/environment/variable_helpers'
require_relative '../../../core/utils/backport'
require_relative '../../client_ip'
require_relative '../../metadata/ext'
require_relative '../../propagation/http'
Expand Down Expand Up @@ -304,7 +305,7 @@ def parse_url(env, original_env)
else
# normally REQUEST_URI starts at the path, but it
# might contain the full URL in some cases (e.g WEBrick)
request_uri.sub(/^#{base_url}/, '')
Datadog::Core::Utils::Backport::String.delete_prefix(request_uri, base_url)
end

base_url + fullpath
Expand Down
14 changes: 14 additions & 0 deletions sig/datadog/core/utils/backport.rbs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
module Datadog
module Core
module Utils
module Backport
module String
# This method can't return `nil`, but we use `String#[]` to compute
# its return value and `String#[]` can return `nil` but only in
# scenarios that are impossible for `delete_prefix`.
def self?.delete_prefix: (::String string, ::String prefix) -> ::String?
end
end
end
end
end