Of course it is METRON-379
On October 28, 2016 at 10:07:52, Otto Fowler ([email protected]) wrote: I was looking at METRON-378, and was wondering if we could discuss a little bit. This issue, as stated is that if your stellar expression references variables that are not passed in at runtime, the call still succeeds and passes back a value. From the issue: @Test(expected = ParseException.class) public void testMissingVariables() { String query = "1 + cannot + add + missing + variables"; run(query, new HashMap<>()); } This works, because getDouble is implemented thusly: private Double getDouble(Token<?> token) { Number n = (Number) token.getValue(); if (n == null) { return 0d; } else { return n.doubleValue(); } } Is the STELLAR function suite, we have to do the work to handle missing parameters and variables, but here, for things handled in the StellarCompiler we do not. So the first question is if we want to change this behavior to be consistent with the rest of the library, that is to throw an exception with invalid parameters. Changing the above function to return null will accomplish that, with the side effect of having to refactor the validation calls. As we have discussed before, having validation implemented by passing in a known failure case and expecting an error is not really ideal, since it muddies the purpose of what you are validating ( syntactical correctness of the expression vs logical runtime correctness ) among other things. With a change like this, validation will get an exception, not the return we currently expect in the tests etc. Maybe we don’t want to handle this in the Compiler at all? Maybe StellarParser’s parse() function should validate parameters? This would, I think, require a refactoring or new version of validate so that running validate inside of parse is not expensive and uses the existing constructs built in the call. This would give ‘global’ validation to parameters I believe, and allow for some cleanup in the existing functions, at the cost of the a per function default handling. Anyways, looking for some sage feedback, hopefully from someone who knows the reasoning behind 'return 0d if null ‘. Maybe this is a documentation issue, or can be treated as such?
