Skip to content

fix(validation): consider mapper convertors in IsValid (#337) - #345

Merged
alirezanet merged 1 commit into
masterfrom
fix/337-isvalid-convertors
Sep 17, 2026
Merged

alirezanet merged 1 commit into
masterfrom
fix/337-isvalid-convertors

Conversation

@alirezanet

@alirezanet alirezanet commented Sep 17, 2026 •

Copy link
Copy Markdown
Owner

Fixes #337

The bug

IsValid compared the raw filter value against the mapped property's CLR type and never looked at the map's convertor, while BaseQueryBuilder.BuildQuery runs the convertor first and only then converts the type.

So validation and execution disagreed: a filter the query builder happily executes was reported invalid.

var mapper = new GridifyMapper<Entity>()
   .AddMap("createdOn", q => q.DateProperty, ConvertRelativeDateTime); // understands "d-2"

var query = new GridifyQuery { Filter = "createdOn>d-2" };

source.ApplyFiltering(query, mapper); // works, returns the right rows
query.IsValid(mapper);                // false  <-- the bug

The fix

The convertor runs before the type check. A string result goes through the existing type check - the only case BuildQuery converts. Any other result becomes the value of the condition directly, so the question is simply can the builder use it, and CanBeUsedAsValue answers that by calling the builder's own value machinery instead of reproducing its rules:

value path what validation calls
plain LINQ Expression.Constant(value, propertyType) - needs the exact type, rejects null for a non-nullable property
EntityFrameworkCompatibilityLayer GridifyTypeBuilder.CreateNewObject(...) - assigns through reflection, so it widens some types

A convertor that throws is a validation error; the builder does not catch it either. A CompositeGMap is ORed over all of its expressions, so the value must work for every one, not just the first that IGMap.To exposes.

Why it calls the real code instead of checking types itself

Two earlier revisions tried to model the builder's rules and both got them wrong, reporting filters as valid that throw ArgumentException when the query is built:

  1. every non-string result through a converted.ToString() round-trip;
  2. every non-string result under the EF layer, on the premise that reflection accepts anything.

That premise is false. PropertyInfo.SetValue widens int→long and int→enum and takes null for a non-nullable property, but rejects long→int, double→decimal, int→decimal, DateTime→int and Guid→int. Running the real path removes the guesswork, and it is less code than the rules it replaces.

Verified agreement

IsValid_WithConvertor_AgreesWithWhetherTheQueryCanBeBuilt asserts the expected verdict and that the verdict equals whether ApplyFiltering can actually build the query, over 30 combinations spanning both value paths:

convertor → property plain LINQ EF layer
long→long, int→int, enum→enum, DateTime→DateTime valid valid
int→long, int→enum invalid valid
null→int?, null→string valid valid
null→int, null→DateTime invalid valid (stores the default)
long→int, double→decimal, int→decimal, DateTime→int, Guid→int invalid invalid
a parsable string → int valid valid
an unparsable string → int invalid invalid

Behaviour changes (for the release note)

  • A map whose convertor throws for the given value used to validate as true and now reports false. Those filters threw at execution anyway.
  • A convertor result the builder cannot use now reports false instead of true.
  • IsValid now invokes the map's convertor. A convertor with side effects, or an expensive one, runs during validation as well as during filtering. IsValid_WithConvertor_InvokesItOncePerValue pins the count to what ApplyFiltering does.
  • Under the EF layer, validation also warms GridifyTypeBuilder's type cache, which the builder was going to populate anyway.
  • Maps without a convertor are untouched: the whole block sits behind if (gMap.Convertor != null).

Tests

Full solution: 675 passing (Gridify.Tests 410, Gridify.Elasticsearch.Tests 213, EF integration 11 / 12 / 29). Builds for all six TFMs including netstandard2.0.

Known limitations, deliberately not addressed here

Each of these is a disagreement between IsValid and execution that exists on master too, is not about convertors, and would change verdicts for maps without a convertor. Worth separate issues.

  • Like / NotLike on a non-string property validates true and always throws - IsValid("Int=*5") on an int property is true on master and here. Considering convertors widens the reach of this hole to three more inputs (a convertor map on a non-string property with =* / !*), which previously happened to be rejected because the raw value failed the type check. Fixing it properly means validating operator/property-type compatibility for every map.
  • Nested-collection and indexed maps get no value validation at all, convertor or not: ExtractPropertyType returns null for a Select(...) or indexer body, so the loop skips the value entirely.
  • AllowNullSearch is read from the wrong place: IsValid reads GridifyGlobalConfiguration.AllowNullSearch while BuildQuery reads mapper.Configuration.AllowNullSearch, so a per-mapper setting validates under the wrong rule - in both directions.
  • Elasticsearch: ElasticsearchQueryBuilder uses the convertor's result only for type dispatch and emits the raw token for most operators, so a convertor map can throw FormatException or silently query the unconverted value. master's stricter IsValid happened to reject some of those filters. The honest fix is in that builder; its 213 tests pass unchanged here.
  • No version bump - following feat(mapper): add case-insensitive per-map override #336, that is left to the release.

@alirezanet
alirezanet force-pushed the fix/337-isvalid-convertors branch from 7441878 to 9c79e2c Compare September 17, 2026 10:30
Repository owner deleted a comment from what-the-diff Bot Sep 17, 2026
@alirezanet
alirezanet force-pushed the fix/337-isvalid-convertors branch 2 times, most recently from fc4edac to 06cdb4e Compare September 17, 2026 11:04
IsValid validated the raw filter value against the mapped property type and
ignored the map's convertor, while BaseQueryBuilder runs the convertor first
and only then converts the type. A filter the query builder executes fine was
therefore reported as invalid, for example a "d-2" relative date on a DateTime
property behind a custom datetime parser.

The convertor now runs before the type check. A string result goes through the
existing type check, the only case the builder converts. Any other result is
used as the value of the condition, so the question is whether the builder can
use it, and CanBeUsedAsValue answers that by calling the builder's own value
machinery rather than reproducing its rules:

- Expression.Constant(value, propertyType) for plain LINQ, which needs the
  exact type and rejects null for a non-nullable property
- GridifyTypeBuilder.CreateNewObject for the Entity Framework compatibility
  layer, which assigns through reflection and so widens some types

Two earlier revisions of this change tried to reproduce those rules instead and
both got them wrong, reporting filters as valid that throw when the query is
built: first every non-string result through a ToString round trip, then every
non-string result under the EF layer, on the mistaken premise that reflection
accepts anything. Reflection widens int into long and int into an enum, and
takes null for a non-nullable property, but rejects long into int, double into
decimal, int into decimal, DateTime into int and Guid into int. Running the
real code path removes the guesswork.

A composite map is ORed over all of its expressions, so the value has to work
for every one of them, not just the first that IGMap.To exposes.

A convertor that throws is a validation error, the builder does not catch it
either. Validation messages keep quoting the value the caller sent rather than
what the convertor produced, since those messages are the payload an API
returns.

IsValid_WithConvertor_AgreesWithWhetherTheQueryCanBeBuilt pins the verdict to
whether ApplyFiltering can build the query across thirty convertor-result and
property-type combinations, on both value paths.

Docs: the IsValid guide gains a section on convertor maps, including the note
that IsValid now invokes the convertor.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

IsValid does not consider mapper's convertors

1 participant