search

crowdin / crowdin-api-client-dotnet

.NET client library for Crowdin API
https://www.nuget.org/packages/Crowdin.Api/
MIT License
52 stars 26 forks source link

BundleExporter.CheckBundleExportStatus cannot deserialize status "in_progress" properly. #167

Closed PatrickMNL closed 12 months ago

PatrickMNL commented 1 year ago

When the BundleExporter.CheckBundleExportStatus returns a status of "in_progress" we run into the following JSON deserialization issue;

System.ArgumentException: Error occurred during deserialization from JSON String
   at Crowdin.Api.Core.Converters.DescriptionEnumConverter.ReadJson(JsonReader reader, Type objectType, Object existingValue, JsonSerializer serializer) in C:\Projects\crowdin-api-client-dotnet\src\Crowdin.Api\Core\Converters\DescriptionEnumConverter.cs:line 85
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.DeserializeConvertable(JsonConverter converter, JsonReader reader, Type objectType, Object existingValue)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.SetPropertyValue(JsonProperty property, JsonConverter propertyConverter, JsonContainerContract containerContract, JsonProperty containerProperty, JsonReader reader, Object target)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.PopulateObject(Object newObject, JsonReader reader, JsonObjectContract contract, JsonProperty member, String id)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateObject(JsonReader reader, Type objectType, JsonContract contract, JsonProperty member, JsonContainerContract containerContract, JsonProperty containerMember, Object existingValue)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateValueInternal(JsonReader reader, Type objectType, JsonContract contract, JsonProperty member, JsonContainerContract containerContract, JsonProperty containerMember, Object existingValue)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.Deserialize(JsonReader reader, Type objectType, Boolean checkAdditionalContent)
   at Newtonsoft.Json.JsonSerializer.DeserializeInternal(JsonReader reader, Type objectType)
   at Newtonsoft.Json.JsonConvert.DeserializeObject(String value, Type type, JsonSerializerSettings settings)
   at Newtonsoft.Json.JsonConvert.DeserializeObject[T](String value, JsonSerializerSettings settings)
   at Crowdin.Api.Core.JsonParser.ParseResponseObject[TData](JObject rootElement) in C:\Projects\crowdin-api-client-dotnet\src\Crowdin.Api\Core\JsonParser.cs:line 53
   at Crowdin.Api.Bundles.BundlesApiExecutor.CheckBundleExportStatus(Int32 projectId, Int32 bundleId, String exportId) in C:\Projects\crowdin-api-client-dotnet\src\Crowdin.Api\Bundles\BundlesApiExecutor.cs:line 133
   at OSM.Translations.Api.Services.CrowdinService.ExportBundle(Bundle bundleDefinition) in C:\Projects\OSM.Translations\OSM.Translations.Api\Services\CrowdinService.cs:line 55
   at OSM.Translations.Api.Services.CrowdinService.GetBundle(String bundleName) in C:\Projects\OSM.Translations\OSM.Translations.Api\Services\CrowdinService.cs:line 21
   at OSM.Translations.Api.V1.TranslationController.GetCrowdinTranslationsBundleAsZip(String bundleName) in C:\Projects\OSM.Translations\OSM.Translations.Api\V1\TranslationController.cs:line 23
   at Microsoft.AspNetCore.Mvc.Infrastructure.ActionMethodExecutor.TaskOfIActionResultExecutor.Execute(IActionResultTypeMapper mapper, ObjectMethodExecutor executor, Object controller, Object[] arguments)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.<InvokeActionMethodAsync>g__Awaited|12_0(ControllerActionInvoker invoker, ValueTask`1 actionResultValueTask)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.<InvokeNextActionFilterAsync>g__Awaited|10_0(ControllerActionInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Rethrow(ActionExecutedContextSealed context)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.<InvokeInnerFilterAsync>g__Awaited|13_0(ControllerActionInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeFilterPipelineAsync>g__Awaited|20_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeAsync>g__Logged|17_1(ResourceInvoker invoker)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeAsync>g__Logged|17_1(ResourceInvoker invoker)
   at Microsoft.AspNetCore.Routing.EndpointMiddleware.<Invoke>g__AwaitRequestTask|6_0(Endpoint endpoint, Task requestTask, ILogger logger)
   at Microsoft.AspNetCore.Authorization.AuthorizationMiddleware.Invoke(HttpContext context)
   at Swashbuckle.AspNetCore.SwaggerUI.SwaggerUIMiddleware.Invoke(HttpContext httpContext)
   at Swashbuckle.AspNetCore.Swagger.SwaggerMiddleware.Invoke(HttpContext httpContext, ISwaggerProvider swaggerProvider)
   at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)

HEADERS
=======
Accept: application/json
Host: localhost:59954
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/114.0.0.0 Safari/537.36
:method: GET
Accept-Encoding: gzip, deflate, br
Accept-Language: en,nl;q=0.9,nl-NL;q=0.8
Referer: https://localhost:59954/swagger/index.html
sec-ch-ua: "Not.A/Brand";v="8", "Chromium";v="114", "Google Chrome";v="114"
sec-ch-ua-mobile: ?0
sec-ch-ua-platform: "Windows"
sec-fetch-site: same-origin
sec-fetch-mode: cors
sec-fetch-dest: empty

Expected: It should properly parse the status to BuildStatus.InProgress.

Reason: The Enum definition has a nice Description attribute above it which is read by a custom deserializer, but it is initialized with inProgress instead of in_progress (perhaps because it has been changed recently?)

PatrickMNL commented 1 year ago

@andrii-bodnar I don't mind picking this up myself, if I can get permissions to open pull requests.

andrii-bodnar commented 1 year ago

@PatrickMNL great, thank you! 🚀

Feel free to fork the repo and create a PR

PatrickMNL commented 1 year ago

After some investigation, I found that both these status representations are being returned by the v2 API;

A proper fix should probably be done on the API server side instead of in this package.

I can still offer a fix, but I don't really like the options I have of fixing it, what I considered:

andrii-bodnar commented 1 year ago

@PatrickMNL thanks a lot for the investigation!

This should definitely be fixed on the API side. Passing it on to the development team

PatrickMNL commented 1 year ago

Alright, glad we're on the same page, thanks @andrii-bodnar! Would u have a ballpark estimate about when we can expect the fix? Just checking if it's worth waiting or that we should build in a small work around for now.

andrii-bodnar commented 1 year ago

@PatrickMNL I will provide the exact estimate as soon as possible. It would be great if we could make a temporary workaround just to not block further integration.

PatrickMNL commented 1 year ago

@andrii-bodnar I have added 'temporary workaround' suggestion in this PR: https://github.com/crowdin/crowdin-api-client-dotnet/pull/169

PatrickMNL commented 1 year ago

Thanks for considering and merging the PR @andrii-bodnar, when can we expect a new release of the NuGet package? I would like to start using it with the fixes included 😄.

andrii-bodnar commented 1 year ago

@PatrickMNL sure, I am going to release a new version in a few minutes

andrii-bodnar commented 1 year ago

@PatrickMNL just released a new version - 2.14.3

PatrickMNL commented 1 year ago

@andrii-bodnar Thanks for the release! I see that the NuGet feed did not yet receive the new version, does that take a while or was it perhaps missed during the update?

PatrickMNL commented 1 year ago

Ignore the nuget feed question, I see it just got updated! Thanks again 👍!

innomaxx commented 1 year ago

@PatrickMNL thanks for your contribution to this project!

@andrii-bodnar can we consider migration to permanent solution for this case? For example add extra [Description] attribute with "in_progress" value or create separate enum for Bundle API. As OP mentioned before.

Probably new value from API is already used by end users. We can avoid breaking backward compatibility just by fixing this on client side

PatrickMNL commented 1 year ago

@innomaxx thanks for the suggestions, I did also consider the second [Description] attribute, but unfortunately that .NET attribute doesn't allow for 2 occurences of the attribute on the same property.

We could also move away from the attribute and instead use a custom one, that allows for multiple names on a property. Or we could indeed consider the seperate enum for the BundleAPI (if we really want to fix this clientsided), both would require small adjustments to the current serialization flow.

I can see that backwards compatibility would indeed break for people with custom implementations, made during the time the Bundle API send back 'in_progress'.

andrii-bodnar commented 1 year ago

Hi @PatrickMNL,

let's create a new class for bundle build status.

Having the status response in the _snakecase is more correct.

Due to some historical reasons, it was introduced in camelCase for the Build Project Translation and Build Project Directory Translation API methods and can't be changed now.

Currently, the following API endpoints have the _snakecase status response:

as well as the correcponding Check ... status

PatrickMNL commented 12 months ago

@andrii-bodnar thanks for the reply.

Can do, definitely, are you aware that introducing the new enum (class?), would cause 'breaking changes' when updating the package, as in, people will need to move their own Bundle implementations to this newly created enum.

If we're okay with that, than I can introduce a PR for that.

Something small to consider; If we're introducing breaking changes, we are also able to fix the existing BuildStatus and move the older incorrectly named variants to something like a LegacyBuildStatus.

andrii-bodnar commented 12 months ago

@PatrickMNL Yes, we'll add a warning to the release notes about these breaking changes.

Something small to consider; If we're introducing breaking changes, we are also able to fix the existing BuildStatus and move the older incorrectly named variants to something like a LegacyBuildStatus.

I think this is a good idea!

PatrickMNL commented 12 months ago

@andrii-bodnar I have created the above PR as potential fix, I am able to test the Bundle API against our live implementation, we don't have a Translation/Directory API implementation internally, so I am unable to test those, is that something you can verify?

andrii-bodnar commented 12 months ago

@PatrickMNL thanks a lot!

We have some examples in this repo, maybe it would be possible to test the new code using these examples?