fix(Table): support QueryPageOptions serialization #7310
Merged
+63 −109
Conversation
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
| Thanks for your PR, @kimdiego2098. Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
Contributor
Reviewer's GuideRefactors QueryPageOptions JSON (de)serialization to use a single FilterKeyValueAction representation and a polymorphic object converter, ensuring filters and search models serialize/deserialize correctly and adds a small sample test usage. Sequence diagram for QueryPageOptions round-trip JSON serialization with FilterKeyValueActionsequenceDiagram actor SampleComponent participant JsonSerializer as SystemTextJson_JsonSerializer participant Converter as JsonQueryPageOptionsConverter participant Options as QueryPageOptions participant Ext as QueryPageOptionsExtensions participant Fkva as FilterKeyValueAction SampleComponent->>Options: Configure filters and searches SampleComponent->>JsonSerializer: Serialize(Options) JsonSerializer->>Converter: Write(writer, Options, options) Converter->>Options: Check Searches, CustomerSearches, AdvanceSearches, Filters alt Any_filter_list_non_empty Converter->>Ext: ToFilter(Options) alt Existing_FilterKeyValueAction Ext-->>Converter: return Options.FilterKeyValueAction else Build_new_FilterKeyValueAction Ext->>Fkva: new FilterKeyValueAction() Ext->>Fkva: Populate from Searches and Filters Ext-->>Converter: return Fkva end Converter->>JsonSerializer: Write filterKeyValueAction property end Converter-->>JsonSerializer: Finish writing QueryPageOptions JSON JsonSerializer-->>SampleComponent: JSON string SampleComponent->>JsonSerializer: Deserialize(json, QueryPageOptions) JsonSerializer->>Converter: Read(ref reader, typeToConvert, options) Converter->>Options: Create new QueryPageOptions Converter->>Options: Set IsVirtualScroll, IsFirstQuery, etc. Converter->>Options: Read filterKeyValueAction and assign FilterKeyValueAction Converter-->>JsonSerializer: return Options JsonSerializer-->>SampleComponent: QueryPageOptions instance with FilterKeyValueAction SampleComponent->>Ext: ToFilter(Options) Ext-->>SampleComponent: returns existing FilterKeyValueAction from Options.FilterKeyValueAction Class diagram for updated QueryPageOptions JSON serialization modelclassDiagram class QueryPageOptions { object SearchModel List~IFilterAction~ Searches List~IFilterAction~ CustomerSearches List~IFilterAction~ AdvanceSearches List~IFilterAction~ Filters internal FilterKeyValueAction FilterKeyValueAction bool IsVirtualScroll bool IsFirstQuery bool IsAdvanceSearch bool IsSimpleSearch string? SearchText int PageIndex int PageItems } class FilterKeyValueAction { string? FieldName object? FieldValue string? Operator bool IsAnd bool IsOr } class ObjectWithTypeConverter { +object Read(Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) +void Write(Utf8JsonWriter writer, object value, JsonSerializerOptions options) } class JsonQueryPageOptionsConverter { +QueryPageOptions Read(Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) +void Write(Utf8JsonWriter writer, QueryPageOptions value, JsonSerializerOptions options) } class QueryPageOptionsExtensions { +FilterKeyValueAction ToFilter(QueryPageOptions option) } interface IFilterAction QueryPageOptions --> "*" IFilterAction : uses QueryPageOptions --> FilterKeyValueAction : aggregates QueryPageOptionsExtensions ..> QueryPageOptions : extension QueryPageOptionsExtensions ..> FilterKeyValueAction : creates JsonQueryPageOptionsConverter ..> QueryPageOptions : converts JsonQueryPageOptionsConverter ..> FilterKeyValueAction : serializes ObjectWithTypeConverter <.. QueryPageOptions : applied_to_SearchModel ObjectWithTypeConverter <.. FilterKeyValueAction : applied_to_FieldValue File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
- The new ObjectWithTypeConverter deserializes arbitrary types via Type.GetType and assembly-qualified names, which is risky and brittle; consider restricting allowed types/namespaces and handling Type.GetType returning null or invalid payloads more defensively.
- When no $type is present, ObjectWithTypeConverter returns a JsonElement clone, which changes the runtime type compared to previously using a concrete model; ensure this is compatible with existing usages of SearchModel/FieldValue or consider a fallback strategy that preserves expected types.
- The JSON round-trip in TablesFilter.OnQueryAsync appears to be test/debug-only and adds overhead on each query; consider removing it or wrapping it in a DEBUG conditional.
Prompt for AI Agents
Please address the comments from this code review: ## Overall Comments - The new ObjectWithTypeConverter deserializes arbitrary types via Type.GetType and assembly-qualified names, which is risky and brittle; consider restricting allowed types/namespaces and handling Type.GetType returning null or invalid payloads more defensively. - When no $type is present, ObjectWithTypeConverter returns a JsonElement clone, which changes the runtime type compared to previously using a concrete model; ensure this is compatible with existing usages of SearchModel/FieldValue or consider a fallback strategy that preserves expected types. - The JSON round-trip in TablesFilter.OnQueryAsync appears to be test/debug-only and adds overhead on each query; consider removing it or wrapping it in a DEBUG conditional. ## Individual Comments ### Comment 1 <location> `src/BootstrapBlazor/Converter/JsonQueryPageOptionConverter.cs:190` </location> <code_context> - } - if (value.Filters.Count != 0) + + if (value.Searches.Count != 0||value.CustomerSearches.Count != 0||value.AdvanceSearches.Count != 0|| value.Filters.Count != 0) { - writer.WriteStartArray("filters"); </code_context> <issue_to_address> **issue (bug_risk):** Guard against the case where `FilterKeyValueAction` is already set but filter lists are empty. Because deserialization sets `FilterKeyValueAction` directly and leaves `Searches` / `CustomerSearches` / `AdvanceSearches` / `Filters` empty, this `if` will skip writing filters when all lists are empty even if `FilterKeyValueAction` is populated (e.g., after a round-trip). To avoid losing existing filter state on re‑serialization, also include `value.FilterKeyValueAction != null` in this condition. </issue_to_address> ### Comment 2 <location> `src/BootstrapBlazor/Components/Filters/FilterKeyValueAction.cs:23` </location> <code_context> /// <summary> /// 获得/设置 Filter 项字段值 /// </summary> + [JsonConverter(typeof(ObjectWithTypeConverter))] public object? FieldValue { get; set; } </code_context> <issue_to_address> **🚨 issue (security):** Using type information from JSON for polymorphic deserialization can be a security risk. Annotating `FieldValue` with `ObjectWithTypeConverter` allows clients to control the `$type` used during deserialization. Because `Type.GetType` with assembly-qualified names can resolve arbitrary types, this is unsafe for untrusted input. Use a restricted type resolution strategy (e.g., a whitelist or custom resolver) instead of calling `Type.GetType` directly on user data. </issue_to_address> ### Comment 3 <location> `src/BootstrapBlazor/Converter/ObjectWithTypeConverter.cs:20-23` </location> <code_context> + if (!doc.RootElement.TryGetProperty("$type", out var typeProp)) + return doc.RootElement.Clone(); // 无类型信息 + + var type = Type.GetType(typeProp.GetString()!)!; + + var valueElement = doc.RootElement.GetProperty("value"); + return JsonSerializer.Deserialize(valueElement.GetRawText(), type, options); + } + </code_context> <issue_to_address> **issue:** Handle cases where the specified type cannot be resolved or the `value` property is missing. `Type.GetType` can return null (e.g., type moved, different assembly, trimming), and the null-forgiving operator will hide this until a later NRE. Also, if `$type` is present but `value` is missing, `GetProperty("value")` will throw. Please handle these cases explicitly (e.g., detect and throw a `JsonException`, fall back to `JsonElement`, or apply a migration strategy) instead of relying on implicit null/KeyNotFound exceptions. </issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } | ||
| if (value.Filters.Count != 0) | ||
| | ||
| if (value.Searches.Count != 0||value.CustomerSearches.Count != 0||value.AdvanceSearches.Count != 0|| value.Filters.Count != 0) |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Guard against the case where FilterKeyValueAction is already set but filter lists are empty.
Because deserialization sets FilterKeyValueAction directly and leaves Searches / CustomerSearches / AdvanceSearches / Filters empty, this if will skip writing filters when all lists are empty even if FilterKeyValueAction is populated (e.g., after a round-trip). To avoid losing existing filter state on re‑serialization, also include value.FilterKeyValueAction != null in this condition.
| /// <summary> | ||
| /// 获得/设置 Filter 项字段值 | ||
| /// </summary> | ||
| [JsonConverter(typeof(ObjectWithTypeConverter))] |
Contributor
There was a problem hiding this comment.
🚨 issue (security): Using type information from JSON for polymorphic deserialization can be a security risk.
Annotating FieldValue with ObjectWithTypeConverter allows clients to control the $type used during deserialization. Because Type.GetType with assembly-qualified names can resolve arbitrary types, this is unsafe for untrusted input. Use a restricted type resolution strategy (e.g., a whitelist or custom resolver) instead of calling Type.GetType directly on user data.
Comment on lines +20 to +23
| var type = Type.GetType(typeProp.GetString()!)!; | ||
| | ||
| var valueElement = doc.RootElement.GetProperty("value"); | ||
| return JsonSerializer.Deserialize(valueElement.GetRawText(), type, options); |
Contributor
There was a problem hiding this comment.
issue: Handle cases where the specified type cannot be resolved or the value property is missing.
Type.GetType can return null (e.g., type moved, different assembly, trimming), and the null-forgiving operator will hide this until a later NRE. Also, if $type is present but value is missing, GetProperty("value") will throw. Please handle these cases explicitly (e.g., detect and throw a JsonException, fall back to JsonElement, or apply a migration strategy) instead of relying on implicit null/KeyNotFound exceptions.
ArgoZhang previously approved these changes Dec 12, 2025
ArgoZhang changed the title 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.
Summary by Sourcery
Fix JSON serialization and deserialization for QueryPageOptions filter data by introducing a reusable typed-object converter and consolidating filter-related fields into FilterKeyValueAction.
Bug Fixes:
Enhancements: