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

Model Substitution #98

Closed
magx2 opened this issue Oct 30, 2023 · 31 comments
Closed

Model Substitution #98

magx2 opened this issue Oct 30, 2023 · 31 comments
Labels
enhancement New feature or request

Comments

@magx2
Copy link

magx2 commented Oct 30, 2023

Let say I have object:

public class HumanIdDto {
  private int id;
  // constructors, getters, setters...
}

and endpoint:

@GetRequest('/api/human')
public Human findHuman(@RequestParam("id") HumanIdDto id) {
  //...
}

By default your plugin will create an HumanIdDto object that contains only one field which is technically OK, but it generates "stupid" objects in API.

Can you add some kind of mapping objects to other object, i.e.:

com.acme.HumanIdDto:java.lang.Integer

In this case java class HumanIdDto will be represented as Integer in Open API. Here you have example of plugin for Swagger 2: https://github.com/kongchen/swagger-maven-plugin#sample-model-substitution

@kbuntrock
Copy link
Owner

This is a very good point. Was thinking about it recently since I cannot know in advance every future java class which need a different mapping (like BigDecimal, LocalDateTime, ...).

Do you want this functionality in the 0.0.17 release? (can take me a few days since I have some plans for my evenings this week. ;) )

@magx2
Copy link
Author

magx2 commented Oct 30, 2023

I'm OK with having it in version 18

@magx2
Copy link
Author

magx2 commented Oct 30, 2023

BTW this format that I've showed you is only for example. Personally I think it's terrible.

I think that something like this would be better:

<modelSubstitutions>
  <modelSubstitution>
    <from>com.acme.HumanIdDto</from>
    <to>java.lang.Integer</to>
  <modelSubstitution>
</modelSubstitutions>

@kbuntrock kbuntrock added the enhancement New feature or request label Nov 2, 2023
@magx2
Copy link
Author

magx2 commented Nov 3, 2023

Any update on this?

@kbuntrock
Copy link
Owner

Code wise, now, but I thought about it this week. :)
I'm thinking about going a bit further and opening more the generator.

@magx2
Copy link
Author

magx2 commented Nov 5, 2023 via email

@kbuntrock
Copy link
Owner

It's more about the time I have to allow to this project. :) But I'll try to work on it tomorrow evening.

@kbuntrock
Copy link
Owner

Hello @magx2 !

Busy week, lot of work and I'll not be able to work on it today. But I still think about you!

If you really need an emergency release, you can start from your copy of the plugin and modify the OpenApiDataType#fromJavaClass function to directly put your model substitution.
Then "mvn install". The snapshot version is now in your local .m2 cache.
To share it for the project so your coworker don't have to do anything, you then can create a small maven repository (basically just a directory tree to the plugin pom+jar) and reference it in your pom with this configuration :

<pluginRepositories>
	<pluginRepository>
		<id>repo</id>
		<url>file://${project.basedir}/project_m2</url>
	</pluginRepository>
</pluginRepositories>

I know it's not perfect, but maybe it can help you to wait more peacefully the next release. :)

Wish you a good evening.

@magx2
Copy link
Author

magx2 commented Nov 7, 2023

Thanks! It works as a temporary solution, but still I need the fix ;)

magx2 pushed a commit to magx2/openapi-maven-plugin that referenced this issue Nov 7, 2023
@magx2
Copy link
Author

magx2 commented Nov 7, 2023

I've added a PR with changes that are required #102 .

I know it's not fully done but if you don't have a time you can start from there (FYI there are also changes from the PR #101)

magx2 pushed a commit to magx2/openapi-maven-plugin that referenced this issue Nov 7, 2023
@magx2
Copy link
Author

magx2 commented Nov 14, 2023

Any update on this?

@kbuntrock
Copy link
Owner

Any update on this?

Yes, I was able to work on it this weekend. Still missing in my MR :

  • not wasting time scanning a substituted class
  • not referencing it in the schema section
  • some unit tests

@magx2
Copy link
Author

magx2 commented Dec 12, 2023

Hey, any update on this?

@kbuntrock
Copy link
Owner

kbuntrock commented Dec 13, 2023 via email

@magx2
Copy link
Author

magx2 commented Dec 14, 2023

FYI: Please make sure that mapping from class to class (not primitive) also works. Previously I did some fast PoC for mapping object to primitive (long) and it was working. Today I had to map from one class to another and it was not working, because I was only changing openApiType without originalType/javaType.

I think it would be good if you would have test for those cases:

  1. OBJECT -> primitive (long, int, string, ...)
  2. OBJECT -> OBJECT
  3. OBJECT -> ARRAY
  4. ARRAY -> OBJECT
  5. OBJECT -> date types

@magx2
Copy link
Author

magx2 commented Dec 22, 2023

Hey, do you have a branch with this functionality? Maybe I can build it locally and use it for now.

@magx2
Copy link
Author

magx2 commented Dec 27, 2023

Hey, do you have a branch with this functionality? Maybe I can build it locally and use it for now.

?

@magx2
Copy link
Author

magx2 commented Jan 2, 2024

Hey, did you had time to fix it?

@kbuntrock
Copy link
Owner

kbuntrock commented Jan 3, 2024

I've got this, but it is still not finished. Focusing on it for my new year resolutions so you can try it : https://github.com/kbuntrock/openapi-maven-plugin/pull/104/files

As you can see, I'm driving this in a different path than the kongchen plugin.

The openapi-model.yml is the file describing defaults types models (mainly the type / format pairs)
It can be extended / partially replaced by anything you want to configure.

Then there is the default "model-association.yml" file, describing how to link a java class to a model. And if it's by "equality" or "assignability". It can also be extended / partially replaced.
This should cover your scenarios n°1, 2, 3 and 5 in a very customizable way.

I'm not planning to handle scenario number 4. Do you have use cases for it?

@magx2
Copy link
Author

magx2 commented Jan 3, 2024

Awesome! Good that you are back in the game 😁.

I will take a look on your PR tommorow.

About point 4 - I don't have a use case of it. I was just listing all possibilities. After thinking about it in my opinion there is no use case for mapping array to object.

@kbuntrock
Copy link
Owner

Better to not dig into it then, it would be a genericity nightmare.

@magx2
Copy link
Author

magx2 commented Jan 10, 2024

8buwf9

@magx2
Copy link
Author

magx2 commented Jan 16, 2024

right?

@kbuntrock
Copy link
Owner

We can for sure now say it will not be for December. :) Working on it.

@kbuntrock
Copy link
Owner

Hello @magx2.

I've finally reached a point I'm keen to merge. But I first need to check if it fulfil your needs.
Would you mind testing this branch? #104

I have not yet wrote the documentation so I write here a draft to help you.

The class mapping is now based on two configuration files :
First : the description of component models :
https://github.com/kbuntrock/openapi-maven-plugin/pull/104/files#diff-3ffe90d665e945193398ecbf990a4840c2c7e49cff85aba8f7d408d7fa6bc827

Secondly, the mapping between descriptions and scanned class :
https://github.com/kbuntrock/openapi-maven-plugin/pull/104/files#diff-86546cf9c0181dd6b47c8c6fc7bb295a81adc3d25ef32f5207de714a52fb4027
A mapping is defined by class equality or class assignability.

The two linked files are the default ones, shipped with the plugin. But one can add an extra mapping file with the option <modelsAssociationsPath>path/to/custom-model-association.yml</modelsAssociationsPath>

And even an extra model file for more complex use cases with the following option :
<openapiModelsPath>path/to/custom-model-association.yml</openapiModelsPath>

Please tell me if it check your use case. And don't hesitate if you need an example of usage.

Sorry for the long development time and I wish you a good evening. :)

@magx2
Copy link
Author

magx2 commented Jan 29, 2024

I have a question: Why did you introduced 2 new files instead of having the configuration in pom.xml?

@magx2
Copy link
Author

magx2 commented Jan 29, 2024

And another question:

I have a class com.foo.Xyz and I want to map it to class com.foo.Abc. How can I do it?

@magx2
Copy link
Author

magx2 commented Jan 30, 2024

I've added it to my project and it looks good. I'm not supper happy with extra files (you could map the same thing in pom.xml configuration), but if you prefer it then it's fine

@kbuntrock
Copy link
Owner

Running out of time tonight. Thank you very much for your time spent testing and reading my PR.
Will try to finish reading your comments / questions tomorrow.

@kbuntrock
Copy link
Owner

Hello @magx2. Here some answers to your questions.

I have a question: Why did you introduced 2 new files instead of having the configuration in pom.xml?

The model file can describe complex types. For this one, I thought describing them in yml is easier. And yml + xml mix terribly.
For the mapping file, I did not even think about it, I was focused on the functionality.

But I like your suggestion. As for the freeFields or the default error section, I'll allow to write a definition in json in the xml, or to reference a file.
BTW : to be clear, when using file configurations, I advice to not put them in the resource folder since they should not be packed in the application.

I have a class com.foo.Xyz and I want to map it to class com.foo.Abc. How can I do it?

You can't. Imagining the potential use cases, I'm convinced mapping a class to another class is primarily used to map a forgotten/custom class. Here, we can map forgotten class (as you have seen with ZonedDateTime), but we can also create complete custom objects.

But I can be wrong. What about your use case? You really need to remap a class by another class?

@magx2
Copy link
Author

magx2 commented Feb 3, 2024

The model file can describe complex types. For this one, I thought describing them in yml is easier. And yml + xml mix terribly.
For the mapping file, I did not even think about it, I was focused on the functionality.

I think you are able to do this without mixing yml and xml

But I can be wrong. What about your use case? You really need to remap a class by another class?

It's just not to write open api yml myself but rather leave it to the plugin. I was able do it so you can ignore this request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants