160 scs multiapi plugin homogenize code generation in openapi and asyncapi #200

eduardofrancosngular · 2023-04-11T15:23:27Z

Code refactoring to use JsonNode instead of swagger apis

github-actions

Thank you for collaborating with the project to help us improve!

github-actions · 2023-04-11T15:26:51Z

This pull request hasn't been labeled as release nor documentation. Please ensure this is intentional before merging.

josemenvegsngular · 2023-04-11T15:40:29Z

multiapi-engine/src/main/java/com/sngular/api/generator/plugin/commonApi/Constants.java

+
+public final class Constants {
+
+  public static final String INTEGER = "integer";


A doubt: won't it be better to group the constant values depending on their objective?
I'm thinking of an enum group with all the types (integer, double, float, ... as we get in UNION) instead of getting a big group with a constant mix.

Anyway, I think that, at least in this file, there is a style issue in line 15 (double space between static and final)

AdrianLagartera

Check the way you put some type of variable to following one way, of course if you want use var and String its a good idea for example if you use the var in a lambda or for a generic object, but if two methods have a very similar code, try to replicate the same way as I mention in previous comments

And please fix the tests, I think you change a lot of classes and only I see one test change, be sure about you test properly the new changes

AdrianLagartera · 2023-04-11T16:11:12Z

multiapi-engine/pom.xml

+    <dependency>
+      <groupId>org.springframework</groupId>
+      <artifactId>spring-web</artifactId>
+      <version>5.3.19</version>
+    </dependency>


I think add a new dependency just to use the RequestMethod.values method which only return a list of enums with the http operations not the best solution, I could suggest to you create your own class enum for this purpose.

And by the way be careful when you add a new dependency, because in this case this version of spring-web have a known vulnerability

AdrianLagartera · 2023-04-11T16:41:11Z

multiapi-engine/src/main/java/com/sngular/api/generator/plugin/commonApi/MapperUtilCommon.java

+    var typeMap = "";
+    if (mapSchema.get(ADDITIONAL_PROPERTIES)==null) return null;
+    final var mapNode = mapSchema.get(ADDITIONAL_PROPERTIES);
+    final var mapValueType = mapNode.findPath(TYPE);
+    typeMap = getCollectionType(mapNode, mapValueType, prefix, suffix);
+    return typeMap;
+  }
+
+  public static String getCollectionType(final JsonNode mapNode, final JsonNode mapValueType, final String prefix, final String suffix) {
+    String typeMap = null;
+    if ((checkTextValueNotNull(mapValueType))&&(!mapValueType.textValue().contains("#"))) {


Why at the beginning of this method use var, and in the next one use String, checking other parts of the code you did the same, I think you should take a way to choose var or String to avoid problems with readability, because looks like each one with a different type of variable could be uses for different purpose

davidgayoso · 2023-04-12T07:02:18Z

multiapi-engine/src/main/java/com/sngular/api/generator/plugin/openapi/OpenApiGenerator.java

-import com.sngular.api.generator.plugin.openapi.model.GlobalObject;
-import com.sngular.api.generator.plugin.openapi.model.PathObject;
-import com.sngular.api.generator.plugin.openapi.model.SchemaObject;
+import com.sngular.api.generator.plugin.openapi.model.*;


Please remove this * and import only those classes that you really need.

davidgayoso · 2023-04-12T07:05:21Z

...iapi-engine/src/main/java/com/sngular/api/generator/plugin/openapi/utils/MapperAuthUtil.java


+import java.util.*;


Here you have another * on a import, so I tell you the same as my previous comment.

davidgayoso · 2023-04-12T07:07:47Z

...i-engine/src/main/java/com/sngular/api/generator/plugin/openapi/utils/MapperContentUtil.java

-      final String itemType = Objects.nonNull(arraySchema.getItems().get$ref()) ? getRef(arraySchema.getItems(), specFile) : arraySchema.getItems().getType();
+    } else if ((ARRAY.equalsIgnoreCase(getTextValue(schema,TYPE))) &&(checkNotEmpty(schema,ITEMS))){
+      final String itemType = Objects.nonNull(schema.get(ITEMS).get(REF)) ? MapperUtilCommon.getRef(schema.get(ITEMS), specFile.getModelNamePrefix(),
+                                                                                                    specFile.getModelNameSuffix()) : getTextValue(schema.get(ITEMS),TYPE);


I think here you have some issues related with the style, so I suggest you to run the autoformatter.

janDuinRod · 2023-04-12T08:27:18Z

multiapi-engine/src/main/java/com/sngular/api/generator/plugin/commonApi/MapperUtilCommon.java

+    return null;
+  }
+  public static boolean checkTextValueNotNull(JsonNode schema){
+    return (schema != null && schema.textValue()!= null && !schema.isNull());
+  }
+


Make sure you are using the OSS style rules

janDuinRod · 2023-04-12T08:40:39Z

multiapi-engine/src/main/java/com/sngular/api/generator/plugin/openapi/OpenApiGenerator.java

 import org.apache.commons.io.FilenameUtils;
 import org.apache.commons.lang3.StringUtils;
+import static com.sngular.api.generator.plugin.commonApi.Constants.*;


Avoid * imports

janDuinRod · 2023-04-12T08:48:13Z

multiapi-engine/src/main/java/com/sngular/api/generator/plugin/openapi/OpenApiGenerator.java


 public class OpenApiGenerator {

+  public static final String AUTHENTICATION = "Authentication";


Are you sure this shouldn't be private?

janDuinRod · 2023-04-12T08:49:03Z

multiapi-engine/src/main/java/com/sngular/api/generator/plugin/openapi/OpenApiGenerator.java

+  public static final String SERVERS = "servers";
+
+  public static final String SECURITY_SCHEMES = "securitySchemes";
+
+  public static final String SECURITY = "security";


As I said on my last comment shouldn't this be private as well?

janDuinRod · 2023-04-12T09:06:02Z

multiapi-engine/src/main/java/com/sngular/api/generator/plugin/openapi/OpenApiGenerator.java

+      File dir = path.toFile();
+      boolean created = dir.mkdirs();
+      if (!created && !dir.isDirectory()) {
+        throw new RuntimeException("Failed to create directory: " + dir);
+      }
      path = path.resolve(convertPackageToTargetPath(fileSpecPackage, isModel));
    }
-    if (!path.toFile().isDirectory()) {
-      path.toFile().mkdirs();
+    File dir = path.toFile();
+    if (!dir.isDirectory()) {
+      boolean created = dir.mkdirs();
+      if (!created) {
+        throw new RuntimeException("Failed to create directory: " + dir);


Directories are created from a path in 2 different ways in the same function, also, the code is repeated multiple times, wouldn't it be better to extract it to a function?

janDuinRod

Please run the autoformater and checkstyle with the already existing conifgurations (styles folder), apart from fixing all the aforementioned issues.

5uso-sng

Please make sure to run the autoformatter, as well as checkstyle. There's a lot of indentation, spacing, and newline inconsistencies. Other style differences, such as the use of returns or final variables are pointed out by our checkstyle configuration.

Also, note that the integration tests aren't passing

5uso-sng · 2023-04-12T08:25:22Z

OSS_style_idea.xml

This file seems to be a duplicate, another copy already existed in the styles directory

5uso-sng · 2023-04-12T08:32:55Z

multiapi-engine/src/main/java/com/sngular/api/generator/plugin/commonApi/MapperUtilCommon.java

+  public static String getTextValue(JsonNode node, String field){
+    try{
+      if(checkTextValueNotNull(node.get(field)))
+        return node.get(field).textValue();
+    }
+    catch (StackOverflowError e){
+      return null;
+    }
+    return null;
+  }


Since this is a public function, it would probably be a good idea to annotate it as nullable

I was thinking... what would change if we do something like:

catch (StackOverflowError e){} finally { return null; } }

To avoid duplicating return null;

5uso-sng · 2023-04-12T08:36:38Z

multiapi-engine/src/main/java/com/sngular/api/generator/plugin/commonApi/MapperUtilCommon.java

+    return typeArray;
+  }
+
+  public static boolean checkNotEmpty (JsonNode node, String ... fields){


The parameters of this function should be final

5uso-sng · 2023-04-12T08:37:18Z

multiapi-engine/src/main/java/com/sngular/api/generator/plugin/commonApi/MapperUtilCommon.java

+  }
+
+  public static boolean checkNotEmpty (JsonNode node, String ... fields){
+      for (String field : fields.clone()) {


Why are we cloning a varargs of an immutable type to iterate over it?

5uso-sng · 2023-04-12T08:41:54Z

multiapi-engine/src/main/java/com/sngular/api/generator/plugin/commonApi/MapperUtilCommon.java

+
+  public static boolean checkNotEmpty (JsonNode node, String ... fields){
+      for (String field : fields.clone()) {
+        if (node == null || node.get(field) == null || node.get(field).toString().isEmpty() || (NULL.equalsIgnoreCase(node.get(field).toString())))


The value of node doesn't change during the loop, so it would be better extracting the first check from the loop

5uso-sng · 2023-04-12T08:42:24Z

multiapi-engine/src/main/java/com/sngular/api/generator/plugin/commonApi/MapperUtilCommon.java

+  public static boolean checkNotEmpty (JsonNode node, String ... fields){
+      for (String field : fields.clone()) {
+        if (node == null || node.get(field) == null || node.get(field).toString().isEmpty() || (NULL.equalsIgnoreCase(node.get(field).toString())))
+          return false;


We are discouraged from using return statements in the middle of functions

5uso-sng · 2023-04-12T08:49:18Z

multiapi-engine/src/main/java/com/sngular/api/generator/plugin/commonApi/MapperUtilCommon.java

+    } catch (IOException e) {
+      e.printStackTrace();
+    }


Is it safe to continue execution after this fails?

5uso-sng · 2023-04-12T08:51:59Z

multiapi-engine/src/main/java/com/sngular/api/generator/plugin/openapi/OpenApiGenerator.java

  private void processFile(final SpecFile specFile, final String filePathToSave) throws TemplateException, IOException {
+    try {


This try block has a way bigger scope than it should, especially since it's ignoring the exception. It's also strange to catch IOExceptions in a function that does throw IOException

5uso-sng · 2023-04-12T09:05:32Z

...iapi-engine/src/main/java/com/sngular/api/generator/plugin/openapi/utils/MapperAuthUtil.java

+      return API_KEY_AUTH;
+    } else if (type.equalsIgnoreCase(OAUTH_2)) {
+      return StringUtils.capitalize(OAUTH);
+    } else if (type.equalsIgnoreCase(HTTP)) {
+      return HTTP_BASIC_AUTH;


In these cases we typically store the value to return in a final variable that's uninitialized at the start of the function and set where these returns are, with a single return at the end of the function

5uso-sng · 2023-04-12T09:23:01Z

...iapi-engine/src/main/java/com/sngular/api/generator/plugin/openapi/utils/MapperPathUtil.java

-    }
-    if (Boolean.TRUE.equals(checkIfOperationIsNull(path.getValue().getPatch()))) {
-      operationObjects.add(createOperation(openAPI, path.getValue().getPatch(), "PATCH", specFile, globalObject, operationIdList));
+    for ( RequestMethod method: RequestMethod.values()) {


This enum from Spring doesn't seem necessary. It would be preferrable to use our own, as to reduce the amount of dependencies

5uso-sng · 2023-04-12T10:06:10Z

Oh, just noticed the version numbers will need to be updated. Make sure to do it not just in the engine pom, but in the maven and gradle plugins as well

josemenvegsngular · 2023-05-03T11:10:49Z

multiapi-engine/src/main/java/com/sngular/api/generator/plugin/commonApi/MapperUtilCommon.java

+    } else {
+      mapValueType = arrayNode.get(REF);
+    }
+    typeArray = getCollectionType(arrayNode, mapValueType, prefix, suffix);


I'm looking this code and L.88 and I think that the condition can be moved inside getCollectionType method:

in both, the second parameter is obtained from the first parameter (in L.88 send always the TYPE but in L.116, it check if the TYPE is present?)

the issue is: does this method called from other parts of the code?

even inside the own method it checks if the second parameter contains # (as it would be a reference, instead of a type)

josemenvegsngular · 2023-05-11T14:09:10Z

multiapi-engine/src/main/java/com/sngular/api/generator/plugin/asyncapi/AsyncApiGenerator.java


-          processOperation(fileParameter, ymlParentPath, entry, channel, operationId, channelPayload, totalSchemas);
-        }
+        handleMissingPublisherConsumer(fileParameter, channel, operationId);


May be there is a reason, but just with this code I have a doubt:
If operationId = getOperationId(channel) and channelPayload = getChannelPayload(channel) why in handleMissingPublisherConsumer and processOperation pass operationId and channelPayload as arguments when we can obtain their values through channel (which is also an argument)?

eduardofrancosngular linked an issue Apr 11, 2023 that may be closed by this pull request

[SCS-MultiApi-Plugin]: Homogenize code generation in openApi and AsyncApi #160

Open

github-actions bot reviewed Apr 11, 2023

View reviewed changes

jemacineiras assigned jemacineiras, AdrianLagartera, nuriaricofr, josemenvegsngular, 5uso-sng, davidgayoso and janDuinRod Apr 11, 2023

jemacineiras added the enhancement New feature or request label Apr 11, 2023

josemenvegsngular reviewed Apr 11, 2023

View reviewed changes

AdrianLagartera reviewed Apr 11, 2023

View reviewed changes

davidgayoso reviewed Apr 12, 2023

View reviewed changes

janDuinRod requested review from 5uso-sng and janDuinRod April 12, 2023 08:20

5uso-sng requested review from janDuinRod and removed request for janDuinRod April 12, 2023 08:20

janDuinRod reviewed Apr 12, 2023

View reviewed changes

5uso-sng suggested changes Apr 12, 2023

View reviewed changes

jemacineiras unassigned jemacineiras and AdrianLagartera Apr 24, 2023

jemacineiras assigned jemacineiras and unassigned nuriaricofr, josemenvegsngular, 5uso-sng, davidgayoso and janDuinRod Apr 24, 2023

jemacineiras marked this pull request as draft April 24, 2023 08:16

josemenvegsngular reviewed May 3, 2023

View reviewed changes

josemenvegsngular reviewed May 11, 2023

View reviewed changes

jemacineiras closed this May 18, 2023

jemacineiras force-pushed the 160-scs-multiapi-plugin-homogenize-code-generation-in-openapi-and-asyncapi branch from e10f393 to 04ef68c Compare May 18, 2023 15:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

160 scs multiapi plugin homogenize code generation in openapi and asyncapi #200

160 scs multiapi plugin homogenize code generation in openapi and asyncapi #200

eduardofrancosngular commented Apr 11, 2023

github-actions bot left a comment

github-actions bot commented Apr 11, 2023

josemenvegsngular Apr 11, 2023 •

edited

Loading

AdrianLagartera left a comment •

edited

Loading

AdrianLagartera Apr 11, 2023

AdrianLagartera Apr 11, 2023

davidgayoso Apr 12, 2023

davidgayoso Apr 12, 2023

davidgayoso Apr 12, 2023

janDuinRod Apr 12, 2023

janDuinRod Apr 12, 2023

janDuinRod Apr 12, 2023

janDuinRod Apr 12, 2023

janDuinRod Apr 12, 2023

janDuinRod left a comment

5uso-sng left a comment •

edited

Loading

5uso-sng Apr 12, 2023

5uso-sng Apr 12, 2023

josemenvegsngular May 12, 2023

5uso-sng Apr 12, 2023

5uso-sng Apr 12, 2023

5uso-sng Apr 12, 2023

5uso-sng Apr 12, 2023

5uso-sng Apr 12, 2023

5uso-sng Apr 12, 2023

5uso-sng Apr 12, 2023

5uso-sng Apr 12, 2023

5uso-sng commented Apr 12, 2023

josemenvegsngular May 3, 2023

josemenvegsngular May 11, 2023


		public final class Constants {

		public static final String INTEGER = "integer";


		public class OpenApiGenerator {

		public static final String AUTHENTICATION = "Authentication";

		private void processFile(final SpecFile specFile, final String filePathToSave) throws TemplateException, IOException {
		try {

160 scs multiapi plugin homogenize code generation in openapi and asyncapi #200

160 scs multiapi plugin homogenize code generation in openapi and asyncapi #200

Conversation

eduardofrancosngular commented Apr 11, 2023

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 11, 2023

josemenvegsngular Apr 11, 2023 • edited Loading

Choose a reason for hiding this comment

AdrianLagartera left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janDuinRod left a comment

Choose a reason for hiding this comment

5uso-sng left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

5uso-sng commented Apr 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josemenvegsngular Apr 11, 2023 •

edited

Loading

AdrianLagartera left a comment •

edited

Loading

5uso-sng left a comment •

edited

Loading