asyncapi / modelina

A library for generating typed models based on inputs such as AsyncAPI, OpenAPI, and JSON Schema documents with high customization
https://modelina.org
Apache License 2.0
295 stars 170 forks source link

AnonymousSchema file is generated for Enum #1576

Open Tenischev opened 10 months ago

Tenischev commented 10 months ago

Describe the bug

AnonymousSchema class file is generated for Enum instead of Enum with normal name. Issue is valid for both JavaFileGenerator and JavaGenerator generators. Issue found during exploration of possibility to use Modelina for model generation in Java spring template, see #https://github.com/asyncapi/java-spring-template/pull/342

How to Reproduce

  1. Use API
  2. Generate with Modelina by:
    
    const modelina = require('@asyncapi/modelina')
    const path = require("path");

module.exports = { 'generate:before': generator => { const javaGenerator = new modelina.JavaFileGenerator();

    javaGenerator.generateToFiles(generator.asyncapi, path.resolve(generator.targetDir, 'src/main/java/com/asyncapi/modelina/'), {
        collectionType: "List",
        presets: [
            {
                preset: modelina.JAVA_COMMON_PRESET,
                options: {
                    equal: true,
                    hashCode: true,
                    classToString: true
                }
            }
        ]
    }, true)
}

};

3. Find in target directory `AnonymousSchema_2` file. This file corresponds to enum property of element from the API:
    command:
      type: string
      enum:
        - on
        - off
      description: Whether to turn on or off the light.
      x-pi: false


#### Expected behavior
The name of class file should be `Command` or `CommandEnum` like the property name, not `AnonymousSchema`.
Also I do not see a reason to generate this Enum in a separate class file.
Tenischev commented 10 months ago

@magicmatatjahu @jonaslagoni @kennethaasan ask for comments

jonaslagoni commented 10 months ago

The name of class file should be Command or CommandEnum like the property name Might be better in some cases, but what happens when you encounter two command properties across the AsyncAPI document, with two different entries?

Property names are seriously unstable to use as inferring names, which might work in some cases.

Say we changed to use it, the behavior that Modelina would take is to merge those two models if they have the same ID. Would that be desirable?

But would be happy to access a PR that adaptively changes the naming strategy of the AsyncAPI processor through an option, i.e. we can have multiple variants of this function: https://github.com/asyncapi/modelina/blob/e2c4fa349da480a990d42365e06afda6015463bb/src/processors/AsyncAPIInputProcessor.ts#L195 or reuse them where it makes sense.

Would you want to provide a PR for it @Tenischev ?

Also I do not see a reason to generate this Enum in a separate class file.

All individual models are generated into separate files with generateToFiles, you can use generate to mix and match the models as you see fit after the generation process and band them together.

Tenischev commented 10 months ago

@jonaslagoni I want to mention that problem with naming will not occur if not generate dedicated class file for the parameter which is not an object by schema definition. Now, Modelina expand component model described in API and create a problem from this behaviour. There will be no problem with naming, if "Command or CommandEnum" will be sub-class (property element) for turnOnOffPayload class like it written in the API. I will have a look what generate could do, but as a developer I want to have tool that simply generates class files for my model, not reading the docs to understand why tool behave strange. Generation of AnonymousSchema when model doesn't contain any anonymous object is not correct work.

jonaslagoni commented 10 months ago

There will be no problem with naming, if "Command or CommandEnum" will be sub-class (property element) for turnOnOffPayload class like it written in the API.

Sure would be possible to do, but then if the same enum is referenced in multiple objects, you cannot reuse the instance across them because they will be in different packages 🙂

I will have a look what generate could do, but as a developer I want to have tool that simply generates class files for my model, not reading the docs to understand why tool behave strange. Generation of AnonymousSchema when model doesn't contain any anonymous object is not correct work.

Each developer will have their own expectation of correct behavior, we will never be able to create a one-fits-all, that's why the tool enables you to mold it as you see it.

Tenischev commented 10 months ago

Sure would be possible to do, but then if the same enum is referenced in multiple objects, you cannot reuse the instance across them because they will be in different packages 🙂

This enum is a property of particular object. If API creator would like to reuse enum there all possibilities to make it clear from schema definition by placing it as a separate object. Why Modelina overthink API and schema design? Why Modelina changes what is written in API? And OK, maybe "user would like to reuse Enum", but why only Enum? Why not a string or an integer or an array? Let's place every property of each object as a separate class, no need to stop the fantasy.

jonaslagoni commented 10 months ago

This enum is a property of particular object. If API creator would like to reuse enum there all possibilities to make it clear from schema definition by placing it as a separate object.

Modelina can't know that. We could enable customization through extensions that then control the algorithm slightly. Same with overwriting the behavior with options.

Same with the processing, you are free to create your own processor, in your own repo that explicitly changes how the meta-models are created 🙂 All up to you.

Why Modelina overthink API and schema design? Why Modelina changes what is written in API?

Modelina doesn't care about it being an API. It only changes what is given, to ensure it's valid values in the language it's constrained in. Nowhere in any JSON Schema standard is it defined that the property should act as a name for the enum, it's one interpretation 🙂

And OK, maybe "user would like to reuse Enum", but why only Enum? Why not a string or an integer or an array? Let's place every property of each object as a separate class, no need to stop the fantasy.

If you want that go for it 😛 Modelina will enable you to do it for sure 👍 Just create your own generator, with split being all types, and add a renderer for it 😉

Enum + Classes is just something that every language offers, so it was standardized like that across generators.

github-actions[bot] commented 6 months ago

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

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 :heart:

rquinio1A commented 4 months ago

Coming from openapi-generator-maven-plugin, I was also suprised by these generated AnonymousSchemaN Java classes that look a bit ugly.

Sorry if I missed this in the discssion above, but what should I do to customize the enum class names ? It might be worth a section in https://modelina.org/docs/languages/java ?