Escaped Commas cannot be used in a filter string

[mary-gao]

Description

It's not possible right now to search for items using a comma in the filter string. So, when I GET

/entities?filter[name]=like:%2C

I don't get the entries that I expect (all the entities with a name that contains a comma).

It looks like this is because we split the filter string on the comma after it has already been decoded, so we do not distinguish between "?filter[name]=a%2Cc" and "?filter[name]=a,c".

Environment

• JsonApiDotNetCore Version: 2.4.0

[jaredcnance]

🤔 I'm not sure that we should distinguish between encoded and non encoded commas (or even if we can—if i remember correctly, it's already decoded when we get it). This behavior comes from the JSON:API recommendations on filtering: https://jsonapi.org/recommendations/#filtering/

But, maybe we should add a flag to opt out of comma separated value parsing. Would that work for your needs? And if so, would you use it app-wide or on a per resource basis?

[mary-gao]

I was hoping to use comma separated value parsing as well, actually. I noticed that the url was decoded already by the time the query parsing happened, but I don't know why that is. Couldn't we get the unescaped string directly from the request in the controller, and then split the query parameters, and then decode the query parameters?

[ScottEDubya]

I don't think the jsonapi spec specifically says anything about what you should/shouldn't be able to filter on. if i wanted to filter on multiple things with commas in their names, my url should look like the following:
http://domain.com/entities?filter[name]=foo%2C%20bar,boo%2C%20far
so we could search for two names "foo, bar" and "boo, far". there's a distinguishable difference in the literal URL between the commas for value separation and the commas which we're looking for in our filter.

i think this is related/under the hood nearly the same issue as the one i opened a bit ago: #408
the way the library is handling the url (or possibly allowing .net core to do the url parsing in its default way maybe?) is limiting our ability to effectively filter our data sets

[jaredcnance]

I think that's an accurate assessment. What we'll need to figure out is how to get the raw url encoded values since it looks like AspNetCore already decodes them when deserializing to string. Right now we're just passing in the HttpContext.Request.Query. We can probably get them directly out of the request but may need to do our own parsing. Or we may be able to inject our own formatter, will have to do some research.

[ScottEDubya]

Yeah, @mary-gao seems to have found that the controllers do have access to the literal query string, but as far as it being the right solution to write jsonapi's own parser, i'm not sure about that one. feels like something someone in the past would have run into and there might already be an elegant solution out there!

[jaredcnance]

In order to implement this, we will need to use HttpContext.Request.QueryString instead of HttpContext.Request.Query. The difference here is that QueryString contains the raw string value prior to parsing. But, that means we'll need to split the query string on & and = ourselves. This isn't great because that means AspNetCore will be doing unnecessary work and allocations when parsing and constructing the QueryCollection. So, then for every request the path is stored in at least 3 different locations in memory (but probably 4 since it's probably accessible through the Location header as well).

It would be a lot better if we could hijack the parsing work so it only happens once and leaves the values percent encoded. I did some quick looking but haven't found where the QueryCollection actually gets built in the AspNetCore pipeline. I looked at implementing a custom IValueProviderFactory, but by the time it is called, it looks like the QueryCollection has already been created.

[jaredcnance]

Digging a little further this is where it happens:

• The Query object is stored in the QueryFeature
  https://github.com/aspnet/HttpAbstractions/blob/1d3521113ad69a9903d1590c77d67da04c5f5dfd/src/Microsoft.AspNetCore.Http/Internal/DefaultHttpRequest.cs#L116

• Which is loaded from a cache
  https://github.com/aspnet/HttpAbstractions/blob/1d3521113ad69a9903d1590c77d67da04c5f5dfd/src/Microsoft.AspNetCore.Http/Internal/DefaultHttpRequest.cs#L48-L49

• or a new one is created
  https://github.com/aspnet/HttpAbstractions/blob/1d3521113ad69a9903d1590c77d67da04c5f5dfd/src/Microsoft.AspNetCore.Http/Internal/DefaultHttpRequest.cs#L18

• when the Query is requested it lazily parses the values by calling into QueryHelpers.ParseNullableQuery
  https://github.com/aspnet/HttpAbstractions/blob/1d3521113ad69a9903d1590c77d67da04c5f5dfd/src/Microsoft.AspNetCore.Http/Features/QueryFeature.cs#L61

• which decodes the characters
  https://github.com/aspnet/HttpAbstractions/blob/1d3521113ad69a9903d1590c77d67da04c5f5dfd/src/Microsoft.AspNetCore.WebUtilities/QueryHelpers.cs#L166

So, based on that we'd need a way to provide a custom HttpRequest implementation that inherits from DefaultHttpRequest and just changes the query parsing behavior. Since the DefaultHttpRequest is instantiated directly by the DefaultHttpContext rather than using an injected factory (https://github.com/aspnet/HttpAbstractions/blob/1d3521113ad69a9903d1590c77d67da04c5f5dfd/src/Microsoft.AspNetCore.Http/DefaultHttpContext.cs#L196), it seems like we'd need to create our own implementation of HttpRequest, HttpContext and IHttpContextAccessor. So, I'm leaning towards parsing the QueryString ourselves.

As a side note this would be a breaking change since I suspect some clients are already encoding , as %2C and using it as an &key=value2 replacement.

[roblankey]

Alternatively, what about just using a CSV-type parser and surrounding filter values containing commas with quotes. CsvHelper and TinyCsvParser are the two that jumped out pretty quickly when searching.

[jaredcnance]

I don't think we need to bring a dependency in for this. Parsing is not difficult and I don't think it should be specific to %2C. I just didn't want to do extra work on each request if we didn't need to. I think to avoid a breaking change and unexpected behavior for existing users we can:

• Introduce an IQueryParamParser:

public interface IQueryParamParser {
  QueryCollection GetParsedQueryParams();
}

• Add a default implementation that forwards the already parsed/cached values

public class DefaultQueryParamParser : IQueryParamParser {
  private readonly HttpContext _httpContext
  public DefaultQueryParamParser(IHttpContextAccessor httpContextAccessor) {
     _httpContext = httpContextAccessor.HttpContext;
  }
  public QueryCollection GetParsedQueryParams() => _httpContext.Request.Query;
}

• Add optional implementation that doesn't decode values

// better name desired...
public class RawValueQueryParamParser : IQueryParamParser { /* TBD */ }

• Provide either a strategy enum on JsonApiOptions or just require the user to inject the service.

services.AddJsonApi(options => {
  options.QueryParsingStrategy = QueryParsingStrategy.KeepRawValues;
});

services.AddScoped<RawValueQueryParamParser, DefaultQueryParamParser>();

In a future major release we might make this the default behavior. I suspect it will be necessary to address #408.