I18n.t('.') returns hash representing translation values

oorion commented 2 years ago

What I tried to do

Tried to run I18n.t('.') or any string that only contains periods

What I expected to happen

I expected it to return something telling me there was no associated translation value since we haven't defined one

What actually happened

It returns a hash representing all translation values

Versions of i18n, rails, and anything else you think is necessary

It's not clear to me if this is a bug or just undocumented behavior but I would expect the method to return the same type of object regardless of the input.

Bonus points for providing an application or a small code example which reproduces the issue.

Thanks! :heart:

codealchemy commented 2 years ago

📝 Some additional context, this appears to only happen when the key given matches the separator used.

# Ruby 2.7.5
# i18n 1.8.11

>> require 'i18n'
=> true
>> I18n.backend.store_translations('en', foo: { bar: 'baz' })
=> {:foo=>{:bar=>"baz"}}
>> I18n.t('.', raise: true)
=> {:foo=>{:bar=>"baz"}}
>> I18n.t('.', separator: '-', raise: true)
I18n::MissingTranslationData: translation missing: en.no key
from /bundle/ruby/2.7.0/gems/i18n-1.8.11/lib/i18n.rb:374:in `handle_exception'
>> I18n.t('-', raise: true)
I18n::MissingTranslationData: translation missing: en.-
from /bundle/ruby/2.7.0/gems/i18n-1.8.11/lib/i18n.rb:374:in `handle_exception'
>> I18n.t('-', separator: '-', raise: true)
=> {:foo=>{:bar=>"baz"}}

Due to how keys are split up when normalizing https://github.com/ruby-i18n/i18n/blob/a1dc4244fc73418a3c0860592b1884da94017992/lib/i18n.rb#L409

Adding a separate case in #normalize_key for when the provided key matches the separator resolves the issue, ie.

        when Array
          key.flat_map { |k| normalize_key(k, separator) }
+      when /^#{Regexp.quote(separator)}+$/
+        [key]
        else

Switching the split to use a regex instead is another approach that could work (and solve https://github.com/ruby-i18n/i18n/issues/247)

        when Array
          key.flat_map { |k| normalize_key(k, separator) }
        else
-         keys = key.to_s.split(separator) 
+         keys = key.to_s.split(/(?!(\b?|^)#{Regexp.quote(separator)}+$)#{Regexp.quote(separator)}{1}/)
          keys.delete('')

Though not sure if either would be the desired approach (or behavior) given the use case - a custom separator can be passed or configured, and effectively do the same thing:

>> require 'i18n'
=> true
>> I18n.default_separator =  /(?!(\b?|^)\.+$)\.{1}/
=> /(?!(\b?|^)\.+$)\.{1}/
>> I18n.backend.store_translations('en', foo: { bar: 'baz', :'qu
  ix.' => 'quix' })
=> {:foo=>{:bar=>"baz", :"quix."=>"quix"}}
>> I18n.t('foo.quix.')
=> "quix"
>> I18n.t('.', raise: true)
I18n::MissingTranslationData (translation missing: en..)
from /bundle/ruby/2.7.0/gems/i18n-1.8.11/lib/i18n.rb:374:in `handle_exception'

radar commented 2 years ago

@oorion I can confirm that's indeed what happens for me too.

Thank you @codealchemy for the additional context here and along with a suggested fix. Would you like to try your hand at submitting a PR for this?

codealchemy commented 2 years ago

@radar in looking into this further there looks to be a decent performance hit (14%-52% slower per check) when using a regex in either of the implementations mentioned above. I'm happy to run with one, but given that all calls for translations pass through normalize_key several times wanted to get your thoughts on this prior to a PR.

Measurements

```ruby # Ruby 2.7.3 require 'benchmark/ips' sample = 'foo.bar.baz.qux.yada.yada' separator = '.' Benchmark.ips do |x| x.report("split string") do sample.split(separator) end x.report("split regex") do sample.split(/(?!(\b|^)#{Regexp.quote(separator)}+$)#{Regexp.quote(separator)}{1}/) end x.compare! end # Warming up -------------------------------------- # split string 168.847k i/100ms # split regex 10.869k i/100ms # Calculating ------------------------------------- # split string 1.693M (± 1.8%) i/s - 8.611M in 5.088305s # split regex 116.806k (± 1.8%) i/s - 586.926k in 5.026523s # Comparison: # split string: 1692893.6 i/s # split regex: 116806.1 i/s - 14.49x (± 0.00) slower Benchmark.ips do |x| sample_leaf = sample.split(separator)[0] x.report("case comp") do case sample_leaf when separator then '' end end x.report("case regex") do case sample_leaf when /^#{Regexp.quote(separator)}+$/ then '' end end x.compare! end # Warming up -------------------------------------- # case comp 1.447M i/100ms # case regex 28.170k i/100ms # Calculating ------------------------------------- # case comp 14.585M (± 0.7%) i/s - 73.778M in 5.058784s # case regex 276.301k (± 4.0%) i/s - 1.380M in 5.004822s # Comparison: # case comp: 14584828.3 i/s # case regex: 276300.8 i/s - 52.79x (± 0.00) slower ```

radar commented 2 years ago

If it's going to be that much slower, then no I don't think we could support it. It would essentially undo a lot of the performance improvement work that the Shopify team have been doing.

Workaround here is to not have keys ending in dots.

ruby-i18n / i18n