Tokenizer crash when passing an array of hashes - Add support for "key[0].foo"

[ndbroadbent]

Hello, I've been using dentaku for a few years, and it's been really great! Thanks for all your work on it!

I just ran into a crash where I need to process some formulas inside an array of hashes. (My case is that I have an array of fields in a table, where each hash represents a row in the table.)

I've been using a fork that just includes this one commit with an "expand" option: DocSpring@458ddd1

EDIT: It looks like Dentaku now calls FlatHash.expand to expand the result by default! That's great.

Here's the branch in my fork, where I've added a failing test case with this "array of hashes" case: https://github.com/DocSpring/dentaku/tree/array_of_hashes

Failing test:

    it "evaluates expressions in hashes and arrays, and expands the results" do
      calculator.store(
        fruit_quantities: {
          apple: 5,
          pear: 9
        },
        fruit_prices: {
          apple: 1.66,
          pear: 2.50
        }
      )
      expressions = {
        weekly_budget: {
          fruit:  "weekly_budget.apples + weekly_budget.pears",
          apples: "fruit_quantities.apple * discounted_fruit_prices.apple",
          pears:  "fruit_quantities.pear * discounted_fruit_prices.pear",
        },
        discounted_fruit_prices: {
          apple: "round(fruit_prices.apple * discounts[0], 2)",
          pear: "round(fruit_prices.pear * discounts[1], 2)"
        },
        discounts: ["0.4 * 2", "0.3 * 2"],
        percents: [
          { percent: 75 },
          { percent: 35 },
          { percent_avg: "round((percents[0].percent + percents[1].percent) / 2)" },
        ]
      }
      solver = described_class.new(expressions, calculator)

      expect(solver.solve!(expand: true)).to eq(
        "weekly_budget" => {
          "fruit" => 20.15,
          "apples" => 6.65,
          "pears" => 13.50
        },
        "discounted_fruit_prices" => {
          "apple" => 1.33,
          "pear" => 1.50
        },
        "discounts" => [0.8, 0.6],
        "percents" => [
          { "percent" => 75 },
          { "percent" => 35 },
          { "percent_avg" => 55 },
        ]
      )
    end

Failure error message:

$ rspec ./spec/bulk_expression_solver_spec.rb:81


  1) Dentaku::BulkExpressionSolver#solve! evaluates expressions in hashes and arrays, and expands the results
     Failure/Error: raise TokenizerError.for(reason, meta), message

     Dentaku::TokenizerError:
       parse error at: ':percent=>75}'
     # ./lib/dentaku/tokenizer.rb:107:in `fail!'
     # ./lib/dentaku/tokenizer.rb:27:in `tokenize'
     ...

(This test was previously passing when it didn't include the "percents" array.)

I can see that an array of strings is supported (discounts: ["0.4 * 2", "0.3 * 2"],), but it doesn't seem to handle the array of hashes.

I'm not quite sure where to start with this, so I was wondering if you might have any idea about how to handle this case in the tokenizer?

Thanks for your time, and no problem at all if you don't have time to look into this! (I know it's a very obscure case!)

[ndbroadbent]

I think I'm making a little bit of progress. I realized that array indexes and hash keys are basically the same thing (inspired by JavaScript), so that might make it easier to implement. I added an experimental test case for FlatHash:

describe Dentaku::FlatHash do
  let(:expressions) do
    {
      string: 'foo',
      hash: {
        string: 'bar',
        number: 12,
        hash: {
          string: 'baz'
        },
        array: ['foo']
      },
      array: ['foo'],
      array_hash: [
        { string: 'foo' },
        { string: 'bar' },
        'baz',
      ]
    }
  end

  it 'flattens a hash with nested hashes and arrays' do
    flattened_hash = Dentaku::FlatHash.from_hash(expressions)
    expect(flattened_hash).to eq(
      :string => "foo",
      :"hash.string" => "bar",
      :"hash.number" => 12,
      :"hash.hash.string" => "baz",
      :"hash.array.0" => "foo",
      :"array.0" => "foo",
      :"array_hash.0.string" => "foo",
      :"array_hash.1.string" => "bar",
      :"array_hash.2" => "baz",
    )
  end
end

And the code:

module Dentaku
  class FlatHash
    def self.from_hash(h, key = [], acc = {})
      case h
      when Hash
        h.each { |k, v| from_hash(v, key + [k], acc) }
      when Array
        h.each_with_index do |el, i|
          from_hash(el, key + [i], acc)
        end
      else
        acc.update(key => h)
      end
      flatten_keys(acc)
    end

The only problem is that this breaks many other things to do with arrays.

I also added another test case for spec/bulk_expression_solver_spec.rb:

    it 'solves all array expressions with nested hashes' do
      calculator.store(first: 2)
      system = {'key' => [{'foo' => 'first'}, 'key[0].foo * first'] }
      solver = described_class.new(system, calculator)
      expect(solver.dependencies).to eq( ...

It looks like Dentaku doesn't support key[0].foo, because I get an error: Dentaku::AST::Multiplication has too many operands. So maybe the access token scanner also needs to be updated to support this.

Would it be possible to add support for hashes inside arrays? I have one case in my app where it's actually currently crashing because of this. Fortunately this case doesn't rely on the dentaku formulas at the moment, so I can ignore the error for now and disable the "formula" feature here, but it would be great if this could be supported!

[rubysolo]

Hmmm, this is a tricky one. I think the root cause of the problem is that stored values in hashes and arrays are accessed differently. Hashes are flattened, and we get the value from "outside" by referencing the flattened key, while arrays are stored, and we can get individual values "inside" of Dentaku. I'll give it some thought and see if I can find a good solution.

[rubysolo]

I added a new branch that allows customizing how nested variables are stored / retrieved: https://github.com/rubysolo/dentaku/tree/lazy-resolver. Can you give that a shot (assuming this is still an issue for you)?

Edit: I tried it out and it fails the same way. 😞 Sorry for the noise.

[ndbroadbent]

Hi @rubysolo, no worries! Also no hurry at all, but I'm definitely still interested if this might be possible in the future. Thanks!