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

Hyphens with JsonStringEnumConverter results in JSON value could not be converted #300

Open
Xeevis opened this issue Jan 19, 2024 · 7 comments
Assignees
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@Xeevis
Copy link

Xeevis commented Jan 19, 2024

Describe the bug
With System.Text.Json there appears to be deficiency with JsonStringEnumConverter which can't convert hyphens to enums and it's also not supporting System.Runtime.Serialization.EnumMember attribute causing impasse.

Meaning this won't work:

public enum MarketplaceId
{
  [System.Runtime.Serialization.EnumMember(Value = @"allegro-pl")]
  AllegroPl = 0,
  [System.Runtime.Serialization.EnumMember(Value = @"allegro-cz")]
  AllegroCz = 1,
}

I've tried to create Fiddle demonstrating it, but Refit doesn't like that for some reason, but the code otherwise works https://dotnetfiddle.net/FraP82


Anyway I've found two ways to overcome this

  1. Revert Refit back to Json.NET, I'd very much like to avoid that
var refit = RestService.For<ISomeApi>(httpClient, new RefitSettings(new NewtonsoftJsonContentSerializer()));
  1. Use better custom converter, like JsonStringEnumMemberConverter I however can't get this to work with Refitter, because generated enums are set to concrete implementation avoiding any extra converters added to Refit.
[JsonConverter(typeof(JsonStringEnumConverter))]
public SomeEnum Id { get; set; }

Thanks for any insight how to get this to work. I'll send a coffee ☕ your way once I get this thing fully working I swear 😁.


OpenAPI Specifications
https://developer.allegro.pl/swagger.yaml

Additional context

Failing operation:

[Headers("Accept: application/vnd.allegro.public.v1+json")]
[Get("/sale/offers")]
Task<OffersSearchResultDto?> SearchOffersUsingGET()

Failing generated code

/// <summary>
/// Identifies a marketplace.
/// </summary>
[System.CodeDom.Compiler.GeneratedCode("NJsonSchema", "14.0.1.0 (NJsonSchema v11.0.0.0 (Newtonsoft.Json v13.0.0.0))")]
public partial class MarketplaceReference
{

  [JsonPropertyName("id")]
  [System.ComponentModel.DataAnnotations.Required(AllowEmptyStrings = true)]
  [JsonConverter(typeof(JsonStringEnumConverter))]
  public MarketplaceId Id { get; set; } = default!;

}

/// <summary>
/// The id of a marketplace.
/// </summary>
[System.CodeDom.Compiler.GeneratedCode("NJsonSchema", "14.0.1.0 (NJsonSchema v11.0.0.0 (Newtonsoft.Json v13.0.0.0))")]
public enum MarketplaceId
{

  [System.Runtime.Serialization.EnumMember(Value = @"allegro-pl")]
  AllegroPl = 0,

  [System.Runtime.Serialization.EnumMember(Value = @"allegro-cz")]
  AllegroCz = 1,

}
@Xeevis Xeevis added the bug Something isn't working label Jan 19, 2024
@christianhelle
Copy link
Owner

@Xeevis Thanks for taking the time to report this

It's a bit tricky with the generated contracts as I fully depend on what NSwag has to offer to generate them. Maybe there's an option somewhere to control this in NSwag, let me see what I can do

@Xeevis
Copy link
Author

Xeevis commented Jan 22, 2024

@christianhelle Hello, any progress on this? I guess I might just go with postprocessing generated code, simple "search and replace" to get rid of hardcoded attributes.

- [JsonConverter(typeof(JsonStringEnumConverter))]

This may not be the correct place to ask, but Is there even any particular reason why generic converters are hardcoded? Doesn't make any sense to me, anyone can just define their own set of converters on the serializer which is flexible and more concise than decorating every single enum.

var jsonSerializerOptions = new JsonSerializerOptions()
{
  WriteIndented = true,
  DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingDefault
};

// jsonSerializerOptions.Converters.Add(new JsonStringEnumConverter());
jsonSerializerOptions.Converters.Add(new JsonStringEnumMemberConverter());
jsonSerializerOptions.Converters.Add(new DateTimeOffsetNullHandlingConverter());

@christianhelle
Copy link
Owner

@Xeevis Sorry for the delay. Work, life, and other things got in the way

NSwag, the tool I use to generate contracts and parse the OpenAPI document has a code generator setting called JsonConverters which allows you to specify a string array of JsonConverters to add to the serialization options. I have little to no experience with this but have you tried setting this in your .refitter settings file?

{
  "openApiPath": "/path/to/your/openAPI",
  "codeGeneratorSettings": {
    "jsonConverters": [
      "JsonStringEnumMemberConverter",
      "DateTimeOffsetNullHandlingConverter",
    ]
  }
}

Search and replace is alright when this is only done once or twice, but if you need to re-generate code every time, and then clean up afterwards, it gets very tedious, very quickly.

I noticed that the docs for the .refitter file format are lacking this information so I will update that at some point, when I have a bit more time on my hands

@Xeevis
Copy link
Author

Xeevis commented Jan 22, 2024

Thank you for response. Doesn't seem to work, found this in issue Custom JsonConverters missing in generation

NSwag translates your code into an OpenAPI spec - it cannot understand the procedural nature of custom converters - so you need to manually transform the spec so that it correctly describes what your IsoDateConverter is doing... and then the generators need to produce the right code based on the spec - independent of how you actually implemented it in C#.

Found another issue relating to my exact issue, there it's suggested to override template, any idea how to do that with Reffiter?
Feature request: possibility to replace the JsonStringEnumConverter for System.Text.Json

I was able to change the attribute using template overrides (https://github.com/RicoSuter/NSwag/wiki/Templates). It would still be nice to have it as a configurable option, as to remove the need for a template override; however, this is now a non-issue to all my intents, constructions and purposes.

@christianhelle
Copy link
Owner

Found another issue relating to my exact issue, there it's suggested to override template, any idea how to do that with Reffiter?
Feature request: possibility to replace the JsonStringEnumConverter for System.Text.Json

@Xeevis As far as I know, NSwag uses liquid templates to generate the API client. I don't know if they also use it to generate the contracts.

I will work on a solution for this in the next few days (or weeks) and will keep you in the loop. One of the APIs I use at work uses an _ in the enum values so I have the same problem as you have

My problem OpenAPI spec looks like this:

    ChargingFacility:
      type: object
      description: ChargingFacility model
      properties:
        Amperage:
          type: integer
          format: int32
        ChargingModes:
          type: array
          description: List of charging modes that are supported.
          items:
            type: string
            enum:
              - Mode_1
              - Mode_2
              - Mode_3
              - Mode_4
              - CHAdeMO
        PowerType:
          type: string
          enum:
            - AC_1_PHASE
            - AC_3_PHASE
            - DC
        Voltage:
          type: integer
          format: int32
        Power:
          type: number
          minimum: 0.0001
      required:
        - PowerType
        - Power

@Xeevis
Copy link
Author

Xeevis commented Jan 23, 2024

Indeed JsonStrinEnumConverter leaves much to be desired and it's a shame Microsoft won't address it. This leaves everyone who doesn't control inputs in search of custom implementation.

If this proves to be impossible or too much time consuming, here is how I solved it in the meantime with PowerShell:

Write-Host "Downloading latest swagger.yaml from Allegro.pl ... " -ForegroundColor Yellow -NoNewline
Invoke-WebRequest https://developer.allegro.pl/swagger.yaml -OutFile swagger.yaml
Write-Host "Done ✅" -ForegroundColor Green

Get-ChildItem -Recurse -Filter "*.refitter" -ErrorAction "SilentlyContinue" | ForEach-Object {
    Write-Host "Compiling $($_.FullName) ..." -ForegroundColor Yellow -NoNewline
    $output = & dotnet refitter --settings-file $_.FullName --no-logging --no-banner
    # Write-Output $output # Write out OpenAPI statistics
    Write-Host "Done ✅" -ForegroundColor Green
    $generatedFilePath = $output | Select-String -Pattern "Output\:\s(.*\.cs)" | ForEach-Object { $_.Matches.Groups[1].value }
    Write-Host "Postprocessing output for $($generatedFilePath) ..." -ForegroundColor Yellow -NoNewline
    # Use improved String Enum converter that can handle underscores, spaces and hyphens
    (Get-Content -Path $generatedFilePath) -Replace 'JsonStringEnumConverter', 'JsonStringEnumMemberConverter' | Set-Content -Path $generatedFilePath
    Write-Host "Done ✅" -ForegroundColor Green
}

Write-Host "... Code generation completed!" -ForegroundColor Green

@christianhelle
Copy link
Owner

@Xeevis Thanks for the PowerShell script!

@christianhelle christianhelle added the help wanted Extra attention is needed label Jan 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants