fix: six bugs found while auditing the library

lib/deepl_diff/chunker.rb

@@ -3,7 +3,7 @@
 class DeepLDiff::Chunker
   class Error < StandardError; end
 
-  Chunk = Struct.new(:texts, :bytesize)
+  Chunk = Struct.new(:texts, :escaped_size)
 
   MAX_CHUNK_SIZE = 1700
   COUNT_LIMIT = 300
@@ -39,20 +39,25 @@ def chunks
 
   def next_chunk?(tail, value)
     tail.nil? ||
-      (size(value) + tail.bytesize > limit) ||
-      tail.texts.size > count_limit
+      (escaped_size(value) + tail.escaped_size > limit) ||
+      tail.texts.size >= count_limit
   end
 
-  def size(text)
+  # What the limit is about is the size of the request that goes over the
+  # wire, so every measurement here is of the escaped form. Mixing it with
+  # String#size lets a chunk of non-ASCII text run several times over.
+  def escaped_size(text)
     CGI.escape(text).size
   end
 
   def update_chunk(chunk, value)
     chunk.texts << value
-    chunk.bytesize += value.size
+    chunk.escaped_size += escaped_size(value)
   end
 
   def validate_value_size(value)
-    raise Error, "Too long part #{value.size} > #{limit}" if value.size > limit
+    size = escaped_size(value)
+
+    raise Error, "Too long part #{size} > #{limit}" if size > limit
   end
 end

lib/deepl_diff/request.rb

@@ -3,18 +3,22 @@
 class DeepLDiff::Request
   extend Forwardable
 
+  class Error < StandardError; end
+
   def_delegators :DeepLDiff, :api, :cache_store, :rate_limiter
   def_delegators :"DeepLDiff::Linearizer", :linearize, :restore
 
   def initialize(values, options)
     @values = values
-    @options = options
+    # #from and #to consume their keys so the rest can go to the API as-is.
+    # Copy first: the caller's hash is theirs, and it is often frozen.
+    @options = options.dup
   end
 
   def call
     validate_globals
 
-    return values if from == to || values.empty?
+    return values if same_language? || nothing_to_translate?
 
     translation
   end
@@ -31,6 +35,19 @@ def to
     @to ||= options.delete(:to) { nil }
   end
 
+  # A detected language arrives as a String while :to is usually a Symbol, so
+  # the two have to be compared on equal footing or the short circuit never
+  # fires and the text gets translated into its own language.
+  def same_language?
+    !to.nil? && from.to_s.casecmp?(to.to_s)
+  end
+
+  # Covers values holding no translatable text at all: "", nil, an empty
+  # collection, or a scalar the tokenizer has nothing to say about.
+  def nothing_to_translate?
+    text_tokens_texts.all?(&:empty?)
+  end
+
   def detect_language
     api.translate(text_tokens_texts.join(" ")[0..100], nil, to)
        .detected_source_language.downcase
@@ -126,7 +143,12 @@ def restore_spacing(source_value, value)
   # Restores texts from tokens
   # [..., "<b>Horoshiy</b> Malchik", ...]
   def texts_translated
-    @texts_translated ||= tokens_translated.map do |group|
+    @texts_translated ||= tokens_translated.map.with_index do |group, index|
+      source = texts[index]
+      # Only strings are rebuilt from tokens. Anything else has no tokens to
+      # rebuild from; nil keeps collapsing to "" the way it always has.
+      next source unless source.nil? || source.is_a?(String)
+
       group.map { |value, type| type == :text ? value : fix_ascii(value) }.join
     end
   end
@@ -138,7 +160,13 @@ def translation
 
   def call_api(values)
     check_rate_limit(values)
-    [api.translate(values, from, to, options)].flatten.map(&:text)
+    translations = [api.translate(values, from, to, options)].flatten.map(&:text)
+    return translations if translations.size == values.size
+
+    # Letting a short response through means shifting nils into the results,
+    # which surfaces much later as a NoMethodError far from the cause.
+    raise Error,
+          "API returned #{translations.size} translations for #{values.size} values"
   end
 
   def cache

lib/deepl_diff/tokenizer.rb

@@ -149,7 +149,8 @@ def end_markup(name)
 
   class << self
     def tokenize(value)
-      return [] if value.nil?
+      # Anything that is not a string has no markup and no sentences in it.
+      return [] unless value.is_a?(String)
 
       tokenizer = new(value).tap do |h|
         Ox.sax_parse(h, StringIO.new(value), HTML_OPTIONS)

lib/deepl_diff/version.rb

@@ -1,5 +1,5 @@
 # frozen_string_literal: true
 
 module DeepLDiff
-  VERSION = "2.0.0"
+  VERSION = "2.1.0"
 end

test/deepl_diff/chunker_test.rb

@@ -26,7 +26,7 @@ class ChunkerTest < Minitest::Test
     ],
     "splits_on_the_count_limit" => [
       [SHORT] * 10,
-      [[SHORT] * 6, [SHORT] * 4]
+      [[SHORT] * 5, [SHORT] * 5]
     ]
   }.freeze
 
@@ -42,6 +42,25 @@ def test_raises_when_a_single_value_exceeds_the_limit
     assert_match(/Too long part/, error.message)
   end
 
+  # The limit is about the size of the request that goes over the wire, and
+  # CGI.escape inflates Cyrillic sixfold. Measuring the raw String#size
+  # anywhere here let chunks of non-ASCII text run several times over.
+  def test_measures_non_ascii_values_by_their_escaped_size
+    value = "я" * 3
+
+    # Three characters raw, eighteen escaped. Measured raw, both values fit
+    # in one chunk of 20; measured as sent, they cannot.
+    assert_equal 3, value.size
+    assert_equal 18, CGI.escape(value).size
+    assert_equal [[value], [value]], chunk([value, value])
+  end
+
+  def test_raises_when_the_escaped_size_of_one_value_exceeds_the_limit
+    error = assert_raises(DeepLDiff::Chunker::Error) { chunk(["я" * 4]) }
+
+    assert_match(/Too long part 24 > 20/, error.message)
+  end
+
   private
 
   def chunk(values)

test/deepl_diff/request_test.rb

@@ -4,19 +4,23 @@
 
 class RequestTest < Minitest::Test
   Translation = Struct.new(:text)
+  Detection = Struct.new(:detected_source_language)
 
   # Records what it was asked to translate so the call can be asserted on,
   # and answers with a canned response.
   class FakeApi
     attr_reader :calls
 
-    def initialize(response)
+    def initialize(response, detected: nil)
       @response = response
+      @detected = detected
       @calls = []
     end
 
     def translate(text, from, to, options = {})
       @calls << [text, from, to, options]
+      return Detection.new(@detected) if from.nil?
+
       @response.map { |value| Translation.new(value) }
     end
   end
@@ -89,6 +93,89 @@ def test_translates_text_around_markup_and_leaves_the_markup_alone
     assert_equal [[%w[One Black So Red that], :en, :ru, {}]], api.calls
   end
 
+  # The options hash belongs to the caller. Consuming :from and :to out of it
+  # broke the second call with the same hash, and blew up outright on a frozen
+  # one -- which is what a hash of settings kept in a constant is.
+  def test_leaves_the_callers_options_hash_alone
+    options = { from: :en, to: :ru }
+
+    DeepLDiff.api = FakeApi.new(["Какая-то строка"])
+    DeepLDiff.cache_store = FakeCacheStore.new
+
+    2.times do
+      assert_equal "Какая-то строка", DeepLDiff::Request.new("Some string", options).call
+    end
+    assert_equal({ from: :en, to: :ru }, options)
+  end
+
+  def test_accepts_a_frozen_options_hash
+    DeepLDiff.api = FakeApi.new(["Какая-то строка"])
+    DeepLDiff.cache_store = FakeCacheStore.new
+
+    result = DeepLDiff::Request.new("Some string", { from: :en, to: :ru }.freeze).call
+
+    assert_equal "Какая-то строка", result
+  end
+
+  # A detected language comes back as a String while :to is usually a Symbol,
+  # so the source == target short circuit never fired and the text was paid
+  # for and translated into its own language.
+  def test_skips_the_translation_when_the_detected_language_is_the_target
+    api = FakeApi.new([], detected: "RU")
+    DeepLDiff.api = api
+    DeepLDiff.cache_store = FakeCacheStore.new
+
+    result = DeepLDiff::Request.new("привет", { to: :ru }).call
+
+    assert_equal "привет", result
+    assert_equal 1, api.calls.size, "only the detection call should be made"
+  end
+
+  def test_raises_when_the_api_returns_fewer_translations_than_asked_for
+    DeepLDiff.api = FakeApi.new(%w[Один])
+    DeepLDiff.cache_store = FakeCacheStore.new
+
+    error = assert_raises(DeepLDiff::Request::Error) do
+      DeepLDiff::Request.new({ a: "One", b: "Two" }, { from: :en, to: :ru }).call
+    end
+
+    assert_match(/returned 1 translations for 2 values/, error.message)
+  end
+
+  # These used to raise NoMethodError on #empty? or TypeError inside Ox.
+  UNTRANSLATABLE = [42, :sym, "", "   "].freeze
+
+  UNTRANSLATABLE.each do |value|
+    define_method(:"test_passes_through_#{value.inspect.gsub(/\W/, '_')}_untouched") do
+      api = FakeApi.new([])
+      DeepLDiff.api = api
+      DeepLDiff.cache_store = FakeCacheStore.new
+
+      assert_equal value, DeepLDiff::Request.new(value, { from: :en, to: :ru }).call
+      assert_empty api.calls
+    end
+  end
+
+  def test_passes_through_nil_untouched
+    api = FakeApi.new([])
+    DeepLDiff.api = api
+    DeepLDiff.cache_store = FakeCacheStore.new
+
+    assert_nil DeepLDiff::Request.new(nil, { from: :en, to: :ru }).call
+    assert_empty api.calls
+  end
+
+  # Scalars nested in a structure are passed through too, while nil keeps
+  # collapsing to "" the way it always has.
+  def test_passes_nested_scalars_through_and_still_blanks_out_nils
+    DeepLDiff.api = FakeApi.new(%w[Один])
+    DeepLDiff.cache_store = FakeCacheStore.new
+
+    result = DeepLDiff::Request.new({ a: "One", n: 42, skip: nil }, { from: :en, to: :ru }).call
+
+    assert_equal({ a: "Один", n: 42, skip: "" }, result)
+  end
+
   private
 
   # Translates `values` from :en to :ru against fakes.