Skip to content

JsonConverterAttribute for Query Params #1275

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
boginw opened this issue May 22, 2023 · 10 comments
Closed

JsonConverterAttribute for Query Params #1275

boginw opened this issue May 22, 2023 · 10 comments

Comments

@boginw
Copy link
Contributor

boginw commented May 22, 2023

Is your feature request related to a problem? Please describe.
Currently, in the serialization and deserialization of request bodies, the JsonConverterAttribute is considered, allowing for custom conversion logic to be applied. However, when it comes to filters, this attribute is ignored. While this behavior is expected since query parameters are not in JSON format, there are cases where it would be useful to apply certain conversions.

Consider the following scenario where a custom-type Wrapper and its corresponding WrapperJsonConverter are used:

using System;
using System.Text.Json;
using System.Text.Json.Serialization;

[JsonConverter(typeof(WrapperJsonConverter))]
class Wrapper
{
    public string Part1 { get; }
    public string Part2 { get; }

    public Wrapper(string parts)
    {
        string[] split = parts.Split(",");
        Part1 = split[0];
        Part2 = split[1];
    }

    public Wrapper(string part1, string part2)
    {
        Part1 = part1;
        Part2 = part2;
    }
    
    public override string ToString() => Part1 + "," + Part2;
}

public class WrapperJsonConverter : JsonConverter<Wrapper>
{
    public override Wrapper Read(
        ref Utf8JsonReader reader,
        Type typeToConvert,
        JsonSerializerOptions options
    )
    {
        return new Wrapper(reader.GetString());
    }

    public override void Write(
        Utf8JsonWriter writer,
        Wrapper value,
        JsonSerializerOptions options
    )
    {
        writer.WriteStringValue(value.ToString());
    }
}

Currently, when new Wrapper("part1", "part2") is serialized and included in a request body, it is correctly converted to "part1,part2". However, if the client attempts to use a query parameter like equals(wrapper, 'part1,part2'), it will encounter a QueryParseException with the message "Failed to convert 'part1,part2' of type 'String' to type 'Wrapper'." This can lead to confusion and frustration, as the only way forward, is the take the fields from Wrapper and adding them wherever needed.

Describe the solution you'd like
To address this limitation, I propose enhancing RuntimeTypeConverter to support custom type conversions for query parameters as well. This feature would enable developers to define their own conversion logic for custom types when processing query parameters, similar to how it's done for request bodies.

Describe alternatives you've considered

An alternative approach to address the serialization and deserialization of custom types in query parameters could involve utilizing the JsonApiResourceDefinition. In this scenario, the Wrapper class could be treated as a resource, and a corresponding JsonApiResourceDefinition could be created. However, since Wrapper isn't a resource, this would not be advisable.

@bkoelman
Copy link
Member

Hi @boginw, thanks for your proposal. I tried a few things, but am unable to reproduce what you're describing.

Can you please provide a complete example demonstrating your use of Wrapper in a JSON:API resource class? Is it mapped to a single database column or multiple? Which database provider are you using? Do you have any EF Core mapping logic in place?

And what does your HTTP request(s) look like? Do you want to filter on the individual parts? And what would the JSON response body look like?

@boginw
Copy link
Contributor Author

boginw commented May 23, 2023

Hi @bkoelman

Of course, I've set up a test repository showcasing the issue: https://github.com/boginw/json-api-dot-net-query-params-feature
This project does not use any database at all, as this is not necessary to showcase the issue.

The project contains a single resource weather-forecast. The resource has a property Wrapper which is of the expected type. I've added a controller WeatherForecastController : BaseJsonApiController<WeatherForecast, int?> with the ability to handle GET and POST requests.

For convenience, you can fetch WeatherForecasts by:

curl --location 'http://localhost:5000/weather-forecast'

That should return 5 randomly generated WeatherForecasts.

We can also send a WeatherForecast which will just return it back:

curl --location 'http://localhost:5000/weather-forecast' \
--header 'Content-Type: application/vnd.api+json' \
--data '{
    "links": {
        "self": "http://localhost:5000/weather-forecast",
        "first": "http://localhost:5000/weather-forecast"
    },
    "data": {
        "type": "weatherForecasts",
        "attributes": {
            "date": "2023-05-24",
            "temperatureC": 48,
            "summary": "Sweltering",
            "wrapper": "128540050,243020457"
        },
        "links": {
            "self": "http://localhost:5000/weather-forecast"
        }
    }
}'

But as soon as we try to filter by wrapper:

curl --location "http://localhost:5000/weather-forecast?filter=equals(wrapper,'123,321')"

It fails with:

JsonApiDotNetCore.Queries.Internal.Parsing.QueryParseException: ↵
    Failed to convert '123,321' of type 'String' to type 'Wrapper'.

@bkoelman
Copy link
Member

Thanks for the steps, makes sense now.

This project does not use any database at all, as this is not necessary to showcase the issue.

Actually, it matters a lot how data is stored and queried. I was unable to use Wrapper in our GettingStarted sample because EF Core initialization fails at startup, as it is unable to choose a constructor. It needs to be able to create resource instances during materialization of a SQL result set into objects. Even if EF Core could pick a constructor, JsonApiDotNetCore cannot do so. This is observable when using sparse fieldsets against a database. Therefore the only way to make that work is to remove all constructors from Wrapper and make its properties read/write.

To make sort/filter work in the database (and be efficient on large tables), it needs to be translated to SQL. For that to happen, EF Core must be able to "read through" your wrapper. There are various ways to do that, such as:

  • using an owned entity (stored as individual columns, or as a JSON blob)
  • possibly registering a type conversion in the model
  • publicly exposing the data as a JSON string, backed by a calculated property that reads from database-mapped columns
  • turning Wrapper into a full resource, effectively using JSON:API relationship semantics
  • ...

Therefore it matters greatly how you expect API clients to interact with the data inside the wrapper, and how it is stored internally. There's also ASP.NET ModelState validation to consider, which uses reflection to descend into objects.

I do not see usage of JsonConverterAttribute in the repro repository. I don't think it actually works, because resource properties are copied into a dictionary before serialization starts. Therefore such attributes are lost. But it is possible to use the custom converter by adding it to JsonApiOptions.SerializerOptions.Converters at startup.

Opening up the static RuntimeTypeConverter for extension may seem like the right approach, based on the sample you shared, and I now understand why you suggested that. The thing is, for something like this to make it into the project, we need to rationalize how it works in a realistic API project, and what the experience will be in combination with existing features.

I suspect there's an easier/better way to achieve what you need, using existing features. And I'm willing to guide you in the right direction, but I need to understand the big picture, to be of help. So please explain your use case by providing a more realistic sample that you'd potentially put into production, including a database, etc. Could you try adding your customizations to the GettingStarted sample and see if you can get that running?

@boginw
Copy link
Contributor Author

boginw commented May 24, 2023

Thank you for your detailed response and suggestions. I've made some updates to the test repository and included the GettingStarted sample with the necessary modifications to demonstrate the issue.

In the modified GettingStarted sample, I've added the Wrapper class as a field on the Book class. To handle the serialization and deserialization of the Wrapper type, I registered a ValueConverter<Wrapper, string> in the ConfigureConventions method of the SampleDbContext class. The Wrapper class still has the JsonConverter annotation, which allows the Wrapper type to be correctly serialized and deserialized when performing GET and POST requests.

However, the issue still persists when trying to filter on the Wrapper property using query parameters. When sending a GET request with a filter like equals(wrapper, '123,321'), the same QueryParseException occurs: "Failed to convert '123,321' of type 'String' to type 'Wrapper'."

I do not see usage of JsonConverterAttribute in the repro repository...

In the original and the GettingStarted projects, the Wrapper class is annotated with the JsonConverter-attribute. See this for reference. In both projects JsonApiOptions.SerializerOptions.Converters is not used. Perhaps I'm missing something, but the results clearly show it working, both with serialization and deserialization.

The name Wrapper is perhaps a bit too generic. For context, we intend to use a type that can identify a specific variant of a product. It would include a product ID and a product variant code. Let's say it is shirts, then it could be like "1234-XL". The intention is not to use it as a full resource with relationship semantics, but rather as a custom type that can be serialized and deserialized correctly in both request bodies and query parameters.

Based on your experience with JsonApiDotNetCore, are there any existing features or approaches that you would recommend for handling custom-type conversions in query parameters? I would greatly appreciate any guidance you can provide in this regard.

Thanks :)

@bkoelman
Copy link
Member

Well, it works out a lot better than I expected! That's mainly because you're acting at the type level.

I expected the JsonConverter<> to be used on the resource property. That's why I failed to see its usage. It works here because the converter is used on the Wrapper type instead. Here's why:

  1. JADNC copies resource property values into a string-to-object dictionary (so attributes on the declaring property are lost)
  2. The JSON serializer inspects the runtime type of the dictionary value, sees Wrapper, then reads the attributes on that type (and any nested properties).

Likewise, when sparse fieldsets are used, JADNC generates a LINQ query such as:

books.Select(book => new Book
{
  Id = book.Id,
  Wrapper = book.Wrapper
});

So it just copies existing Wrapper instances instead of instantiating them. That's handled by the EF Core ValueConverter. Again, this converter is applied to the Wapper type, instead of to the Book.Wrapper property.

I discovered that ModelState validation does not run for Wrapper properties that are annotated with [Required], [MinLength] etc. That's because JADNC injects a custom handler to make partial updates work. That handler assumes that any non-top-level properties must be JSON:API relationships. This may not be of concern to you, just something I hadn't realized before.

I've been playing with opening up the filter parser and found the issue you reported affects the following filter functions (no other query string parameters):

  • comparisons: equals, lessThan, lessOrEqual, greaterThan, greaterOrEqual
  • text matching: contains, startsWith, endsWith
  • set matching: any

Passing a custom type converter works for all of them, except any. That's because it requires the argument to be of type string, while the custom converter maps from a string to a Wrapper instance. I'm not sure it's safe to assume that ToString() is always the right way to obtain a string representation (thinking of arrays, dictionaries, generic types, etc).

Back to your scenario: wouldn't it be easier to just expose the product ID and variant as separate JSON:API fields and store them in separate database columns? That would enable more powerful filters, such as:
?filter=and(equals(id,'123'),any(variant,'L','XL','XXL'))

Options, with some pointers:

  1. one attribute, one column: no action needed
  2. one attribute, two columns: exposed calculated property + non-exposed mapped column; handle post/patch in ResourceDefinition
  3. two attributes, one column: if filters are needed, implement QueryRewriter (see example for composite keys)
  4. two attributes, two columns: no action needed

I'm gonna be out in the upcoming days and will get back to this next week.

@bkoelman
Copy link
Member

Passing a custom type converter works for all of them, except any.

Correction: It works for any, but not the substring match functions (contains, startsWith, and endsWith).

I'm trying to implement custom query string value converters, but I've found no way to make text-matching work, even with custom converters in place.

source
  .Where(item => item.Product.ToString().EndsWith("-XL))
  .OrderBy(item => item.Id)
  .Take(10)

fails with:

The LINQ expression (...) could not be translated. Additional information: Translation of method 'JsonApiDotNetCoreTests.IntegrationTests.FilterValueConversions.CompositeValue.Product.ToString' failed.

This makes me wonder if going this route makes sense at all.

  • The resource declares a single attribute (composed of two fields). The inner fields are inaccessible for sorting, paging, sparse fieldset selection, and filtering.
  • The value is stored in a single database column.
  • The JSON in request/response bodies consists of two elements.

So why not just store it in separate database columns, which gives you all the JSON:API capabilities out of the box?

@boginw
Copy link
Contributor Author

boginw commented Jun 5, 2023

Thank you for your detailed analysis and suggestions, and sorry for this late response.

I understand your points about the limitations of using a custom type converter for substring match functions and the benefits of storing the data in separate database columns. However, in our specific use case, we have a requirement to keep the product ID and variant together as a single entity. This is due to some business logic that treats them as a single unit. Splitting them into separate fields would complicate this logic and potentially introduce errors.

I understand that this approach has its limitations with respect to JSON:API capabilities. However, we are willing to work within these limitations given our specific requirements.

I'm interested in exploring other potential solutions that could work within our constraints. For example, could we possibly extend the filter parser to handle our custom type? Any further guidance you could provide would be greatly appreciated.

@bkoelman
Copy link
Member

bkoelman commented Jun 8, 2023

So if I understand correctly, you'd like to:

  • Store the product ID with variant as a single free-format string column in the database
  • Expose the product ID with variant as a single free-format string attribute through JSON:API
  • Apply POST/PATCH and query string filters solely on the composed string value

This makes me wonder why the Wrapper type exists at all. It treats any input as valid, so apparently its purpose is not input validation. Then the only reason I can think of why it exists is to provide typed access internally within the API project. In that case, having an internally calculated property would be the easiest solution (see boginw/json-api-dot-net-query-params-feature#1).

If input validation is desired, this is best solved using a resource definition that validates the incoming attribute value (see https://www.jsonapi.net/usage/common-pitfalls.html#jsonapi-resources-are-not-ddd-domain-entities).

I'm not saying no to extending filter parsing; it's just something unavailable today. I'd like to address that, but it's related to #1277 and I'm not sure yet how to best solve that.

@bkoelman
Copy link
Member

@boginw can you take a look at #1284 and give it a try within the next few days? And could you let me know if this addresses your needs? I'm open to alternatives; any feedback you'd be able to give me is welcome.

@bkoelman
Copy link
Member

bkoelman commented Jul 5, 2023

Closing due to inactivity. Please comment if you need this to be reopened.

@bkoelman bkoelman closed this as completed Jul 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

2 participants