fix(parser): correct parser and value-conversion bugs - #114
Merged
Merged
Conversation
WebApplication.CreateBuilder watches appsettings files for changes. On macOS the FileSystemWatcher start can hang, and then the integration run hangs before the first test. The tests never change these files at run time, so the fixture turns off config reload.
…test can_filter_with_property_to_property_child_properties filtered all recipes with Author.Name == Title. Other tests write recipes with random titles and random author names to the same database. When one pair matched, the test got two rows and failed. The test now queries only the two recipes that it inserts.
The parser and conversion checks need more seed data and settings. Each recipe now has a Sku, a Serving text with commas, and a ServeTime with fractional seconds. The custom-operation preset adds sku_is, and a new hidden-price preset maps a prevented Price property to cost. The --culture flag runs the driver in a different thread culture, and the output JSON records the culture and the time zone.
Operator aliases (for example eq and and) were applied with a regex over the whole filter string before parsing. The regex also changed text inside quoted values, so Title eq "salt and pepper" became Title == "salt && pepper" and matched nothing. The comparison and logical operator parsers now read the configured aliases directly. An alias still matches without regard to case and must be followed by whitespace or the end of the input. Query names set with HasQueryName still resolve when an alias operator follows them.
Number values were read with the decimal separator of the current culture only. Under de-DE or fr-FR, Rating > 4.5 failed to parse, so the same filter worked on one server and failed on another. The number grammar now takes the longer of the invariant match and the current culture match. A '.' decimal point parses in every culture, and a value that parsed before gives the same result. List numbers always use the '.' decimal point because ',' separates the items. The literal checks in IsPropertyPath and the count operator value accept both cultures.
The in and not-in operators split the list value on every comma after the quotes were removed. Serving ^^ ["Warm, with syrup"] became the two items Warm and with syrup, so it matched nothing, and !^^ matched every row. The list grammar now escapes each item before it joins the items, and the value and enum conversions split on unescaped commas only. Each item is still trimmed, as before.
Filter values are now query parameters. Npgsql rejects a DateTimeOffset parameter with a non-zero offset, so a filter such as `SpecificDate == 2024-01-15T10:00:00+02:00` threw on Postgres. The parser now converts the value to the same instant in UTC.
An unquoted DateTime with a fraction and a zone, for example 2024-01-15T08:00:00.500Z, failed to parse because the grammar read the zone before the fraction. An unquoted TimeOnly with a fraction also failed to parse, and a quoted TimeOnly lost a fraction with fewer than three digits, so 08:30:00.5 became 08:30:00. The grammar now reads up to seven fraction digits before the zone, the time grammar accepts a fraction, and the TimeOnly value keeps the milliseconds and microseconds of the parsed time.
A property-to-property comparison widens an int property to decimal with a Convert node. The equals and not-equals operators then converted every Convert node to bool, so 'Age == Rating' threw. The bool conversion now applies only to a derived property value boxed to object.
The case-sensitive @=, _=, and _-= operators and their negations called the string method on a null property. In memory, this threw a NullReferenceException. These operators now add the same null check as the case-insensitive operators, so a null property does not match @=, _=, or _-= and matches their negations. On Postgres, the results do not change.
The public ComparisonOperator factories accepted a usesAll argument but did not give it to the constructor. An operator from a factory always matched any item of a collection, never every item. The factories now pass usesAll through.
The sort parser split each clause on every white-space character, so 'Age desc' gave an empty direction and threw ArgumentException. The parser now ignores empty parts, so extra spaces or a tab before the direction work.
No test used `#<=` or `ApplyQueryKit(QueryKitData)`, and no test asserted the returned rows for `^$`. The new tests assert the expression and the returned rows for `#<=`, the returned rows for `^$`, `^$*`, and `%^$`, and apply a filter, a sort, and a query name through QueryKitData on memory and Postgres.
pdevito3
force-pushed
the
fm/qk-parser-conversion-bugs
branch
from
September 30, 2026 04:21
4e00793 to
253370f
Compare
This was referenced Sep 30, 2026
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
1 participant
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.
Summary
This PR corrects the parser and value-conversion bugs from the QueryKit review (PR 3). Each finding has its own commit, a regression test, and before and after proof from the
verify-querykitskill. The PR will be rebase-merged, so every commit passes the unit and integration tests.This PR has no breaking changes for a v1.14.2 consumer. Each fix changes only an input that threw, or that gave a result that no consumer can use. Five fixes from the first version of this PR changed v1.14.2 behavior. They moved out of this PR, and each one will get its own follow-up PR:
nullwith a case-sensitive string operator throwsQueryKitParsingException.QueryKitExceptionsubtypes.!^$(does-not-have) excludes collections that have the value.The strict invariant number grammar also moved out. This PR keeps a smaller number fix that does not change a v1.14.2 result.
Four kept fixes change a v1.14.2 result for an input that did not work as the client wrote it. Review them as judgment calls:
Title eq "salt and pepper"compared with"salt && pepper". It now compares with"salt and pepper".Title ^^ ["a, b"]was a list of two items. It is now one item.TimeOnlyvalue lost a fraction with fewer than three digits, so"08:30:00.5"was08:30:00. It is now08:30:00.5.ComparisonOperator.EqualsOperator(usesAll: true)and the other 23 factories ignoredusesAll. They now use it.The PR also contains these items:
DateTimeOffsetvalue with a non-zero offset failed on Postgres.#<=,^$, andApplyQueryKit(QueryKitData).--cultureflag for theverify-querykitharness.Release notes
No action is necessary to upgrade. These inputs now work:
4.5parses as a number in every culture. In a culture with a decimal comma,4,5keeps its v1.14.2 result.^^or!^^value stays in the value.DateTimeOffsetvalue with a non-zero offset works on Postgres.DateTime,DateTimeOffset, andTimeOnly.Age == Rating(int and decimal properties) works.NullReferenceExceptionon anullproperty in memory.ComparisonOperatorfactories keep theusesAllvalue.Age desc) works.Proof
The proof comes from
verify-querykit. Each drive runs the same input on two targets: LINQ to Objects (memory) and EF Core on Postgres. The "before" runs used theQueryKit/source ofmain(d54de85), and the "after" runs used the code with the fix. The machine time zone is EET, and the culture is en-US unless the table gives a different value.1. Operator aliases changed quoted text
fix(parser): resolve operator aliases in the grammar. Config:word-operators(eq,and,oras aliases).Directions eq "Whisk and fry"Pancakes/ 1PancakesTitle eq "Pancakes" or Title eq "Beef Stew"The second row shows that the logical aliases still work.
2. A decimal point failed in a culture with a decimal comma
fix(parser): accept a decimal point in every culture. The number grammar now takes the longer match of the invariant grammar and the current-culture grammar. A list number always uses., because,separates the list items. Culture: de-DE.Price > 4.5ParsingException/ParsingExceptionBeef Stew/ 1Beef StewPrice > 4,54,5keeps its v1.14.2 value, 45. A strict invariant grammar that rejects4,5is a breaking change, so it moved to a follow-up PR.3. In and not-in lists split on commas inside quoted values
fix(parser): keep commas inside quoted list values.Serving ^^ ["Warm, with syrup"]Pancakes/ 1PancakesServing !^^ ["Warm, with syrup"]Pancakes) / 3 rowsEach item is still trimmed, as in v1.14.2.
Title ^^ [" Pancakes ", "Beef Stew "]gives 2 rows / 2 rows before and after.Regression from #110: DateTimeOffset values with an offset
fix(parser): send date time offset values as utc. After #110, filter values are parameters. Npgsql rejects aDateTimeOffsetparameter with a non-zero offset. Proof: the integration testdate_time_offset_value_with_offset_matches_same_instantfailed before the fix withCannot write DateTimeOffset with Offset=02:00:00 to PostgreSQL type 'timestamp with time zone', only offset 0 (UTC) is supported.It passes after the fix.4. Fractional seconds were lost
fix(parser): keep fractional seconds in date and time values.ServeTime == "08:30:00.5"Pancakes/ 1PancakesServeTime == 08:30:00.5ParsingException/ParsingExceptionPancakes/ 1PancakesServeTime == "12:15:30.25"Salt Bread/ 1Salt BreadCreatedAt == 2024-01-15T08:00:00.000ZParsingException/ParsingExceptionPancakes/ 1Pancakes5.
Age == Rating(int and decimal) threwfix(operators): compare int and decimal properties with equals.Rating == PriceParsingException/ParsingExceptionRating != PriceParsingException/ParsingException6. Case-sensitive string operators threw on a null property
fix(operators): handle null in case-sensitive string operators.DirectionsisnullforPlain Water.Directions @= "fry"NullReferenceException/ 1Pancakes/ 1PancakesDirections _= "Whisk"NullReferenceException/ 1Directions _-= "fry"NullReferenceException/ 1Directions !@= "fry"NullReferenceException/ 3Directions !_= "Whisk"NullReferenceException/ 3Directions !_-= "fry"NullReferenceException/ 3Title @= nullArgumentNullException/ 0 rowsArgumentNullException/ 0 rows (unchanged)7. The operator factories ignored
usesAllfix(operators): pass usesAll through the operator factories. A filter string cannot reach this API, so a consumer-side probe calls the public factories.ComparisonOperator.EqualsOperator(usesAll: true).UsesAll = False, and the same for all 24 factories.UsesAll = Truefor all 24 factories.8. A sort direction after more than one space threw
fix(sort): read the sort direction after more than one space.Rating descArgumentException/ArgumentExceptionRating sidewaysArgumentException/ArgumentExceptionArgumentException/ArgumentException(unchanged)A leading space or a tab before the direction already worked, and
Title,already threwSortParsingException.Tests
test(filter): query only its own recipes in the property-to-property test.can_filter_with_property_to_property_child_propertiesqueried all recipes. Other tests make recipes with random titles and random author names, and a pair can matchAuthor.Name == Title. The test now queries only its own two recipes. Main already fixes the other flaky tests (random property names and states) in 72af1da.test(integration): turn off config reload in the test fixture.WebApplication.CreateBuilderwatches the appsettings files for changes. On macOS,FileSystemWatchercan hang on start, and then the integration run hangs before the first test. A stack dump of a hung run showedPhysicalFilesWatcher.TryEnableFileSystemWatcherinFileSystemWatcher.Start. The fixture now passes--hostBuilder:reloadConfigOnChange=false. A temporary guard showed that no config source reloads on change with this flag.test(filter): cover count less-than-or-equal, has, and query kit data. New tests for#<=(expression and Postgres rows), for the rows that^$,^$*, and%^$return, and forApplyQueryKit(QueryKitData)(memory and Postgres, with a filter, a sort, and a query name). Drive:Ingredients #<= 1gives 1Plain Wateron both targets.test(verify-querykit): add fixtures and a culture flag to the harness. The harness has newSku,Serving, andServeTimecolumns, asku_iscustom operation, ahidden-pricepreset, and a--cultureflag.Verification
dotnet test QueryKit.UnitTests/anddotnet test QueryKit.IntegrationTests/pass at every commit (git rebase --execonorigin/main).