Symbolize keys and freeze values when loading from YAML #583

paarthmadan · 2021-11-03T19:18:33Z

Changes introduced

Pysch, Ruby's YAML parser, now supports configuring freeze and symbolize_names as options for YAML#unsafe_load_file and YAML#load_file.

This PR sets both of these parameters to true.

Intended outcomes

In setting freeze: true, all values returned will now be frozen objects. Further, string values will be deduplicated.

In setting symbolize_names: true, keys will be directly parsed as symbols. This is particularly beneficial, as we already eventually perform a call to #deep_symbolize_keys. This way, they'll take on their Symbol representation at parse-time.

lib/i18n/backend/base.rb

casperisfine · 2021-11-05T15:02:49Z

lib/i18n/backend/base.rb

+      # are supported in the current version of Pysch.
+      def yaml_load_options
+        @yaml_load_options ||= YAML_LOAD_OPTIONS.select do |k, v|
+          YAML.load_file('test/test_data/locales/en.yml', k => v)


Suggested change

YAML.load_file('test/test_data/locales/en.yml', k => v)

YAML.load('', **{k => v})

Two things here:

That load_file wouldn't work once loaded in an app because it is relative to the current working directory ($PWD)). But you don't need to load a file anyway, load take the same options.

It's best to use **{} to make sure it's passed as keyword arguments, not a positional hash.

Initially, I was trying to use #load, but it turns out load's signature wasn't always a good proxy for the signature of load_file.

Until this commit (by yourself), ruby/psych#463, not all kwargs on load were available to load_file.

This is problematic, as for instance on Ruby 2.7.4, load accepts :symbolize_names but load_file doesn't.

Any suggestions? I certainly agree that using load would be optimal, and didn't fully think through the consequence of using load_file as you pointed out above.

Could accessing the methods args directly help here? Something along the lines of

YAML_LOAD_OPTIONS = YAML.method(:load_file).parameters.each_with_object({}) do |(context, param_name), args| args[param_name] = true if context == :key && %i[symbolize_names freeze].include?(param_name) end

Could accessing the methods args directly help here?

In this instance no, because for a very long time these methods weren't taking keyword args, but one big option hash.

lib/i18n/backend/base.rb

radar · 2021-11-07T21:50:31Z

lib/i18n/backend/base.rb

@@ -236,14 +236,23 @@ def load_rb(filename)
          eval(IO.read(filename), binding, filename)
        end

+        # Feature tests YAML load options and only selects those that
+        # are supported in the current version of Psych.
+        YAML_LOAD_OPTIONS = { symbolize_names: true, freeze: true }.select do |k, v|


Rather than rescuing on an ArgumentError, I would rather a version check (either of Ruby, or of whatever's added the support for these options).

Pseudocode like:

if version >= 3.0.0 <symbolise code> else <old path> end

The problem is that bundled gem sometimes "lie" about their version because of https://bugs.ruby-lang.org/issues/18169, it happened to me previously hence I I now default to feature testing.

That being said, we could conflate this with the feature testing for unsafe_load_file:

if YAML.respond_to?(:unsafe_load_file) # Psych 4.0 way YAML.unsafe_load_file(filename, symbolize_names: true, freeze: true) else YAML.load_file(filename) end

We'll miss a few versions but that's probably fine.

radar

Please do not rescue from ArgumentError.

casperisfine · 2021-11-08T15:59:47Z

@paarthmadan I took the liberty to update your PR. Now it only applies for psych >= 3.3.2. We could theoretically include a few slightly older versions but it's not worth the trouble.

paarthmadan · 2021-11-08T18:00:05Z

Thanks @casperisfine, I appreciate it! FWIW, I agree with the proposed strategy 👍 We scope the change to a smaller (but incomplete) subset of versions to avoid needing to rescue from an ArgumentError.

Sarkar44

request change

paarthmadan · 2021-11-22T15:43:34Z

@radar Just bumping this! Addressed your feedback, now we simply append the arguments when we're certain they're supported.

casperisfine · 2021-11-22T15:55:14Z

A couple note (which doesn't mean they should be added to this PR):

The same could be done for .json files:

Recent json gems also have symbolize_names and freeze
They also have load_file which allows bootsnap to really speed up parsing.

That being said, I've never seen a .json translation file in the wild, so...

As for performance, with the keys already parsed as symbols, deep_symbolize_keys will be faster, but it still need to walk the object graph which is quite slow. It would be even faster to not call it because we know it's useless.

The problem is that it's called in load_file, so it would need a bit of a refactoring to achieve this, but that may be better done in a followup.

radar · 2021-12-13T07:44:06Z

test/backend/simple_test.rb

@@ -72,7 +72,11 @@ def setup

  test "simple load_yml: loads data from a YAML file" do
    data = I18n.backend.send(:load_yml, "#{locales_dir}/en.yml")
-    assert_equal({ 'en' => { 'foo' => { 'bar' => 'baz' } } }, data)
+    if ::YAML.respond_to?(:unsafe_load_file)
+      assert_equal({ :en => { :foo => { :bar => 'baz' } } }, data)


I'd like a check for frozen values here too, please. I think this might work here:

assert data.dig(:en, :foo, :bar).frozen?

Sure, added!

radar

Please add check for frozen

radar · 2022-01-25T00:33:38Z

Please fix the conflict on this branch.

casperisfine reviewed Nov 4, 2021

View reviewed changes

lib/i18n/backend/base.rb Outdated Show resolved Hide resolved

paarthmadan force-pushed the pm/yaml-load-dedup-and-symbolize-keys branch from ea368bf to 0090775 Compare November 5, 2021 14:58

casperisfine reviewed Nov 5, 2021

View reviewed changes

lib/i18n/backend/base.rb Outdated Show resolved Hide resolved

casperisfine reviewed Nov 5, 2021

View reviewed changes

lib/i18n/backend/base.rb Outdated Show resolved Hide resolved

paarthmadan force-pushed the pm/yaml-load-dedup-and-symbolize-keys branch from 0090775 to 2a15f69 Compare November 5, 2021 15:46

radar reviewed Nov 7, 2021

View reviewed changes

radar requested changes Nov 7, 2021

View reviewed changes

casperisfine force-pushed the pm/yaml-load-dedup-and-symbolize-keys branch 2 times, most recently from b52fa23 to 0547768 Compare November 8, 2021 15:58

paarthmadan requested a review from radar November 8, 2021 18:00

Sarkar44 reviewed Nov 9, 2021

View reviewed changes

Sarkar44 approved these changes Nov 9, 2021

View reviewed changes

ruby-i18n deleted a comment from Sarkar44 Nov 9, 2021

This was referenced Dec 8, 2021

Only deep symbolize keys when needed Shopify/i18n#2

Closed

Symbolize names and freeze values when loading from JSON #587

Merged

Only deep_symbolize_keys when needed #588

Merged

radar reviewed Dec 13, 2021

View reviewed changes

radar requested changes Dec 13, 2021

View reviewed changes

paarthmadan force-pushed the pm/yaml-load-dedup-and-symbolize-keys branch from 0547768 to 5e5a092 Compare December 13, 2021 19:15

paarthmadan requested a review from radar December 13, 2021 19:16

radar approved these changes Jan 25, 2022

View reviewed changes

paarthmadan force-pushed the pm/yaml-load-dedup-and-symbolize-keys branch from 5e5a092 to 787bf0d Compare January 26, 2022 15:24

Conditionally assert load_json returns symbolized data

8ac1724

Symbolize and freeze keys when loading from YAML

0fda789

paarthmadan force-pushed the pm/yaml-load-dedup-and-symbolize-keys branch from 787bf0d to 0fda789 Compare January 26, 2022 15:51

radar approved these changes Jan 26, 2022

View reviewed changes

radar merged commit b48b0bc into ruby-i18n:master Jan 26, 2022

paarthmadan deleted the pm/yaml-load-dedup-and-symbolize-keys branch February 18, 2022 16:42

radar mentioned this pull request Nov 13, 2022

[BUG] Using InterpolationCompiler fails #626

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Symbolize keys and freeze values when loading from YAML #583

Symbolize keys and freeze values when loading from YAML #583

paarthmadan commented Nov 3, 2021

casperisfine Nov 5, 2021

paarthmadan Nov 5, 2021 •

edited

Loading

codealchemy Nov 5, 2021 •

edited

Loading

casperisfine Nov 8, 2021

radar Nov 7, 2021

casperisfine Nov 8, 2021

radar left a comment

casperisfine commented Nov 8, 2021

paarthmadan commented Nov 8, 2021

Sarkar44 left a comment

paarthmadan commented Nov 22, 2021

casperisfine commented Nov 22, 2021 •

edited

Loading

radar Dec 13, 2021 •

edited

Loading

paarthmadan Dec 13, 2021

radar left a comment

radar commented Jan 25, 2022

	YAML.load_file('test/test_data/locales/en.yml', k => v)
	YAML.load('', **{k => v})

Symbolize keys and freeze values when loading from YAML #583

Symbolize keys and freeze values when loading from YAML #583

Conversation

paarthmadan commented Nov 3, 2021

Changes introduced

Intended outcomes

casperisfine Nov 5, 2021

Choose a reason for hiding this comment

paarthmadan Nov 5, 2021 • edited Loading

Choose a reason for hiding this comment

codealchemy Nov 5, 2021 • edited Loading

Choose a reason for hiding this comment

casperisfine Nov 8, 2021

Choose a reason for hiding this comment

radar Nov 7, 2021

Choose a reason for hiding this comment

casperisfine Nov 8, 2021

Choose a reason for hiding this comment

radar left a comment

Choose a reason for hiding this comment

casperisfine commented Nov 8, 2021

paarthmadan commented Nov 8, 2021

Sarkar44 left a comment

Choose a reason for hiding this comment

paarthmadan commented Nov 22, 2021

casperisfine commented Nov 22, 2021 • edited Loading

radar Dec 13, 2021 • edited Loading

Choose a reason for hiding this comment

paarthmadan Dec 13, 2021

Choose a reason for hiding this comment

radar left a comment

Choose a reason for hiding this comment

radar commented Jan 25, 2022

paarthmadan Nov 5, 2021 •

edited

Loading

codealchemy Nov 5, 2021 •

edited

Loading

casperisfine commented Nov 22, 2021 •

edited

Loading

radar Dec 13, 2021 •

edited

Loading