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

Case sensitive BulkExpressionSolver#solve fails for capitalized keys declared in imperfect order #308

Open
brandoncc opened this issue May 12, 2024 · 1 comment

Comments

@brandoncc
Copy link

brandoncc commented May 12, 2024

Hello, and thanks for this awesome library!

I've built a system which uses primarily capitalized tokens in expressions. These expressions can reference other tokens with no limit to depth of references. To support this, I've created a graph generator that walks the formulas and has a way to output a list of all tokens found and their formulas, like this:

expressions = {
  FIRST: "SECOND * 2",
  SECOND: "THIRD * 2",
  THIRD: 2,
}

When my formulas are more than one reference level deep, as the example above is, Calculator#solve! raises an UnboundVariableError.

I found that this is because my expression hash is in an order where tokens are referenced before they are declared. The hash above is an example of this. Here is a failing test that I've written in spec/bulk_expression_solver_spec.rb to show the problem:

it 'resolves capitalized keys when they are declared out of order' do
  expressions = {
    FIRST: "SECOND * 2",
    SECOND: "THIRD * 2",
    THIRD: 2,
  }

  result = described_class.new(expressions, calculator).solve

  expect(result).to eq(
    FIRST: 8,
    SECOND: 4,
    THIRD: 2
  )
end

The failure message is:

|| Failures:
|| 
||   1) Dentaku::BulkExpressionSolver#solve resolves capitalized keys when they are declared out of order
||      Failure/Error:
||        expect(result).to eq(
||          FIRST: 8,
||          SECOND: 4,
||          THIRD: 2
||        )
||      
||        expected: {:FIRST=>8, :SECOND=>4, :THIRD=>2}
||             got: {:FIRST=>:undefined, :SECOND=>4, :THIRD=>2}
||      
||        (compared using ==)
||      
||        Diff:
||        @@ -1 +1 @@
||        -:FIRST => 8,
||        +:FIRST => :undefined,
||        
||      # ./spec/bulk_expression_solver_spec.rb:210:in `block (3 levels) in <top (required)>'

If I swap the order of FIRST and SECOND in expressions, though, the test passes.

I dug into BulkExpressionSolver#variables_in_resolve_order as a starting point. When the keys are in the order which allows the test to pass (SECOND, FIRST, THIRD), DependencyResolver.find_resolve_order(dependencies) returns ["third", "SECOND", "second", "FIRST", "THIRD"]. When "FIRST" is declared before "SECOND", this call returns ["second", "FIRST", "third", "SECOND", "THIRD"] and the test fails.

I tracked it down to this line. If I change [identifier] to [identifier.upcase], the test passes no matter what order the expressions are declared in the hash.

When I have this line changed to end with [identifier.upcase], DependencyResolver.find_resolve_order(dependencies) returns ["THIRD", "SECOND", "FIRST"].

I found that if I change the calculator initialization to include case_sensitive: false, then the test will also pass and the resolved dependencies are ["THIRD", "SECOND", "FIRST"].

@brandoncc
Copy link
Author

I think the problem is that StringCasing is used to downcase the hash keys/identifiers when case sensitive is false, but the expressions are not downcased.

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

1 participant