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

Constants are compiled as enums #1471

Closed
Ksisa opened this issue Aug 10, 2023 · 13 comments · Fixed by #1801
Closed

Constants are compiled as enums #1471

Ksisa opened this issue Aug 10, 2023 · 13 comments · Fixed by #1801
Labels
bug Something isn't working released on @next

Comments

@Ksisa
Copy link
Contributor

Ksisa commented Aug 10, 2023

Describe the bug

If I have a value that's a constant on an object, it will get compiled into an anonymous schema. This overcomplicates things by a lot.

Input:

    TradeMessageClient:
      name: Trade Message (client)
      title: Trade Message (client)
      description: A message containing trade data
      payload:
        title: TradePayloadClient
        type: object
        properties:
          operation:
            type: string
            const: Trade
          data:
            $ref: '#/components/schemas/TradeClient'
        additionalProperties: false
        required:
          - operation
          - data

Expected TypeScript output:

interface TradePayloadClient {
  operation: "Trade"
  data: TradeClient;
}

Actual Typescript Output:

interface TradePayloadClient {
  operation: AnonymousSchema_11;
  data: TradeClient;
}
enum AnonymousSchema_11 {
  TRADE = "Trade",
}

Expected C# output:

public class TradePayloadClient
{
    public const string Operation = "Trade";
    public TradeClient? Data { get; set; }
}

Actual C# output:

public class TradePayloadClient
  {
    public AnonymousSchema_11? Operation { get; set; }
    public TradeClient? Data { get; set; }
  }

public enum AnonymousSchema_11
  {
    TRADE
  }

  public static class AnonymousSchema_11Extensions
  {
    public static string? GetValue(this AnonymousSchema_11 enumValue)
    {
      switch (enumValue)
      {
        case AnonymousSchema_11.TRADE: return "Trade";
      }
      return null;
    }

    public static AnonymousSchema_11? ToAnonymousSchema_11(string? value)
    {
      switch (value)
      {
        case "Trade": return AnonymousSchema_11.TRADE;
      }
      return null;
    }
  }
@Ksisa Ksisa added the bug Something isn't working label Aug 10, 2023
@github-actions
Copy link
Contributor

Welcome to AsyncAPI. Thanks a lot for reporting your first issue. Please check out our contributors guide and the instructions about a basic recommended setup useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.

@jonaslagoni
Copy link
Member

@Ksisa try and switch over to the next version, it has this updated 🙂

@Ksisa
Copy link
Contributor Author

Ksisa commented Aug 10, 2023

@jonaslagoni for Typescript, I get this, which is invalid

interface TradePayloadClient {
  operation: 'Trade' = 'Trade';
  data: TradeClient;
}

And for C#, it doesn't retain the constant's value

public class TradePayloadClient
{
  public string Operation { get; set; }
  public TradeClient Data { get; set; }
}

@jonaslagoni
Copy link
Member

jonaslagoni commented Aug 10, 2023

@jonaslagoni for Typescript, I get this, which is invalid

How is that input invalid? 🤔

And for C#, it doesn't retain the constant's value

Yea C# still need a few things

@Ksisa
Copy link
Contributor Author

Ksisa commented Aug 10, 2023

@jonaslagoni It's an interface, variables can't be initialized.
image

@jonaslagoni
Copy link
Member

Ahh yea 😅

So it's part of https://github.com/asyncapi/modelina/blob/042b45e38ae6a08c6a3ef7df9feb1a724eb63969/src/generators/typescript/TypeScriptObjectRenderer.ts#L49C70-L49C75 that is called by

return renderer.renderProperty(property);
, so we just need to provide conditional check whether we render for interface or class, and if interface only render the types.

@Ksisa wanna provide a fix for it?

@Ksisa
Copy link
Contributor Author

Ksisa commented Aug 10, 2023

Yeah, I can take a look tomorrow evening.

@jonaslagoni
Copy link
Member

@Ksisa wanna have a look at the c# generator as well?

It is slightly more complicated than your last PR.

First part is implementing the constant constrainer for C#: https://github.com/asyncapi/modelina/blob/next/src/generators/csharp/constrainer/ConstantConstrainer.ts you can use the TypeScript constant constrainer as a guideline.

Then in the generator, you just need to adapt the property rendering using the .options.const.value value, by altering it here:

return `private ${property.property.type} ${property.propertyName}${nullablePropertyEnding};`;

@Ksisa
Copy link
Contributor Author

Ksisa commented Aug 11, 2023

@jonaslagoni what is the constant constrainer used for? I can see the Java ConstantConstrainer is identical to the TypeScript one, so I'm tempted to just copy paste it, especially since Java is syntactically very similar to C#.

Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity 😴

It will be closed in 120 days if no further activity occurs. To unstale this issue, add a comment with a detailed explanation.

There can be many reasons why some specific issue has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this issue forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️

@github-actions github-actions bot added the stale label Dec 10, 2023
@jonaslagoni
Copy link
Member

@jonaslagoni what is the constant constrainer used for? I can see the Java ConstantConstrainer is identical to the TypeScript one, so I'm tempted to just copy paste it, especially since Java is syntactically very similar to C#.

It renders the constant value in language at hand. Some languages will overlap in that others will be unique. So it's vary possible that the implementation is gonna be the same or very similar between them 🙂

@github-actions github-actions bot removed the stale label Dec 11, 2023
@moritzkalwa
Copy link
Contributor

@jonaslagoni Is const only implemented for strings? 🤔 If so, should there be a check on the property type in the generator so we don't generate junk (e.g. private const double property = undefined)?

@asyncapi-bot
Copy link
Contributor

🎉 This issue has been resolved in version 4.0.0-next.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working released on @next
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants