GPU-2196: OpenAPI generation tests#746
Conversation
| try { | ||
| // set the channel | ||
| channelIdSet = setChannelToTransaction(templateListParams.nodeId); | ||
| channelIdSet = setChannelToTransaction(templateListParams.folderNodeId); |
There was a problem hiding this comment.
Duplicated query param nodeId causes API generation violation.
There was a problem hiding this comment.
It is not duplicated for this method here. It is duplicated for method TemplateResource.list(), but it is not used in that method, so that method should not use TemplateListParameterBean anyways.
So instead of changing the name of the query parameter, which is a breaking change (and confusing), better replace the TemplateListParameterBean in TemplateResource.list() by an apropriate alternative. Also this is a bugfix and should therefor be done for cms 6.3.x onwards.
|
|
||
| /* | ||
| * (non-Javadoc) | ||
| * @see com.gentics.contentnode.rest.api.ImageResource@loadContent(Integer id) |
There was a problem hiding this comment.
Unused + deprecated + makes turbulence on OpenAPI generation.
| <plugin> | ||
| <groupId>org.openapitools</groupId> | ||
| <artifactId>openapi-generator-maven-plugin</artifactId> | ||
| <version>7.19.0</version> |
There was a problem hiding this comment.
This also validates the generated openapi.json in the compile time.
| * <p>The Rest API client builds upon an underlying Jersey-client: detailed information about the use of the | ||
| * WebTarget-object (base) to build requests can be found at http://jersey.java.net/ .</p> | ||
| */ | ||
| public class RestClient { |
There was a problem hiding this comment.
Making the RestClient an interface is a breaking change. See https://www.gentics.com/Content.Node/cmp8/guides/rest_java_client.html
| try { | ||
| // set the channel | ||
| channelIdSet = setChannelToTransaction(templateListParams.nodeId); | ||
| channelIdSet = setChannelToTransaction(templateListParams.folderNodeId); |
There was a problem hiding this comment.
It is not duplicated for this method here. It is duplicated for method TemplateResource.list(), but it is not used in that method, so that method should not use TemplateListParameterBean anyways.
So instead of changing the name of the query parameter, which is a breaking change (and confusing), better replace the TemplateListParameterBean in TemplateResource.list() by an apropriate alternative. Also this is a bugfix and should therefor be done for cms 6.3.x onwards.
| @Override | ||
| @GET | ||
| @Path("/{id}/perms/{type}") | ||
| @Path("/{id}/perms/{permType}") |
There was a problem hiding this comment.
Why is it necessary to rename this?
There was a problem hiding this comment.
Started with several clashing type parameters, as OpenAPI spec does not allow similarly named parameters, even if they belong to the separate URL parts (path vs query). I have put more clarified names in the path parameters, which value only the path, not the name itself.
| /** | ||
| * A class to test the generated client of OpenAPI specification | ||
| */ | ||
| public class OpenAPIClient implements RestClient { |
There was a problem hiding this comment.
What's the purpose of this class? It does exactly the same as the original RestClient (which was now renamed to JerseyRestClientImpl): It is a wrapper around an instance of JerseyClient and manages login, cookies and a little bit of failure handling.
None of our tests, which use the RestClient would use any of the methods generated for DefaultApi, but would use the methods of jakarta.ws.rs-api.
There was a problem hiding this comment.
Just for the purposes of testing the generated client as much as possible, which here is indeed a login and failures. Can be removed along with the Jenkins parameter, especially if introducing a breaking change.
There was a problem hiding this comment.
Should be removed, and the RestClient restored as class.
I am also questioning the generation of the DefaultApi, since it is not part of the CMS API and is not used anywhere.
| */ | ||
| @DELETE | ||
| @Path("/{key}") | ||
| @Hidden |
There was a problem hiding this comment.
Why is this necessary? The openapi spec is not built from the classes in this module, but from the interfaces from cms-restapi.
There was a problem hiding this comment.
This method makes an error in the spec, as the @PathParam("{path}") from above is applied to all the resource, making conflicts with the similarly defined methods below, that have {path: *} in the definition. This annotation hides the method from the spec.
There was a problem hiding this comment.
The question is, why is this class part of the OpenAPI spec? The OpenAPI of CMS OSS is generated in the module cms-restapi and does not use this class at all.
2 participants
Technically there is nothing changed to the features or stability of the CMS, so no changelog.