Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
ndbroadbent opened this issue Apr 9, 2020 · 4 comments

Comments

@ndbroadbent
Copy link
Contributor

ndbroadbent commented Apr 9, 2020

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 ndbroadbent changed the title Tokenizer crash when passing an array of hashes Tokenizer crash when passing an array of hashes - Add support for "key[0].foo" Apr 9, 2020
@ndbroadbent
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Owner

rubysolo commented Dec 7, 2020

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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants