Open
Conversation
Contributor
|
That's a nice approach using a context record in place of the Cmp arg. All the extra eunit tests are awesome. If you want, could even put them in a separate PR and we'd merge them right away. It would make it easier to review subsequent PRs because we can obviously see all the existing tests pass. |
5076677 to
7f8f999
Compare
69fe09f to
fa65b74
Compare
fa65b74 to
1c23788
Compare
1c23788 to
82142aa
Compare
Rather than returning a boolean to indicate just success or failure, `mango_selector:match/2` now returns a list of "failures" describing the ways in which the selector failed to match the input. If this list is empty, the match was a success.
We will need to pass other things around between `match` calls as well the current `Cmp` function, so here we replace this argument with a `#ctx` record that intially just contains a `cmp` field.
To give detailed feedback to the caller, the `#ctx` argument to `mango_selector:match/3` now records the path that was taken to reach each value, and this path is added to the `#failure` records. Each path segment is either a binary, if it represents an object property, or an integer if it represents an array index. Items are pushed on the front of `#ctx.path` as this is faster than pushing onto the back of a list. This list can then be reversed once the final list of failures has been generated, before the failures are presented to the caller.
Collecting detailed `#failure` records rather than a boolean true/false
when evaluating selectors imposes a performance penalty, so we would
like to only do this when a selector is used for a VDU, not when it is
used for indexing/filtering.
To this end we introduce "verbose" mode signalled via the `#ctx.verbose`
field, and each branch of `mango_selector:match/3` now has 3 distinct
versions:
- `#ctx{verbose = false}`: this is the original version that returns
true/false, taken when a selector is used for Mango queries.
- `#ctx{verbose = true, negate = false}`: verbose mode, when the
operator is not negated by an enclosing `$not` operator. Returns a
list of `#failure` records which may be empty.
- `#ctx{verbose = true, negate = true}`: verbose mode, when the operator
is negated by an enclosing `$not` operator. Returns a list of
`#failure` records.
The different negation modes are needed because, in order to generate
meaningful failure messages, we need to record whether an operator was
negated. The behaviour of combinators like `$and`, `$or`, `$allMatch`
and `$elemMatch` means not all `$not` operators can be normalized out of
the selector before evaluation. Instead, when we encounter a `$not`
during evaluation, we flip the `#ctx.negate` field before evaluating the
inner operator.
Until now, document updates rejected by a Mango VDU returned an opaque "forbidden" message to the client. This commit adds a detailed list of failures, obtained by converting the `#failure` records returned by `mango_selector:match/3` into human-readable messages.
82142aa to
74d02e0
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
This PR represents work so far on a version of
mango_selector:match/2than can return a representation of how the input fails to match the selector, rather than just a boolean. There are a few commits of developing this behaviour incrementally before everything "snaps into place" to get us code paths that can produce failure descriptions, and retain the original boolean behaviour that avoids creating a lot of ephemeralfailurelists, and can short-circuit on compound operators.The rough steps here are as follows; I'm retaining all these commits for now in case we want to compare different designs for code complexity and performance.
mango_selectorthat check every match operator and its negation. This is necessary since various compound operators like$orand$allMatchhave surprising edge case behaviour on empty lists and we need to make sure this is not broken. Mostly these tests check that if selectorSreturnstrueon an input then{ "$not": S }returnsfalseand vice versa. There are some exceptions to this due to how$orworks and how$andand$orare normalised.match_intthen converts this to a bool on the way out.Cmpargument to everything with actxrecord that containscmp, as well as other things needed for failure generation, e.g. the path to the current value, whether matching is currently negated, etc.$allMatchand$elemMatchnormalisation to avoid the complexity that comes from not being able to apply DeMorgan to them, due to$allMatchbeing defined to return false for empty lists. This is a breaking change that is reverted later since we decided to retain existing behaviour above all.$nothas to be communicated to nodes lower down the tree in order to produce good failure messages. Not all negation can be normalised out of the tree and so all operators need to handle being negated during evaluation.The idea in the design I've ended up with that tries to minimise both complexity and runtime cost is:
#ctx.verbosewhich indicates whether a detailed failure description is wanted.#ctx{verbose=false}, i.e. when only a boolean result is needed.#ctx{verbose=true}case can be implemented by calling the#ctx{verbose=false}case, and creating a#failurerecord if this returns false.#ctx{verbose=false}cases do not need to deal with#ctx{negate=true}; they continue to return their original result and let$notinvert it. We only need special handling of#ctx{negate=true}in verbose mode, where the$notoperator passes its effect down via the#ctx. This reduces the number of cases each operator has to deal with to basically: non-verbose mode, and positive and negative verbose cases.#ctx.pathis only updated in#ctx{verbose=true}code paths so this expense is avoided in non-verbose mode. Path items are added to the front of this list as that's cheaper than doingPath ++ [Item]; we would reverse these before returning to a client.#failurerecords retain a#ctxfrom where they can access the path and negation state, in order to generate a good human-readable error message later on.verbosemodes return consistent results, i.e. ifverbose=falsereturnstrue, thenverbose=truereturns[], and if the former returnsfalse, the latter gives a non-empty list. These are all passing.Testing recommendations
We should benchmark this in its current version, and both
verbosemodes of this version, against a substantial indexing workload to look for performance regressions. Or, if performance is equivalent in bothverbosemodes, we can remove a lot of redundancy by removing theverboseflag entirely.Related Issues or Pull Requests
Checklist
rel/overlay/etc/default.inisrc/docsfolder