Multi-arity featurec #118
Multi-arity featurec #118
Conversation
Closes RackSec#117. Closes RackSec#106.
| `(l/all ~@logic-terms)) | ||
| [0 _] (let [u (gensym) | ||
| logic-terms (map (partial featurec-term u) attr-terms)] | ||
| `(l/fresh [~u] ~@logic-terms)))) |
There was a problem hiding this comment.
FYI: you could write this as one clause, but then it would change what all of the old (= (:ip x) "10.0.0.1") generated code would look like.
|
This adds a new feature, so I should probably add an explicit test that just runs the query. |
|
Oh, and this does not yet solve the infix version of this problem... But that should probably be a different ticket/PR so as to keep these as small as possible... |
|
|
||
| [((= value ((attr lvar) :seq)) :seq)] | ||
| `(l/featurec ~lvar {~attr ~value}) | ||
| [(('= & terms) :seq)] |
There was a problem hiding this comment.
I should consider testing the negative case where some of the terms are bogus, seeing what the error message is, and seeing if I can improve it by introducing a predicate on the terms.
Current coverage is
|
|
OK. I've added the test and I'm working on the infix stuff in a separate PR; so this is ready for review after #111, which it is dependent on. |
|
I have also filed #119 since that is some presumably-confusing behavior we're getting out of the logic engine here. |
| (is (thrown? IllegalArgumentException | ||
| (#'q/dsl->logic '(= (:ip x) "10.0.0.1" "10.0.0.1")))) | ||
| (is (thrown? IllegalArgumentException | ||
| (#'q/dsl->logic '(= (:ip x) "10.0.0.1" "10.0.0.2"))))) |
There was a problem hiding this comment.
Was this assertion repeated by accident?
There was a problem hiding this comment.
Nope; this tests subtly different things (and perhaps I should document that)
The former has multiple literals but they are still consistent
The latter has multiple literals but they are inconsistent
There was a problem hiding this comment.
so the former is rationally equivalent to just having 1 literal; the latter is internally inconsistent and will never succeed
There was a problem hiding this comment.
Ah - I thought I scanned it more thoroughly - I didn't realize the ips only differ by the final octet, 1 vs 2.
There was a problem hiding this comment.
I will add an assertion message and make the IPs more visually distinct
| [1 1] (feature (first literals) (first attr-terms)) | ||
| [1 _] `(l/all ~@(map (partial feature (first literals)) attr-terms)) | ||
| [0 _] (let [u (gensym)] | ||
| `(l/fresh [~u] ~@(map (partial feature u) attr-terms))))) |
There was a problem hiding this comment.
I think it's pretty great that even though this adds clear features compared to the previous version and yet it is only 2 lines longer; the only reason it's longer is because it generates the same code for old samples; if it was OK to always generate a fresh form, it could be just as long as possibly even shorter.
|
This looks good to go - Thanks! |
Closes #106, #117. Builds on #111.