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

POC for Polymorphism #890

Merged
merged 22 commits into from
Sep 13, 2024
Merged

POC for Polymorphism #890

merged 22 commits into from
Sep 13, 2024

Conversation

dabeck81
Copy link
Contributor

@dabeck81 dabeck81 commented Aug 1, 2024

POC for Polymorphism

Copy link

netlify bot commented Aug 1, 2024

Deploy Preview for springwolf-ui ready!

Name Link
🔨 Latest commit af6c89e
🔍 Latest deploy log https://app.netlify.com/sites/springwolf-ui/deploys/66e451f834e37f0008b0c23e
😎 Deploy Preview https://deploy-preview-890--springwolf-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TypeToClassConverter#extractClass is only used by the cloudstream plugin now.
A next step (possibly outside of the PR) can be to apply the same Class -> Type change to the cloudstream plugin and so that TypeToClassConverter can be fully removed, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, that was what I was thinking as well

@@ -227,7 +227,7 @@
},
"name": "io.github.springwolf.examples.amqp.dtos.AnotherPayloadDto",
"title": "AnotherPayloadDto",
"description": "Another payload model",
"description": "",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Were you able to find out, why the description disappears?

Copy link
Contributor Author

@dabeck81 dabeck81 Aug 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The description that is shown is the description of the schema, so the logical place to show this description is on the Schema-component, as it is already.
The description that is on that place, should be the one of the @asyncmessage, in my opinion.

The reason why the description is not anymore shown, is that Swagger creates now the reference to the schema-object, whereas before it was a hardcoded reference by Springwolf itself. Due to removing that hardcoded reference, we do not receive anymore the full schema, but an already referenced schema-object on the payload, without a description.
If this is a blocking matter, what could be done is:

  • Either, fetching the schema from the swagger-reference and fetching the description via that way.
  • Either, setting the description on the reference-schema with a custom modelConverter.

Copy link
Member

@timonback timonback Sep 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point regarding the location of the description and it seems that the description does not belong here.

Springwolf has two ways to specify an optional description via the Swagger Schema annotation (belongs to the schema) and via the AsyncOperation annotation (belongs to the operation).

Can the description be null in that case instead of an empty string?
(I am currently unsure, whether the ui selects the description already from the correct location)

EDIT: Just compared the PR preview https://deploy-preview-890--springwolf-ui.netlify.app/springwolf-ui/asyncapi-ui.html#channel-string-topic and the current state https://demo.springwolf.dev/springwolf-ui/asyncapi-ui.html#channel-string-topic for the string topic and the ui seems to handle channel Operation description and Message description correctly.

@@ -25,8 +25,8 @@
"another-topic": {
"address": "another-topic",
"messages": {
"io.github.springwolf.examples.kafka.dtos.AnotherPayloadDto": {
"$ref": "#/components/messages/io.github.springwolf.examples.kafka.dtos.AnotherPayloadDto"
"java.util.ListIo.github.springwolf.examples.kafka.dtos.AnotherPayloadDto": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The case of List in combination with batch listeners may be debatable.
I've added a ConsumerRecord listener to the kafka example. Using the extractable-classes, do you see an easy way to auto-extract the actual class?
Another modelConverter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, I must admit, that indeed the consumerRecord does not get extracted the way it should. A "modelConverter" is always a good idea to adapt the schema to the way we would like. I think this is a same case as with the MonetaryExample, where Swagger does not see the defined fields of the class

I haven't tried the example on the current code of Springwolf, how it generates. I suppose quite the same.

An idea that comes to my mind is that maybe for each of the different "support-bindings", some custom modelconverters could be added on the classpath. So for example this case: The class ConsumerRecord belongs to the Kafka framework, so only when that binding is selected, the support for consumerRecord could be added with a custom modelconverter. You could define that modelconverter bean on the SpringwolfKafkaBindingAutoConfiguration.

I suppose each framework has its special quircks, so adding custom modelconverters for each of them to the binding-autoconfiguration seems like the way to go.

I don't know what you think.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some code as mentioned in #891 (comment)

If you don't feel strongly about it, I would like to minimize breaking changes and continue to extract List

Schema payloadSchema = resolvedSchema.schema;
preProcessSchemas(payloadSchema, preProcessSchemas, type);
HashMap<String, Schema> postProcessSchemas = new HashMap<>(preProcessSchemas);
postProcessSchema(preProcessSchemas, postProcessSchemas, actualContentType);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am probably missing something:
Is this part of the duplication, you mentioned? It seems that the same schemas are post processed again and/or the resolvedSchema is not post-processed.

Side-note: I didnt know about ResolvedSchema. Great improvement to use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The duplication is more in the methods, I simply duplicated, like:

postProcessSchema
processAsyncApiPayloadAnnotation
processSchemaAnnotation
processCommonModelConverters

Those 4 methods are twice in the swaggerSchemaService. Once for the old way of defining schemas (still in use for headers) and once for the new way of defining schemas (for defining the payload).
So, as part of a next refactoring the header-generation could be adapted to make also use of "resolvedSchema" and then those duplicate methods can go away.

timonback added a commit to timonback/springwolf-core that referenced this pull request Aug 16, 2024
extracted from springwolf#890

Co-authored-by: David Beck <[email protected]>
timonback added a commit to timonback/springwolf-core that referenced this pull request Aug 16, 2024
timonback added a commit that referenced this pull request Aug 16, 2024
extracted from #890

Co-authored-by: David Beck <[email protected]>
timonback added a commit to timonback/springwolf-core that referenced this pull request Aug 16, 2024
timonback added a commit that referenced this pull request Aug 16, 2024
* refactor(core): rename NamedSchemaObject to PayloadSchemaObject

extracted from #890

Co-authored-by: David Beck <[email protected]>

* refactor(core): use SchemaType constants

---------

Co-authored-by: David Beck <[email protected]>
@timonback timonback reopened this Sep 10, 2024
@timonback timonback merged commit 096c8c4 into springwolf:master Sep 13, 2024
24 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants