diff --git a/lib/deepl_diff/chunker.rb b/lib/deepl_diff/chunker.rb index 905744b..fac0837 100644 --- a/lib/deepl_diff/chunker.rb +++ b/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 diff --git a/lib/deepl_diff/request.rb b/lib/deepl_diff/request.rb index 92d0006..d6ed1bc 100644 --- a/lib/deepl_diff/request.rb +++ b/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 # [..., "Horoshiy 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 diff --git a/lib/deepl_diff/tokenizer.rb b/lib/deepl_diff/tokenizer.rb index a609ec3..0d60001 100644 --- a/lib/deepl_diff/tokenizer.rb +++ b/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) diff --git a/lib/deepl_diff/version.rb b/lib/deepl_diff/version.rb index 9d0ab0f..b13c0e6 100644 --- a/lib/deepl_diff/version.rb +++ b/lib/deepl_diff/version.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true module DeepLDiff - VERSION = "2.0.0" + VERSION = "2.1.0" end diff --git a/test/deepl_diff/chunker_test.rb b/test/deepl_diff/chunker_test.rb index b5c82f0..e7d9a1d 100644 --- a/test/deepl_diff/chunker_test.rb +++ b/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) diff --git a/test/deepl_diff/request_test.rb b/test/deepl_diff/request_test.rb index 8beeb93..3a854b5 100644 --- a/test/deepl_diff/request_test.rb +++ b/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.