From fbd6dcfd0dd6ec4fb2b968c42026e8f0b85608b6 Mon Sep 17 00:00:00 2001 From: Maksym Kacharaba Date: Sat, 7 Nov 2026 13:12:36 +0200 Subject: [PATCH] fix: reject unsendable query types, and expand query objects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `[Query] SearchFilter filter` compiled clean, raised no diagnostic, and sent `?filter=SearchFilter%20%7B%20State%20%3D%20open...` — the record's ToString(), URL-encoded. A non-record class sent its type name. The server received garbage and nothing anywhere said so. RequestValueFormatter's ToString() fallback is right for an unknown IFormattable, but it should never have been reachable by a type the generator could see was wrong. A multi-field filter object is the most natural thing a user tries, and this violated the library's own rules twice over: prefer a build error over a runtime one, and never guess what goes in a URL. So the diagnostic rejects what cannot be sent (RXH022, RXH023), and the expansion makes the natural thing the working thing: one query key per property, resolved by the generator at compile time. That is what keeps it reflection-free and trimming-safe, and why only a generator can offer it — the AOT smoke test now drives it and still publishes with zero trim warnings. Decisions worth knowing: - Field names are the property names exactly as written, not camelCased. Nothing in this library transforms a name — a [Query] parameter already sends its parameter name verbatim — so transforming properties would make the two rules disagree. [Query("per_page")] on a property renames it, which is the same escape hatch parameters have. - Nested objects are refused, not flattened. ASP.NET Core spells a nested key "filter.state" and OpenAPI's deepObject spells it "filter[state]"; picking one silently would be the same guess this change exists to stop. One level covers the flat filter and pagination objects that motivate the feature, and refusing leaves room to add nesting later behind an explicit opt-in. - Inherited properties are expanded, since a filter sharing a pagination base is the obvious shape and ignoring the base would drop fields silently. - Null properties are omitted and a null object contributes nothing, matching how a null [Query] parameter already behaves. Access uses `?.` except on a non-nullable struct, where it would not compile and null is impossible. - Dictionaries keyed by string contribute their own keys. They are the one query shape whose keys are unknown until run time, so they are spread in from a runtime helper rather than emitted field by field — which is also why QueryValues is now a collection expression rather than an array. The generator's sendable-type check mirrors RequestValueFormatter's switch, including Uri, which is neither a special type nor IFormattable and so has to be named in both. The two have to agree or the check is a lie. 1461 tests (from 1368); build clean with warnings-as-errors, format clean, native AOT publishes with no trim warnings and runs. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 22 +- docs/defining-a-client.md | 52 +++ docs/diagnostics.md | 42 +++ .../Attributes/QueryAttribute.cs | 17 +- .../AnalyzerReleases.Unshipped.md | 2 + .../ClientGenerator.cs | 319 +++++++++++++++++- src/Runax.Http.SourceGenerator/Diagnostics.cs | 11 + src/Runax.Http.SourceGenerator/Emitter.cs | 59 +++- src/Runax.Http.SourceGenerator/Model.cs | 50 ++- src/Runax.Http/README.md | 12 + src/Runax.Http/Requests/QueryFields.cs | 45 +++ tests/Runax.Http.AotSmoke/AotClient.cs | 22 ++ tests/Runax.Http.AotSmoke/Program.cs | 8 + .../DiagnosticTests.cs | 139 ++++++++ .../Contracts/ISearchClient.cs | 71 ++++ tests/Runax.Http.Tests/QueryObjectTests.cs | 245 ++++++++++++++ 16 files changed, 1097 insertions(+), 19 deletions(-) create mode 100644 src/Runax.Http/Requests/QueryFields.cs create mode 100644 tests/Runax.Http.Tests/Contracts/ISearchClient.cs create mode 100644 tests/Runax.Http.Tests/QueryObjectTests.cs diff --git a/CHANGELOG.md b/CHANGELOG.md index 78baa8a..39de390 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,7 +27,20 @@ All packages in the repository are versioned together. - Unbounded by default. Adding a budget nobody asked for would change the behaviour of every existing client on upgrade. - `Timeout`'s documentation now says "per attempt", which it never did. - +- **Query objects.** A whole object as a `[Query]` parameter is expanded to one key per property, + resolved by the generator at compile time — so it needs no reflection and survives trimming, + which is why only a generator can offer it. Field names are the property names exactly as + written, because nothing in this library silently transforms a name; `[Query("per_page")]` on a + property renames it. Inherited properties are included, null properties are omitted, collection + properties repeat their key, and values go through the same formatter as every other query value + so a date spells the same thing either way. + - **Nested objects are refused** rather than flattened. ASP.NET Core spells a nested key + `filter.state` and OpenAPI's `deepObject` spells it `filter[state]`; picking one silently would + be the same guess the new diagnostics exist to prevent. +- **Dictionary query parameters.** A sequence of `KeyValuePair` contributes its own keys + — the one query shape whose keys are not known until run time, and so the one resolved there. +- Two new diagnostics. `RXH022` rejects a `[Query]` type the runtime cannot turn into text; + `RXH023` names the property of an otherwise-expandable object that cannot be sent. - **`[Retry]` on a method**, for the one flaky endpoint on an otherwise healthy API. Surfaces an attempt count, `DelayMilliseconds` (milliseconds because a `TimeSpan` cannot be an attribute argument) and `AllowNonIdempotent`. It refines the client's registered settings rather than @@ -99,12 +112,17 @@ All packages in the repository are versioned together. ### Fixed +- **A `[Query]` object no longer sends `ToString()` to your server.** `[Query] SearchFilter filter` + compiled clean, raised no diagnostic, and sent + `?filter=SearchFilter%20%7B%20State%20%3D%20open...` — the record's `ToString()`, URL-encoded. A + non-record class sent its type name. `RequestValueFormatter`'s `ToString()` fallback is right for + an unknown `IFormattable` but should never have been reachable by a type the generator could see + was wrong, and a multi-field filter object is the most natural thing to try. - **A rebuilt request no longer changes underneath a transport.** `TransportRequest` is documented as immutable and a transport is entitled to hold on to one, but the pipeline handed over its live header list — so on a retry the next attempt's header changes rewrote a request that had already been sent. It showed up as the first attempt appearing to carry the second attempt's token, and was caught by the test written to prove token refresh worked. - - **`BodyFormat.UrlEncoded` and `BodyFormat.Multipart`** are now emitted, completing the body formats the attribute has always advertised. A form body comes from a sequence of key/value pairs — a `Dictionary`, or `IEnumerable>` for values that diff --git a/docs/defining-a-client.md b/docs/defining-a-client.md index ac0e686..53fbf62 100644 --- a/docs/defining-a-client.md +++ b/docs/defining-a-client.md @@ -68,6 +68,58 @@ default header. A null **path** value is an error at run time. `/repos//issues` addresses something other than what the caller named, and would most likely 404 a long way from the cause. +### Query objects + +A whole object works as a `[Query]` parameter, expanded to one key per property: + +```csharp +public sealed record SearchFilter +{ + public string? State { get; init; } + + [Query("per_page")] + public int PerPage { get; init; } = 30; + + public IEnumerable? Labels { get; init; } +} + +[Get("/search")] +Task SearchAsync([Query] SearchFilter filter, CancellationToken ct = default); +``` + +sends `?State=open&per_page=50&Labels=bug&Labels=docs`. + +The generator resolves the properties at compile time, so this stays reflection-free and survives +trimming — which is why only a generator can offer it. + +- **Field names are the property names, exactly as written.** Nothing in this library transforms a + name, so `State` sends `State`. `[Query("per_page")]` on the property renames it, the same way it + renames a parameter. +- **Inherited properties are included**, so a filter sharing a pagination base type works. +- **Null properties are omitted**, matching a null parameter. A null object contributes nothing at + all. +- **Collection properties repeat their key**, matching a collection parameter. +- **Values go through the same formatter**, so a `DateTimeOffset` property spells exactly what a + `DateTimeOffset` parameter would. +- **Nested objects are refused** ([`RXH023`](diagnostics.md#rxh023)). There is no single convention + for naming a nested field — ASP.NET Core spells it `filter.state`, OpenAPI's `deepObject` spells + it `filter[state]` — so flattening would have to guess. Flatten the type, or pass the inner + object as its own parameter. + +A **dictionary** keyed by string works too, contributing its own keys: + +```csharp +[Get("/search")] +Task SearchAsync([Query] Dictionary filters, CancellationToken ct = default); +``` + +That is the one query shape whose keys are not known until run time, so it is the one resolved +there rather than by the generator. + +A `[Query]` type the runtime cannot turn into text is [`RXH022`](diagnostics.md#rxh022), a build +error. Before that diagnostic existed, such a type fell through to `ToString()` and the server +received `?filter=SearchFilter { State = open, Page = 2 }` with no warning anywhere. + ### Value formatting Every value — path, query or header — is formatted the same way, invariantly: diff --git a/docs/diagnostics.md b/docs/diagnostics.md index ba59afc..26abd52 100644 --- a/docs/diagnostics.md +++ b/docs/diagnostics.md @@ -38,6 +38,8 @@ removal added `RXH019` rather than repurposing the number. | [RXH019](#rxh019) | Error | A body parameter typed wrong for its format | | [RXH020](#rxh020) | **Warning** | A `[Retry]` that will not retry anything | | [RXH021](#rxh021) | Error | A `[Retry]` with fewer than one attempt | +| [RXH022](#rxh022) | Error | A `[Query]` type that cannot be sent | +| [RXH023](#rxh023) | Error | A query object property that cannot be sent | --- @@ -306,6 +308,46 @@ Attempts include the first, so the lowest meaningful value is `1`. **Fix:** use `[Retry(1)]` if you meant "do not retry this method even though the client is configured to" — which is a real thing to want. Otherwise give it the attempt count you intended. +### RXH022 + +**This parameter type cannot be sent as a query value.** + +```csharp +[Get("/search")] +Task SearchAsync([Query] Opaque value); // ✗ +``` + +A query value is formatted, never serialized, so the runtime can only send a type it recognises: a +string, number, bool, enum, date, `Guid` or `Uri`, anything `IFormattable`, a collection of those, +a dictionary keyed by string, or an object whose properties are all of those. + +Without this diagnostic such a type fell through to `ToString()` and the server received +`?filter=SearchFilter { State = open, Page = 2 }` — garbage, with no warning. That is exactly the +silent-wrong-request this library exists to prevent. + +**Fix:** pass the scalars individually, pass a dictionary, or make the type a flat object whose +properties are all sendable — see [Query objects](defining-a-client.md#query-objects). + +### RXH023 + +**A query object property cannot be sent.** + +```csharp +public sealed record Inner(string State); +public sealed record Outer(Inner Nested, int Page); + +[Get("/search")] +Task SearchAsync([Query] Outer filter); // ✗ Nested +``` + +The parameter is expandable but one of its properties is not sendable. The most common cause is a +**nested object**, which is refused rather than flattened: ASP.NET Core spells a nested key +`filter.state`, OpenAPI's `deepObject` spells it `filter[state]`, and others differ again — so +flattening would have to guess, which is the thing [`RXH022`](#rxh022) exists to stop. + +**Fix:** flatten the type, pass the inner object as its own `[Query]` parameter, or drop the +property. + ## Suppressing The normal mechanisms apply — `#pragma warning disable RXH010`, a `` entry, or an diff --git a/src/Runax.Http.Abstractions/Attributes/QueryAttribute.cs b/src/Runax.Http.Abstractions/Attributes/QueryAttribute.cs index 18dd78a..d8dcd22 100644 --- a/src/Runax.Http.Abstractions/Attributes/QueryAttribute.cs +++ b/src/Runax.Http.Abstractions/Attributes/QueryAttribute.cs @@ -1,10 +1,21 @@ namespace Runax.Http; /// -/// Binds a parameter to a query string value. The parameter name is used unless -/// is set. Collections are expanded to repeated keys. +/// Binds a parameter to a query string value, or renames a property of an expanded query object. /// -[AttributeUsage(AttributeTargets.Parameter)] +/// +/// +/// On a parameter: the parameter name is used unless is set, and a collection is +/// expanded to repeated keys. +/// +/// +/// On a property: renames that field when its declaring type is used as a query object. A query +/// object's fields are otherwise named after their properties exactly as written, because nothing +/// in this library silently transforms a name — so this is how a C# PerPage reaches an API +/// that spells it per_page. +/// +/// +[AttributeUsage(AttributeTargets.Parameter | AttributeTargets.Property)] public sealed class QueryAttribute : Attribute { /// Initializes a new instance of the class. diff --git a/src/Runax.Http.SourceGenerator/AnalyzerReleases.Unshipped.md b/src/Runax.Http.SourceGenerator/AnalyzerReleases.Unshipped.md index 25da5c9..4f8c8e6 100644 --- a/src/Runax.Http.SourceGenerator/AnalyzerReleases.Unshipped.md +++ b/src/Runax.Http.SourceGenerator/AnalyzerReleases.Unshipped.md @@ -20,3 +20,5 @@ Rule ID | Category | Severity | Notes RXH019 | Runax.Http | Error | This parameter type cannot be sent in that body format RXH020 | Runax.Http | Warning | This [Retry] will not retry anything RXH021 | Runax.Http | Error | [Retry] needs at least one attempt +RXH022 | Runax.Http | Error | This parameter type cannot be sent as a query value +RXH023 | Runax.Http | Error | A query object property cannot be sent diff --git a/src/Runax.Http.SourceGenerator/ClientGenerator.cs b/src/Runax.Http.SourceGenerator/ClientGenerator.cs index 2c0ee95..7598a5a 100644 --- a/src/Runax.Http.SourceGenerator/ClientGenerator.cs +++ b/src/Runax.Http.SourceGenerator/ClientGenerator.cs @@ -328,11 +328,50 @@ private static ImmutableArray AnalyzeParameters( } } - parameters.Add(new ParameterModel( - parameter.Name, - ReadWireName(attribute) ?? parameter.Name, - binding, - typeName)); + var wireName = ReadWireName(attribute) ?? parameter.Name; + + if (binding == ParameterBinding.Query) + { + // A query value is formatted, never serialized, so the runtime can only turn it + // into text if the type is one it recognises. Left unchecked, an unrecognised type + // falls through to ToString() and the server receives "Filter { State = open }" — + // garbage, with no warning anywhere. + var shape = ClassifyQuery(parameter.Type, out var fields, out var problem); + + if (problem is not null) + { + diagnostics.Add(problem.Property is null + ? Diagnostic.Create( + Diagnostics.UnsupportedQueryParameterType, + Location(parameter), + parameter.Name, + method.ToDisplayString(), + parameter.Type.ToDisplayString(), + problem.Advice) + : Diagnostic.Create( + Diagnostics.UnsupportedQueryObjectProperty, + Location(parameter), + parameter.Name, + method.ToDisplayString(), + problem.Property, + problem.PropertyType, + problem.Advice)); + + continue; + } + + parameters.Add(new ParameterModel( + parameter.Name, + wireName, + binding, + typeName, + shape, + BuildQueryFields(parameter, fields))); + + continue; + } + + parameters.Add(new ParameterModel(parameter.Name, wireName, binding, typeName)); } return parameters.ToImmutable(); @@ -685,6 +724,276 @@ private static string Describe(ResponseModel response) return null; } + /// Why a query parameter cannot be sent, and what to do about it. + private sealed class QueryProblem + { + internal QueryProblem(string advice, string? property = null, string? propertyType = null) + { + Advice = advice; + Property = property; + PropertyType = propertyType; + } + + internal string Advice { get; } + + /// The offending property, when the parameter itself was fine. + internal string? Property { get; } + + internal string? PropertyType { get; } + } + + private const string QueryAdvice = + "A query value has to be something the runtime can turn into text: a string, number, bool, " + + "enum, date, Guid or Uri, anything IFormattable, a collection of those, a dictionary keyed " + + "by string, or an object whose properties are all of those."; + + /// + /// Decides how a [Query] parameter becomes query keys. + /// + /// + /// Order matters. A string is an IEnumerable<char> and a dictionary is an + /// IEnumerable<KeyValuePair<,>>, so the narrow cases are tested before the + /// general one or both would be mis-classified as collections. + /// + private static QueryShape ClassifyQuery( + ITypeSymbol type, + out ImmutableArray fields, + out QueryProblem? problem) + { + fields = ImmutableArray.Empty; + problem = null; + + if (IsSendableValue(type)) + return QueryShape.Scalar; + + if (IsStringKeyedSequence(type)) + return QueryShape.Dictionary; + + if (TryGetSequenceElement(type, out var element)) + { + if (IsSendableValue(element!)) + return QueryShape.Scalar; + + problem = new QueryProblem( + $"Its items are '{element!.ToDisplayString()}', which cannot be sent. " + QueryAdvice); + + return QueryShape.Scalar; + } + + var properties = ReadableProperties(type); + + if (properties.Length == 0) + { + problem = new QueryProblem(QueryAdvice); + return QueryShape.Scalar; + } + + foreach (var property in properties) + { + if (IsSendableValue(property.Type)) + continue; + + if (TryGetSequenceElement(property.Type, out var itemType) && IsSendableValue(itemType!)) + continue; + + // Nesting is refused rather than flattened. There is no single convention for the key + // of a nested field — ASP.NET Core spells it "filter.state", OpenAPI's deepObject + // spells it "filter[state]", and others differ again — so picking one silently would be + // the same guess this diagnostic exists to prevent. One level covers the flat filter + // and pagination objects that motivate the feature. + problem = new QueryProblem( + ReadableProperties(property.Type).Length > 0 + ? "A query object's properties cannot themselves be objects: there is no one " + + "convention for naming a nested field, so flattening would have to guess. " + + "Flatten the type, or pass the inner object as its own [Query] parameter." + : QueryAdvice, + property.Name, + property.Type.ToDisplayString()); + + return QueryShape.Object; + } + + fields = properties; + + return QueryShape.Object; + } + + /// + /// The public, readable, non-indexer instance properties of a type and its bases. + /// + /// + /// Inherited properties are included — a filter type with a shared pagination base is the + /// obvious shape, and ignoring the base would drop fields silently. Declaration order is + /// preserved, most-derived first, so the generated query string is stable across builds. + /// + private static ImmutableArray ReadableProperties(ITypeSymbol type) + { + if (type.TypeKind is not (TypeKind.Class or TypeKind.Struct or TypeKind.Interface)) + return ImmutableArray.Empty; + + var properties = ImmutableArray.CreateBuilder(); + var seen = new HashSet(StringComparer.Ordinal); + + for (var current = Unwrap(type); current is not null; current = current.BaseType) + { + if (current.SpecialType == SpecialType.System_Object) + break; + + foreach (var member in current.GetMembers().OfType()) + { + if (member.DeclaredAccessibility != Accessibility.Public + || member.IsStatic + || member.IsIndexer + || member.GetMethod is null + || !seen.Add(member.Name)) + { + continue; + } + + properties.Add(member); + } + } + + return properties.ToImmutable(); + } + + /// Whether RequestValueFormatter can turn this type into text. + /// Mirrors that formatter's switch; the two have to agree or this check is a lie. + private static bool IsSendableValue(ITypeSymbol type) + { + var bare = StripNullable(type); + + if (bare.TypeKind == TypeKind.Enum) + return true; + + switch (bare.SpecialType) + { + case SpecialType.System_String: + case SpecialType.System_Boolean: + case SpecialType.System_Char: + case SpecialType.System_SByte: + case SpecialType.System_Byte: + case SpecialType.System_Int16: + case SpecialType.System_UInt16: + case SpecialType.System_Int32: + case SpecialType.System_UInt32: + case SpecialType.System_Int64: + case SpecialType.System_UInt64: + case SpecialType.System_Single: + case SpecialType.System_Double: + case SpecialType.System_Decimal: + case SpecialType.System_DateTime: + return true; + } + + // Uri is handled explicitly by the formatter and is neither a special type nor + // IFormattable, so it has to be named here too. + if (bare.ToDisplayString() is "System.DateTimeOffset" or "System.DateOnly" or "System.TimeOnly" + or "System.TimeSpan" or "System.Guid" or "System.Uri") + { + return true; + } + + return bare.AllInterfaces.Any(i => i.ToDisplayString() == "System.IFormattable"); + } + + /// Whether this is a sequence of key/value pairs keyed by string. + private static bool IsStringKeyedSequence(ITypeSymbol type) + { + if (!TryGetSequenceElement(type, out var element)) + return false; + + return element is INamedTypeSymbol { Arity: 2 } pair + && pair.ConstructedFrom.ToDisplayString() + == "System.Collections.Generic.KeyValuePair" + && pair.TypeArguments[0].SpecialType == SpecialType.System_String; + } + + /// The element type of an IEnumerable<T>, excluding string. + private static bool TryGetSequenceElement(ITypeSymbol type, out ITypeSymbol? element) + { + element = null; + + if (type.SpecialType == SpecialType.System_String) + return false; + + var bare = StripNullable(type); + + if (bare is INamedTypeSymbol { Arity: 1 } self + && self.ConstructedFrom.ToDisplayString() == "System.Collections.Generic.IEnumerable") + { + element = self.TypeArguments[0]; + return true; + } + + foreach (var implemented in bare.AllInterfaces) + { + if (implemented is { Arity: 1 } + && implemented.ConstructedFrom.ToDisplayString() + == "System.Collections.Generic.IEnumerable") + { + element = implemented.TypeArguments[0]; + return true; + } + } + + return false; + } + + private static ITypeSymbol StripNullable(ITypeSymbol type) + => type is INamedTypeSymbol { OriginalDefinition.SpecialType: SpecialType.System_Nullable_T } nullable + ? nullable.TypeArguments[0] + : type; + + private static INamedTypeSymbol? Unwrap(ITypeSymbol type) + => StripNullable(type) as INamedTypeSymbol; + + /// + /// The access expression for each field of an expanded query object. + /// + /// + /// Null-conditional unless the parameter is a non-nullable struct, where it would not compile + /// and a null object is impossible anyway. Using ?. is also what makes a null object + /// contribute nothing: every field reads as null, and a null query value is already omitted. + /// + private static ImmutableArray BuildQueryFields( + IParameterSymbol parameter, + ImmutableArray properties) + { + if (properties.IsDefaultOrEmpty) + return ImmutableArray.Empty; + + var nullable = parameter.Type.IsReferenceType + || parameter.Type is INamedTypeSymbol + { + OriginalDefinition.SpecialType: SpecialType.System_Nullable_T + }; + + var accessor = nullable ? "?." : "."; + var fields = ImmutableArray.CreateBuilder(properties.Length); + + foreach (var property in properties) + { + fields.Add(new QueryFieldModel( + ReadPropertyWireName(property) ?? property.Name, + $"{parameter.Name}{accessor}{property.Name}")); + } + + return fields.ToImmutable(); + } + + /// A property's [Query("name")] rename, if it carries one. + private static string? ReadPropertyWireName(IPropertySymbol property) + { + foreach (var attribute in property.GetAttributes()) + { + if (attribute.AttributeClass?.ToDisplayString() == $"{AttributesNamespace}.QueryAttribute") + return ReadWireName(attribute); + } + + return null; + } + /// /// Whether this type can be sent in the given body format, and if not, what to do instead. /// diff --git a/src/Runax.Http.SourceGenerator/Diagnostics.cs b/src/Runax.Http.SourceGenerator/Diagnostics.cs index a6664b1..37e667b 100644 --- a/src/Runax.Http.SourceGenerator/Diagnostics.cs +++ b/src/Runax.Http.SourceGenerator/Diagnostics.cs @@ -140,6 +140,17 @@ internal static class Diagnostics "'{0}' declares [Retry({1})]. Attempts include the first, so the lowest meaningful value is " + "1 — which says this method must not be retried even though its client is configured to."); + internal static readonly DiagnosticDescriptor UnsupportedQueryParameterType = Error( + "RXH022", + "This parameter type cannot be sent as a query value", + "'{0}' on '{1}' is typed '{2}', which cannot be sent in a query string. {3}"); + + internal static readonly DiagnosticDescriptor UnsupportedQueryObjectProperty = Error( + "RXH023", + "A query object property cannot be sent", + "'{0}' on '{1}' is a query object, but its property '{2}' is typed '{3}', which cannot be " + + "sent in a query string. {4}"); + private static DiagnosticDescriptor Error(string id, string title, string format) => new(id, title, format, Category, DiagnosticSeverity.Error, isEnabledByDefault: true); diff --git a/src/Runax.Http.SourceGenerator/Emitter.cs b/src/Runax.Http.SourceGenerator/Emitter.cs index b37650c..9fab398 100644 --- a/src/Runax.Http.SourceGenerator/Emitter.cs +++ b/src/Runax.Http.SourceGenerator/Emitter.cs @@ -104,14 +104,7 @@ private static void EmitRequest(StringBuilder code, MethodModel method, string i .Where(parameter => parameter.Binding == ParameterBinding.Path) .Select(parameter => (Literal(parameter.WireName), parameter.Name))); - EmitPairs( - code, - indent, - "QueryValues", - "string, object?", - method.Parameters - .Where(parameter => parameter.Binding == ParameterBinding.Query) - .Select(parameter => (Literal(parameter.WireName), parameter.Name))); + EmitQueryValues(code, indent, method); var headers = method.ConstantHeaders .Select(header => (Literal(header.Key), Literal(header.Value))) @@ -279,6 +272,56 @@ private static string BuildCondition(ResponseModel response) return string.Join(" && ", tests); } + /// + /// Emits the query entries, which unlike the other collections can come from three shapes. + /// + /// + /// A collection expression rather than an array literal, because a dictionary argument + /// contributes an unknown number of entries and has to be spread in. The other two shapes are + /// fixed at compile time: a scalar is one entry, and an object is one per property, resolved by + /// the generator so nothing reflects over the type at run time. + /// + private static void EmitQueryValues(StringBuilder code, string indent, MethodModel method) + { + const string pair = "global::System.Collections.Generic.KeyValuePair"; + + var elements = new List(); + + foreach (var parameter in method.Parameters) + { + if (parameter.Binding != ParameterBinding.Query) + continue; + + switch (parameter.QueryShape) + { + case QueryShape.Object: + foreach (var field in parameter.QueryFields) + elements.Add($"new {pair}({Literal(field.WireName)}, {field.Access})"); + + break; + + case QueryShape.Dictionary: + elements.Add($"..{Runtime}.Requests.QueryFields.From({parameter.Name})"); + break; + + default: + elements.Add($"new {pair}({Literal(parameter.WireName)}, {parameter.Name})"); + break; + } + } + + if (elements.Count == 0) + return; + + code.AppendLine($"{indent} QueryValues ="); + code.AppendLine($"{indent} ["); + + foreach (var element in elements) + code.AppendLine($"{indent} {element},"); + + code.AppendLine($"{indent} ],"); + } + private static void EmitPairs( StringBuilder code, string indent, diff --git a/src/Runax.Http.SourceGenerator/Model.cs b/src/Runax.Http.SourceGenerator/Model.cs index d461d27..505e9b9 100644 --- a/src/Runax.Http.SourceGenerator/Model.cs +++ b/src/Runax.Http.SourceGenerator/Model.cs @@ -21,15 +21,57 @@ internal enum ParameterBinding CancellationToken } +/// How a [Query] parameter turns into query keys. +internal enum QueryShape +{ + /// One key, from the parameter itself. A collection repeats that key per item. + Scalar, + + /// One key per property, resolved at compile time. + Object, + + /// One key per entry, resolved at run time from the sequence's own keys. + Dictionary +} + +/// One field of an expanded query object. +/// +/// The access expression is built by the generator rather than the emitter, because whether a null +/// object is reachable — and therefore whether ?. is needed — is a fact about the parameter's +/// type that only the symbol knows. +/// +internal sealed class QueryFieldModel +{ + internal QueryFieldModel(string wireName, string access) + { + WireName = wireName; + Access = access; + } + + /// The query key this field travels under. + internal string WireName { get; } + + /// The expression that reads it, such as filter?.State. + internal string Access { get; } +} + /// What the generated method does with one of its parameters. internal sealed class ParameterModel { - internal ParameterModel(string name, string wireName, ParameterBinding binding, string typeName) + internal ParameterModel( + string name, + string wireName, + ParameterBinding binding, + string typeName, + QueryShape queryShape = QueryShape.Scalar, + ImmutableArray queryFields = default) { Name = name; WireName = wireName; Binding = binding; TypeName = typeName; + QueryShape = queryShape; + QueryFields = queryFields.IsDefault ? ImmutableArray.Empty : queryFields; } /// The C# parameter name, used when emitting the method signature and body. @@ -46,6 +88,12 @@ internal ParameterModel(string name, string wireName, ParameterBinding binding, /// The fully qualified parameter type, as written into the generated signature. internal string TypeName { get; } + + /// How this parameter turns into query keys. Only meaningful for a query binding. + internal QueryShape QueryShape { get; } + + /// The fields of an expanded query object, empty for every other shape. + internal ImmutableArray QueryFields { get; } } /// One [Response] declaration: a condition, and the type to use when it holds. diff --git a/src/Runax.Http/README.md b/src/Runax.Http/README.md index e498be2..fabd40b 100644 --- a/src/Runax.Http/README.md +++ b/src/Runax.Http/README.md @@ -42,6 +42,18 @@ Then inject `IRepositoryClient`. A parameter with no attribute binds to a path p same name; anything else needs an attribute, because guessing that an unbound parameter "must be a query parameter" is how a secret ends up in a URL. +A whole object works as a `[Query]` parameter and is expanded to one key per property, resolved at +compile time so it stays reflection-free: + +```csharp +[Get("/search")] +Task SearchAsync([Query] SearchFilter filter, CancellationToken ct = default); +// ?State=open&per_page=50&Labels=bug&Labels=docs +``` + +A dictionary keyed by string works too, contributing its own keys. A type the runtime cannot turn +into text is a build error rather than a `ToString()` sent to your server. + ## Return types | Signature | Behaviour | diff --git a/src/Runax.Http/Requests/QueryFields.cs b/src/Runax.Http/Requests/QueryFields.cs new file mode 100644 index 0000000..32524bc --- /dev/null +++ b/src/Runax.Http/Requests/QueryFields.cs @@ -0,0 +1,45 @@ +namespace Runax.Http.Requests; + +/// +/// Turns a dictionary-shaped [Query] argument into query entries, for generated code. +/// +/// +/// +/// The one query shape whose keys are not known until run time. An object's fields are +/// resolved by the generator and emitted one by one; a dictionary's are whatever the caller put in +/// it, so the pairs have to be produced here and spliced into the request. +/// +/// +/// Still reflection-free: the argument is already a sequence of key/value pairs, so this only +/// re-shapes what it was handed. Values keep their static type as and are +/// formatted later by the same formatter as every other query value. +/// +/// +public static class QueryFields +{ + /// Re-shapes a dictionary-like argument into query entries. + /// The dictionary's value type. + /// The entries, or . + /// + /// One entry per pair, in the order the sequence yields them. Empty for a null argument, which + /// is how a null query object already behaves. + /// + /// + /// A null value is kept rather than dropped here, because dropping it is the query + /// builder's job — it omits a null value for every other shape too, and duplicating that rule + /// in two places is how the two stop agreeing. + /// + public static IEnumerable> From( + IEnumerable>? fields) + { + if (fields is null) + return []; + + var entries = new List>(); + + foreach (var (key, value) in fields) + entries.Add(new KeyValuePair(key, value)); + + return entries; + } +} diff --git a/tests/Runax.Http.AotSmoke/AotClient.cs b/tests/Runax.Http.AotSmoke/AotClient.cs index dc86b02..9ee657d 100644 --- a/tests/Runax.Http.AotSmoke/AotClient.cs +++ b/tests/Runax.Http.AotSmoke/AotClient.cs @@ -40,6 +40,28 @@ internal interface IThingClient [Response(404, typeof(ThingError))] [Response(typeof(Thing))] Task GetQuirkyAsync(int id, CancellationToken ct = default); + + /// + /// A query object, expanded to one key per property by the generator. Here to prove the + /// expansion needs no reflection: a reflective implementation would fail this publish. + /// + [Get("/things")] + Task SearchAsync([Query] ThingFilter filter, CancellationToken ct = default); + + /// A dictionary query argument, whose keys are spread in at run time. + [Get("/things/dynamic")] + Task SearchDynamicAsync( + [Query] global::System.Collections.Generic.Dictionary filters, + CancellationToken ct = default); +} + +/// A flat filter, expanded into the query string at compile time. +internal sealed record ThingFilter +{ + public string? Name { get; init; } + + [Query("per_page")] + public int PerPage { get; init; } } /// An error envelope, for the dispatched path. diff --git a/tests/Runax.Http.AotSmoke/Program.cs b/tests/Runax.Http.AotSmoke/Program.cs index 094ded9..0094ccf 100644 --- a/tests/Runax.Http.AotSmoke/Program.cs +++ b/tests/Runax.Http.AotSmoke/Program.cs @@ -99,6 +99,14 @@ var authenticated = await things.GetAsync(7); Check(authenticated.Id == 7, "a request through the full pipeline still round-trips"); +// Query-object expansion under AOT. The generator resolved these keys at compile time, so this +// line working is the evidence that nothing walks the type's properties at run time. +var searched = await things.SearchAsync(new ThingFilter { Name = "widget", PerPage = 50 }); +Check(searched.Id == 7, "query object expanded"); + +var dynamic = await things.SearchDynamicAsync(new Dictionary { ["state"] = "open" }); +Check(dynamic.Id == 7, "dictionary query spread"); + Console.WriteLine("AOT smoke test passed."); return 0; diff --git a/tests/Runax.Http.SourceGenerator.Tests/DiagnosticTests.cs b/tests/Runax.Http.SourceGenerator.Tests/DiagnosticTests.cs index c8aaeba..0a2aea2 100644 --- a/tests/Runax.Http.SourceGenerator.Tests/DiagnosticTests.cs +++ b/tests/Runax.Http.SourceGenerator.Tests/DiagnosticTests.cs @@ -663,6 +663,145 @@ public sealed record ApiError(string Error); run.Ids.ShouldBeEmpty(); } + [Theory] + [InlineData("string")] + [InlineData("int")] + [InlineData("int?")] + [InlineData("bool")] + [InlineData("global::System.Guid")] + [InlineData("global::System.DateTimeOffset")] + [InlineData("global::System.Uri")] + [InlineData("global::System.Collections.Generic.IEnumerable")] + [InlineData("string[]")] + public void A_sendable_query_type_is_accepted(string type) + { + var run = GeneratorHarness.RunContract($$""" + [BaseAddress("https://api.example.com")] + public interface IGood + { + [Get("/things")] + Task GetAsync([Query] {{type}} value); + } + """); + + run.Ids.ShouldBeEmpty(); + } + + [Fact] + public void A_query_object_is_expanded_rather_than_rejected() + { + var run = GeneratorHarness.RunContract(""" + [BaseAddress("https://api.example.com")] + public interface IGood + { + [Get("/things")] + Task GetAsync([Query] Filter filter); + } + + public sealed record Filter(string State, int Page); + """); + + run.Ids.ShouldBeEmpty(); + + // One key per property, resolved at compile time — which is the whole point: no reflection. + run.AllSource.ShouldContain("\"State\""); + run.AllSource.ShouldContain("\"Page\""); + run.AllSource.ShouldNotContain("\"filter\""); + } + + [Fact] + public void A_dictionary_query_parameter_is_spread_at_run_time() + { + var run = GeneratorHarness.RunContract(""" + [BaseAddress("https://api.example.com")] + public interface IGood + { + [Get("/things")] + Task GetAsync([Query] Dictionary filters); + } + """); + + run.Ids.ShouldBeEmpty(); + run.AllSource.ShouldContain("QueryFields.From"); + } + + [Fact] + public void RXH022_a_query_parameter_that_cannot_be_sent() + { + // The bug this replaced: a type with no usable properties previously fell through to + // ToString() and the server received "Opaque" with no warning anywhere. + var run = GeneratorHarness.RunContract(""" + public interface IBad + { + [Get("/things")] + Task GetAsync([Query] Opaque value); + } + + public sealed class Opaque + { + private int Hidden { get; set; } + } + """); + + run.Ids.ShouldContain("RXH022"); + run.Message("RXH022").ShouldContain("query string"); + } + + [Fact] + public void RXH022_a_collection_of_objects_cannot_be_sent() + { + var run = GeneratorHarness.RunContract(""" + public interface IBad + { + [Get("/things")] + Task GetAsync([Query] IEnumerable filters); + } + + public sealed record Filter(string State); + """); + + run.Ids.ShouldContain("RXH022"); + run.Message("RXH022").ShouldContain("items"); + } + + [Fact] + public void RXH023_a_query_object_with_a_nested_object_property() + { + // Refused rather than flattened: ASP.NET Core spells a nested key "filter.state" and + // OpenAPI's deepObject spells it "filter[state]", so flattening would have to guess. + var run = GeneratorHarness.RunContract(""" + public interface IBad + { + [Get("/things")] + Task GetAsync([Query] Outer filter); + } + + public sealed record Inner(string State); + public sealed record Outer(Inner Nested, int Page); + """); + + run.Ids.ShouldContain("RXH023"); + run.Message("RXH023").ShouldContain("Nested"); + run.Message("RXH023").ShouldContain("nested field"); + } + + [Fact] + public void RXH023_names_the_property_that_cannot_be_sent() + { + var run = GeneratorHarness.RunContract(""" + public interface IBad + { + [Get("/things")] + Task GetAsync([Query] Filter filter); + } + + public sealed record Filter(string State, System.Text.StringBuilder Builder); + """); + + run.Ids.ShouldContain("RXH023"); + run.Message("RXH023").ShouldContain("Builder"); + } + [Fact] public void Every_diagnostic_reports_a_location_in_the_user_code() { diff --git a/tests/Runax.Http.Tests/Contracts/ISearchClient.cs b/tests/Runax.Http.Tests/Contracts/ISearchClient.cs new file mode 100644 index 0000000..f1e895b --- /dev/null +++ b/tests/Runax.Http.Tests/Contracts/ISearchClient.cs @@ -0,0 +1,71 @@ +namespace Runax.Http.Tests.Contracts; + +/// Pagination fields shared by filters, to prove an inherited property is expanded. +internal abstract record Paged +{ + public int Page { get; init; } = 1; + + [Query("per_page")] + public int PerPage { get; init; } = 30; +} + +/// A flat filter object — the shape that motivates expansion. +internal sealed record SearchFilter : Paged +{ + public string? State { get; init; } + + [Query("sort_by")] + public string? SortBy { get; init; } + + public IEnumerable? Labels { get; init; } + + public DateTimeOffset? Since { get; init; } + + public bool? Archived { get; init; } + + public SearchOrder Order { get; init; } +} + +/// Proves an enum property sends its member name, as an enum parameter already does. +internal enum SearchOrder +{ + Ascending, + Descending +} + +/// A struct filter, so the non-nullable value-type access path is exercised. +internal readonly record struct PointFilter(int X, int Y); + +/// Query objects, dictionaries, and the scalars that already worked. +[BaseAddress("https://api.example.com")] +internal interface ISearchClient +{ + [Get("/search")] + Task SearchAsync([Query] SearchFilter filter, CancellationToken ct = default); + + [Get("/search/nullable")] + Task SearchNullableAsync([Query] SearchFilter? filter, CancellationToken ct = default); + + [Get("/search/point")] + Task SearchPointAsync([Query] PointFilter point, CancellationToken ct = default); + + [Get("/search/struct-nullable")] + Task SearchNullablePointAsync([Query] PointFilter? point, CancellationToken ct = default); + + [Get("/search/dynamic")] + Task SearchDynamicAsync( + [Query] Dictionary filters, + CancellationToken ct = default); + + [Get("/search/dynamic-typed")] + Task SearchDynamicTypedAsync( + [Query] IEnumerable> filters, + CancellationToken ct = default); + + /// An object alongside a plain scalar, which must keep working as before. + [Get("/search/mixed")] + Task SearchMixedAsync( + [Query] string q, + [Query] SearchFilter filter, + CancellationToken ct = default); +} diff --git a/tests/Runax.Http.Tests/QueryObjectTests.cs b/tests/Runax.Http.Tests/QueryObjectTests.cs new file mode 100644 index 0000000..09da781 --- /dev/null +++ b/tests/Runax.Http.Tests/QueryObjectTests.cs @@ -0,0 +1,245 @@ +using Microsoft.Extensions.DependencyInjection; +using Runax.Http.DependencyInjection; +using Runax.Http.Tests.Contracts; +using Runax.Http.Tests.Fakes; +using Runax.Http.Transports; + +namespace Runax.Http.Tests; + +/// +/// A whole object as a [Query] parameter, expanded to one key per property at compile time. +/// +/// +/// Before this, `[Query] SearchFilter filter` compiled clean and sent +/// ?filter=SearchFilter%20%7B%20State%20%3D%20open... — ToString() on the record, +/// with no diagnostic. These tests pin the shape of the query string, because that is the part a +/// server sees and the part that was silently wrong. +/// +public class QueryObjectTests +{ + private static (ISearchClient Client, RecordingTransport Transport) Build() + { + var transport = new RecordingTransport( + respond: _ => Responses.Json("""{"id":1,"name":"ok"}""")); + + var services = new ServiceCollection(); + + var shared = services.AddRunaxHttp(); + shared.UseSystemTextJson(); + shared.AddTransport(transport); + shared.UseDefaultTransport(transport.Name); + + services.AddRunaxClient(); + + return (services.BuildServiceProvider().GetRequiredService(), transport); + } + + private static string Query(RecordingTransport transport) => transport.Last.RequestUri.Query; + + [Fact] + public async Task An_object_expands_to_one_key_per_property() + { + var (client, transport) = Build(); + + await client.SearchAsync(new SearchFilter { State = "open", Page = 2, PerPage = 50 }); + + // Not "?filter=SearchFilter { ... }". + Query(transport).ShouldNotContain("filter="); + Query(transport).ShouldContain("State=open"); + Query(transport).ShouldContain("Page=2"); + } + + [Fact] + public async Task A_property_name_is_sent_exactly_as_written() + { + // Nothing in this library transforms a name: a [Query] parameter sends its parameter name + // verbatim, so a property has to behave the same way or the two rules disagree. + var (client, transport) = Build(); + + await client.SearchAsync(new SearchFilter { State = "open" }); + + Query(transport).ShouldContain("State=open"); + + // Case-sensitively: Shouldly compares case-insensitively by default, which would let a + // lowercased key pass the very assertion meant to catch it. + Query(transport).ShouldNotContain("state=open", Case.Sensitive); + } + + [Fact] + public async Task A_property_can_be_renamed_with_Query() + { + // Which is how a C# PerPage reaches an API that spells it per_page. + var (client, transport) = Build(); + + await client.SearchAsync(new SearchFilter { PerPage = 50, SortBy = "created" }); + + Query(transport).ShouldContain("per_page=50"); + Query(transport).ShouldContain("sort_by=created"); + Query(transport).ShouldNotContain("PerPage"); + Query(transport).ShouldNotContain("SortBy"); + } + + [Fact] + public async Task An_inherited_property_is_expanded_too() + { + // A filter type with a shared pagination base is the obvious shape; ignoring the base would + // drop fields silently, which is the failure this whole change exists to stop. + var (client, transport) = Build(); + + await client.SearchAsync(new SearchFilter { Page = 7 }); + + Query(transport).ShouldContain("Page=7"); + } + + [Fact] + public async Task A_null_property_is_omitted_rather_than_sent_empty() + { + // Consistent with a null [Query] parameter, which is already omitted. + var (client, transport) = Build(); + + await client.SearchAsync(new SearchFilter { State = null, Page = 1, PerPage = 30 }); + + Query(transport).ShouldNotContain("State"); + Query(transport).ShouldNotContain("Since"); + Query(transport).ShouldContain("Page=1"); + } + + [Fact] + public async Task A_collection_property_becomes_repeated_keys() + { + var (client, transport) = Build(); + + await client.SearchAsync(new SearchFilter { Labels = ["bug", "needs triage"] }); + + Query(transport).ShouldContain("Labels=bug"); + Query(transport).ShouldContain("Labels=needs%20triage"); + } + + [Fact] + public async Task Property_values_are_formatted_like_every_other_query_value() + { + // The formatter is shared, so a DateTimeOffset property has to spell the same thing a + // DateTimeOffset parameter does — invariant, round-trippable, lowercase bool. + var (client, transport) = Build(); + + await client.SearchAsync(new SearchFilter + { + Since = new DateTimeOffset(2026, 11, 7, 9, 30, 0, TimeSpan.Zero), + Archived = false, + Order = SearchOrder.Descending + }); + + Query(transport).ShouldContain("Since=2026-11-07T09%3A30%3A00.0000000%2B00%3A00"); + Query(transport).ShouldContain("Archived=false"); + Query(transport).ShouldContain("Order=Descending"); + } + + [Fact] + public async Task A_null_object_contributes_nothing() + { + var (client, transport) = Build(); + + await client.SearchNullableAsync(null); + + Query(transport).ShouldBeEmpty(); + } + + [Fact] + public async Task A_struct_object_expands_without_null_checks() + { + // The non-nullable value-type access path: `point.X` rather than `point?.X`, which would + // not compile. + var (client, transport) = Build(); + + await client.SearchPointAsync(new PointFilter(3, 4)); + + Query(transport).ShouldBe("?X=3&Y=4"); + } + + [Fact] + public async Task A_nullable_struct_object_expands_when_present() + { + var (client, transport) = Build(); + + await client.SearchNullablePointAsync(new PointFilter(1, 2)); + + Query(transport).ShouldBe("?X=1&Y=2"); + } + + [Fact] + public async Task A_nullable_struct_object_contributes_nothing_when_absent() + { + var (client, transport) = Build(); + + await client.SearchNullablePointAsync(null); + + Query(transport).ShouldBeEmpty(); + } + + [Fact] + public async Task A_dictionary_contributes_its_own_keys() + { + // The one shape whose keys are not known until run time, which is why it is spread in + // rather than emitted field by field. + var (client, transport) = Build(); + + await client.SearchDynamicAsync(new Dictionary + { + ["state"] = "open", + ["author"] = "someone" + }); + + Query(transport).ShouldContain("state=open"); + Query(transport).ShouldContain("author=someone"); + Query(transport).ShouldNotContain("filters="); + } + + [Fact] + public async Task A_dictionary_value_is_formatted_and_a_null_one_omitted() + { + var (client, transport) = Build(); + + await client.SearchDynamicTypedAsync(new Dictionary + { + ["page"] = 3, + ["archived"] = true, + ["missing"] = null + }); + + Query(transport).ShouldContain("page=3"); + Query(transport).ShouldContain("archived=true"); + Query(transport).ShouldNotContain("missing"); + } + + [Fact] + public async Task An_empty_dictionary_contributes_nothing() + { + var (client, transport) = Build(); + + await client.SearchDynamicAsync([]); + + Query(transport).ShouldBeEmpty(); + } + + [Fact] + public async Task A_scalar_alongside_an_object_keeps_working() + { + var (client, transport) = Build(); + + await client.SearchMixedAsync("runax", new SearchFilter { State = "open" }); + + // Order follows the parameter list, so the scalar comes first. + Query(transport).ShouldStartWith("?q=runax"); + Query(transport).ShouldContain("State=open"); + } + + [Fact] + public async Task Keys_and_values_are_still_escaped() + { + var (client, transport) = Build(); + + await client.SearchAsync(new SearchFilter { State = "a b&c=d" }); + + Query(transport).ShouldContain("State=a%20b%26c%3Dd"); + } +}