[Issue454]: Add validation that queries provide the correct parameters #455
Conversation
|
todos: I am adding this review, to make sure everyone is ok with the changes prior to changing all the doc. |
CHANGELOG.md
Outdated
| @@ -3,6 +3,7 @@ This is a history of changes to clara-rules. | |||
| # 0.21.0 SNAPSHOT | |||
| * Add names to anonymous functions generated by rule compilation, see [issue 261](https://github.com/cerner/clara-rules/issues/261) and [issue 291](https://github.com/cerner/clara-rules/issues/291) | |||
| * Add alpha node types, see [issue 237](https://github.com/cerner/clara-rules/issues/237) | |||
| * BREAKING: Validate parameters provided to queries exist on query and compilation. see [issue 454](https://github.com/cerner/clara-rules/issues/454) | |||
There was a problem hiding this comment.
It may be better to not just call it breaking. It is in a sense, but previously you’d get no result basically. Perhaps mention? Or is the issue link enough I guess?
There was a problem hiding this comment.
For something like this where giving the now-breaking parameters would never have given meaningful result this might be a bit too much, but not really a big deal either way IMO. Maybe a phrase like "Throw exception on invalid..." to make clear that if users are giving correct input that they won't be impacted.
There was a problem hiding this comment.
+1 to the changes in b559d93
| @@ -1986,6 +1986,9 @@ | |||
| (let [query-node (get-in rulebase [:query-nodes query])] | |||
| (when (= nil query-node) | |||
| (platform/throw-error (str "The query " query " is invalid or not included in the rule base."))) | |||
| (when-not (= (into #{} (keys params)) ;; nil params should be equivalent to #{} | |||
| (:param-keys query-node)) | |||
| (platform/throw-error (str "The query " query " was not provided with the correct parameters, expected: " (:param-keys query-node) ", provided: " (set (keys params))))) | |||
|
|
|||
There was a problem hiding this comment.
Nit : Really long line?
There was a problem hiding this comment.
Looks like this was resolved by the changes in b559d93
CHANGELOG.md
Outdated
| @@ -3,6 +3,7 @@ This is a history of changes to clara-rules. | |||
| # 0.21.0 SNAPSHOT | |||
| * Add names to anonymous functions generated by rule compilation, see [issue 261](https://github.com/cerner/clara-rules/issues/261) and [issue 291](https://github.com/cerner/clara-rules/issues/291) | |||
| * Add alpha node types, see [issue 237](https://github.com/cerner/clara-rules/issues/237) | |||
| * BREAKING: Validate parameters provided to queries exist on query and compilation. see [issue 454](https://github.com/cerner/clara-rules/issues/454) | |||
There was a problem hiding this comment.
For something like this where giving the now-breaking parameters would never have given meaningful result this might be a bit too much, but not really a big deal either way IMO. Maybe a phrase like "Throw exception on invalid..." to make clear that if users are giving correct input that they won't be impacted.
|
Re todos: +1 to clarifying the doc, but if the current state is that we're accepting query calls that won't actually return data properly I think throwing an exception on that is fine even prior to that - i.e. clarifying doc is a good idea but I personally don't think it is a blocker to merging this. |
|
@EthanEChristian just wondering what your plans are with this one, if you're busy I could perhaps help with getting this over the finish line e.g. updating docs as it is quite close I think. |
|
+1 to merging this after the changes in b559d93 |
A PR for #454