Skip to content
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

Dynamic C# API Clients may not handle DateTime on QueryString when client and server cultures are different #3339

Closed
NecatiMeral opened this issue Mar 24, 2020 · 7 comments

Comments

@NecatiMeral
Copy link
Contributor

NecatiMeral commented Mar 24, 2020

Hi,

I'm encountering a issue when using dynamic c# api clients and having datetime parameters on my service dtos.

When using the swagger-UI, the following, working request is created:
http://localhost:44344/api/app/rosterPlanning?ContractNumber=1003666-01&Begin=2019-12-01%2000%3A00%3A00&End=2019-12-31%2000%3A00%3A00

The DynamicHttpProxyInterceptor creates the following request, which doesn't work:
http://localhost:44344/api/app/rosterPlanning?ContractNumber=1003666-01&Begin=01.12.2019+00%3A00%3A00&End=31.12.2019+00%3A00%3A00

The underlying issue is that the datetime value get's string encoded using the wrong cultureinfo.

{
  "code": null,
  "message": "Your request is not valid!",
  "details": "The following errors were detected during validation.\r\n - The value '31.12.2019 00:00:00' is not valid for End.\r\n - The value '01.12.2019 00:00:00' is not valid for Begin.\r\n",
  "validationErrors": [
    {
      "message": "The value '31.12.2019 00:00:00' is not valid for End.",
      "members": [
        "end"
      ]
    },
    {
      "message": "The value '01.12.2019 00:00:00' is not valid for Begin.",
      "members": [
        "begin"
      ]
    }
  ]
}

Example code

public interface IRosterPlanningAppService : IApplicationService
{
   Task<RosterEntryDto[]> GetAsync(ContractQueryDto query);
}

public class ContractQueryDto
{
	[Required]
	public string ContractNumber { get; set; }

	[Required]
	public DateTime Begin { get; set; }

	[Required]
	public DateTime End { get; set; }
}

I think this issue can be fixed in the Volo.Abp.Http.Client.DynamicProxying.UrlBuilder.AddQueryStringParameter method.

Is there any workaround (such as custom validation) to fix this issue?

@maliming maliming added this to the 2.4 milestone Mar 25, 2020
@maliming maliming self-assigned this Mar 25, 2020
@maliming
Copy link
Member

maliming commented Mar 25, 2020

hi @NecatiMeral
Can you share the content of the http request?

eg

ContractNumber=123&Begin=2020-03-25%2018%3A00%3A00&End=2020-03-26%2021%3A00%3A00

ContractNumber: 123
Begin: 2020-03-25 18:00:00
End: 2020-03-26 21:00:00

ContractNumber: 123
Begin: 2020-03-25%2018%3A00%3A00
End: 2020-03-26%2021%3A00%3A00

image

@NecatiMeral
Copy link
Contributor Author

NecatiMeral commented Mar 25, 2020

hi @maliming,

the faulty request contains the following parameters:

ContractNumber=1003666-01&Begin=01.12.2019+00%3A00%3A00&End=31.12.2019+00%3A00%3A00

ContractNumber: 1003666-01
Begin: 01.12.2019 00:00:00
End: 31.12.2019 00:00:00

ContractNumber: 1003666-01
Begin: 01.12.2019+00%3A00%3A00
End: 31.12.2019+00%3A00%3A00

The issue is the value.ToString(), which is used to build the query string.
As temporary workaround, I'm using a fork of the Volo.Abp.Http.Client-package, where I customized the following method. I consider this as a non-production grade hack to continue my work for now. 😄

Volo.Abp.Http.Client.DynamicProxying.UrlBuilder

private static void AddQueryStringParameter(StringBuilder urlBuilder, bool isFirstParam, string name, object value)
{
	urlBuilder.Append(isFirstParam ? "?" : "&");
	// NM@24.03.2020: HACK to make datetime-query parameter work independent from the clients culture
	if (value is DateTime dateTime)
	{
		urlBuilder.Append(name + "=" + System.Net.WebUtility.UrlEncode(dateTime.ToString("yyyy-MM-ddTHH:mm:ssZ")));
	}
	else
	{
		urlBuilder.Append(name + "=" + System.Net.WebUtility.UrlEncode(value.ToString()));
	}
}

Maybe a more standarized format should be used for datetime parameters. I would consider using ISO-8601 format with optional abbreviations (such as my format).

@maliming
Copy link
Member

hi @NecatiMeral
This is actually related to the current culture. I am researching whether the culture of QueryStringValueProviderFactory should be changed.
aspnet/Mvc#5335

@NecatiMeral
Copy link
Contributor Author

The server-side implementation expects a invariant-culture input.
Wouldn't it be simply enough to convert the parameters using the invariant culture?

I think this issue will occur on floating values (0.00 vs 0,00) too.

@maliming
Copy link
Member

maliming commented Apr 2, 2020

MVC has always used InvariantCulture for route data and query strings, ie things that go in the URL. The reasoning behind it is that if you have an app that is localized, you'd want URLs to be universal. If you don't like it, I'd suggest replacing the query and route ValueProviderFactories with your own that respect current culture. The value provider factories are intentionally light on functionality so you can replace them easily.

@hikalkan Should we change the default behavior of QueryStringValueProviderFactory?

https://github.com/dotnet/aspnetcore/blob/c565386a3ed135560bc2e9017aa54a950b4e35dd/src/Mvc/Mvc.Core/src/ModelBinding/QueryStringValueProviderFactory.cs#L30

@NecatiMeral
Copy link
Contributor Author

I don't think that we have to adjust anything on the QueryStringValueProviderFactory. The client should create the url-parameters culture independent, which is currently not the case. I think the server-side mustn't be changed at all.

I can submit a PR adressing this issue by making my hack (#3339 (comment)) more production ready.

@hikalkan
Copy link
Member

hikalkan commented Apr 3, 2020

The client should create the url-parameters culture independent, which is currently not the case

I agree, and also your solution seems good :) I will check & fix it. Thanks.

@hikalkan hikalkan changed the title Dynamic C# API Clients - DateTime handling Dynamic C# API Clients may not handle DateTime on QueryString when client and server cultures are different Apr 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants