Repository navigation
feat: validate the whole query in IsValid(out errors), fix contains on non-string fields (#332) - #346
Conversation
2.18.0 added IsValid(out List<string> validationErrors) for IGridifyFiltering, but nothing equivalent for IGridifyOrdering or IGridifyQuery. Since IGridifyQuery inherits from both, the error collecting call on a GridifyQuery bound to the IGridifyFiltering extension and validated Filter only, so the two overloads disagreed on the same object: var q = new GridifyQuery(1, 100, "Name=Ivan", "ageee"); q.IsValid(out var errors, mapper); // true, errors empty q.IsValid(mapper); // false Adds the two missing overloads: - IGridifyOrdering.IsValid(out validationErrors, mapper) reports unmapped ordering fields as "Field 'x' is not mapped", matching the wording the filtering validation already uses, and captures the message of the GridifyOrderingException that a malformed ordering throws - IGridifyQuery.IsValid(out validationErrors, mapper) validates both parts and concatenates their errors Both parts are validated even when the first already failed, so the caller gets every error instead of only the ones from Filter. The boolean IGridifyQuery overload keeps its short circuit, since it only has to produce the verdict. ParseOrderings is a lazy iterator that throws part way through enumeration, so the foreach stays inside the try and errors collected before the throw are kept.
PR Summary
|
An unmapped field used in both Filter and OrderBy produced the identical message twice, and so did ordering on the same unmapped field more than once. One unmapped field is one problem, and the filtering side already reported it once per distinct field, so ordering now matches: - the ordering overload skips a message it has already collected - the query overload appends only the ordering errors the filter did not already report Distinct unmapped fields are still reported separately.
#332) Three cases where IsValid reported a query as valid and the query builder then threw. Each was found by running the verdict against ApplyFiltering or ApplyOrdering rather than reading the code, and each is checked the way the builder checks it. AllowNullSearch was read from GridifyGlobalConfiguration while BaseQueryBuilder.BuildQuery reads mapper.Configuration, so a mapper built with a different value validated under the wrong rule, in both directions: with the global on and the mapper off, "Rank=null" validated but threw, and with the global off and the mapper on it failed validation although it ran fine. The '?' and '!' ordering suffixes order by the member's HasValue, which LinqSortingQueryBuilder can only build for a Nullable<> member. Ordering was only checked for being mapped, so "Age?" on an int, and "Name?" on a string, validated and then threw GridifyOrderingException. Like and NotLike call string.Contains on the member itself. StartsWith and EndsWith fall back to ToString for a non-string member but these two do not, so "Age=*5" validated and then threw ArgumentException. Note the alternative fix is in the query builder, giving Like the same ToString fallback its siblings have, which would make such filters work rather than be rejected. One theory case asserted "IntProperty?, StringProperty!" was valid, which it never was at execution time; it now covers the nullable members instead. Still not validated, since none can be checked without either an IQueryable or guessing at another subsystem: ordering by a non-orderable member such as a collection or dictionary, values behind nested-collection and indexed maps whose property type cannot be extracted, and the Elasticsearch builder, which emits the raw token for most operators.
Like and NotLike called string.Contains on the member itself, so "Age=*5" on an int threw ArgumentException, while StartsWith and EndsWith had a ToString fallback and "Age^3" worked. The previous commit made validation reject the contains operator on a non-string field to match. That was the wrong side to fix: validation exists to predict the builder, so making it stricter than the builder hides a builder gap rather than closing it. Like and NotLike now fall back to ToString exactly as their siblings do, which is the mirror of the two cases immediately below them, and the validation rejection added in the previous commit is removed. "Age=*5" and "Rank!*1" now build and run. A nullable member with no value has an empty text and simply does not match, rather than throwing. Verified unchanged for string members, for '^' and '$', for the case insensitive '/i' suffix, and with EntityFrameworkCompatibilityLayer on. Two limitations stay, both shared with StartsWith and EndsWith and neither new: the value is still converted to the member's type before the operator runs, so "When=*2024" on a DateTime cannot match and is still reported invalid; and the case insensitive suffix only lowercases a string member, so it has no effect on the text of a non-string one. The Elasticsearch builder has its own handling of this operator and is untouched.
The builder change in the previous commit was verified against real SQL translation and against the Elasticsearch query DSL, but only by scratch probes that left nothing behind. These make those checks permanent, so a later change cannot silently undo them. EF Core, both providers, through ToQueryString() and so without a database: the contains operator on a non-string column reaches the provider as a cast, CONVERT(varchar(11), [u].[Id]) LIKE on SQL Server and u."Id"::text LIKE on PostgreSQL. Each test also covers starts with and ends with, which produce the identical shape, so the four operators cannot drift apart. A fallback the provider could not translate would have been worse than the exception it replaced, which is why this is worth pinning rather than asserting in prose. Elasticsearch already matched a non-string field's text for all four operators, which is what the LINQ builder now does too, so the equivalent test there records the agreement between the two. Also tightens the SQL assertions: '!*' produces NOT LIKE rather than LIKE, and the first version of these tests passed only because the assertion was loose enough not to notice.
Fixes #332
The bug
2.18.0 added
IsValid(out List<string> validationErrors)forIGridifyFiltering, but nothing equivalent forIGridifyOrderingorIGridifyQuery.IGridifyQueryinherits from both, so with noIGridifyQueryoverload to bind to, the error collecting call on aGridifyQueryresolved to the filtering extension and validatedFilteronly.Two overloads of the same method, on the same object, disagreed — verified on
master:That is the shape that bites: code using the error-reporting overload to build a validation response waves a bad
OrderBystraight through, and the failure resurfaces later as a runtime error.The fix
Two new overloads, mirroring the existing filtering one:
Field 'x' is not mapped, the same wording the filtering validation already uses, and captures the message of theGridifyOrderingExceptionthat a malformed ordering throws ("Age bogus"→Ordering validation error: Invalid keyword. expected 'desc' or 'asc').Filter. The booleanIGridifyQueryoverload keeps its&&, since it only has to produce the verdict.IGridifyOrdering.IsValid(mapper)now delegates to the new overload and discards the errors, exactly as the filtering one already did.After:
Falsewith["Field 'ageee' is not mapped"], agreeing with the boolean overload.One detail worth knowing
SyntaxTree.ParseOrderingsis a lazy iterator that throws part way through enumeration, so theforeachhas to stay inside thetry—orders.Any(...)outside it would let the exception escape. Errors collected from fields before the throw are kept, whichIsValid_WithErrors_OnOrdering_WithErrorsBeforeAMalformedOrdering_KeepsBothpins.Tests
Gridify.Tests464, full solution 743, all six TFMs build.The one that matters is a theory asserting the two overloads never disagree, across good, bad and null combinations of
FilterandOrderBy— that property is the bug. Plus a theory covering ordering shapes (desc, multiple fields, the?/!null-check suffixes, empty, whitespace, null, unmapped), the malformed-ordering message, error concatenation from both parts, and the auto-generated mapper path.Three further validation gaps closed
Cases where
IsValidreported a query valid and the builder then threw. Each was found by running the verdict against realApplyFiltering/ApplyOrderingcalls, and each is now checked the way the builder checks it.AllowNullSearchoff on the mapper, on globally,Rank=nullArgumentExceptionAllowNullSearchon the mapper, off globally,Rank=nullOrderBy = "Age?"on anintGridifyOrderingExceptionOrderBy = "Name?"on astringGridifyOrderingExceptionFilter = "Age=*5"on anintArgumentExceptionAllowNullSearchwas read fromGridifyGlobalConfigurationwhileBaseQueryBuilder.BuildQueryreadsmapper.Configuration, so a mapper built with a different value validated under the wrong rule — wrong in both directions. One line.?/!ordering suffixes order by the member'sHasValue, whichLinqSortingQueryBuilder.GetOrderExpressioncan only build for aNullable<>member — note a reference type does not qualify either, soName?on astringthrows too. Ordering validation previously only checked that the field was mapped.Like/NotLikecalledstring.Containson the member itself, soAge=*5threw while the siblingAge^3worked. Fixed in the builder, not in validation — see below.One existing theory case asserted
"IntProperty?, StringProperty!"was valid — it never was at execution time, so it now covers nullable members instead.The contains operator is fixed in the builder, not in validation
LikeandNotLikecalledstring.Containson the member, soAge=*5on anintthrewArgumentException— whileAge^3andAge$5worked, becauseStartsWithandEndsWithhave aToString()fallback.Likewas simply missing it.An earlier revision of this PR made validation reject the operator on non-string fields. That was the wrong side: validation exists to predict what the builder does, so making it stricter than the builder hides the gap instead of closing it.
LikeandNotLikenow fall back toToString()exactly as the two cases immediately below them do, and the validation rejection is gone.The risk with a
ToString()fallback is a provider that cannot translate it — that would be worse than the exception it replaced. So it is pinned by tests in every builder, not asserted in prose:Id=*5becomesCONVERT(varchar(11), [u].[Id]) LIKE @pIssue332ContainsOnNonStringTestsu."Id"::text LIKE @pIssue332ContainsOnNonStringPostgreSqlTests{"wildcard":{"MyInt":{"value":"*5*"}}}ApplyFiltering_WhenTextOperatorIsUsedOnNonStringField_ShouldMatchItsTextmember.ToString().Contains("5")ApplyFiltering_WithContainsOperatorOnANonStringField_ComparesItsTextThe EF tests use
ToQueryString(), so they need no database and run in CI. Each covers^and$alongside=*/!*, since all four produce the identical SQL shape and should not drift apart.Elasticsearch already behaved this way for all four operators, so this change makes the LINQ builder agree with it rather than introducing a divergence.
Also verified unchanged for string members, for the case-insensitive
/isuffix, and withEntityFrameworkCompatibilityLayeron. A nullable member with no value has empty text and simply does not match, rather than throwing.Two limitations remain, both shared with
StartsWith/EndsWithand neither new:When=*2024on aDateTimecannot match and is still reported invalid;/isuffix only lowercases a string member, so it has no effect on the text of a non-string one.Still not validated
None of these can be checked without either an
IQueryablein hand or guessing at another subsystem's rules, so they are left alone and listed for separate issues:Select(...)projection. Validates true, throwsInvalidOperationException.ExtractPropertyTypereturns null for those bodies, so the value is never checked at all."null", which the builder nullifies but validation does not model.ElasticsearchQueryBuilderemits the raw token for most operators, so its behaviour differs from the LINQ builder this validation mirrors.Notes
IGridifyQuery-typed value now binds to the query overload and so validates more — which is the point of the issue, but it does mean a query with a badOrderBythat used to report valid will now report invalid.IsValidguide gains theOrderByexample, the per-part overloads, and a note that the error collecting overload covers both parts.An unmapped field is reported once
Naively concatenating the two lists reported the identical message twice when the same field was unmapped in both parts, and ordering on the same unmapped field twice did the same. The filtering side already reported one error per distinct field, so ordering now matches it: the ordering overload skips a message it has already collected, and the query overload appends only what the filter did not already report. Distinct fields are still reported separately.
Filter/OrderBynope=1/nopenope, nopenope, nope descaaa=1/bbb