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

[cpp-restsdk] Support multi-line descriptions #753

Merged
merged 3 commits into from
Aug 9, 2018

Conversation

Peaches491
Copy link
Contributor

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: master, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

Using the YAML multi-line string notation will cause cpp-rest-sdk codegen to fail, with the newlines making it into the generated code:

  /job/:
    post:
      summary: "Adds a Job object to the Foo tool."
      operationId: "addJob"
      consumes:
      - "application/json"
      parameters:
      - in: "body"
        name: "fullJobDetails"
        description: |
          "JSON that contains all information needed to create a job for
           FooAPI."
        required: true
        schema:
          $ref: "#/definitions/FullJobDetails"

Yields:

    /// <summary>
    /// \&quot;Updates an ImageAttachment\&quot;·
    /// </summary>
    /// <remarks>
    ///·
    /// </remarks>
    /// <param name="fullJobDetails">\&quot;JSON that contains all information needed to create a job for
           FooAPI.\&quot; </param>
    pplx::task<std::shared_ptr<ImageFrameAttachment>> updateImageWithId(
        utility::string_t imageId,
        std::shared_ptr<ImageFrameAttachment> imageFrameAttachment
    ) override;

The uncommented FooAPI.\&quot; </param>, causes a compilation error.

This PR adds escapeText to all references to the numerous description references across DefaultCodegen.java

@jimschubert
Copy link
Member

jimschubert commented Aug 7, 2018

DefaultCodegen isn't the proper place for this escape. This file handles other types of generators besides code (for which this is relevant). If, for example, I wanted to convert an OpenAPI 2.0 spec to OpenAPI 3.0.1, I'd run:

$ openapi-generator generate -g openapi-yaml -i .bak/in/753.yaml -o .bak/out/753

This will currently dump 1:1 text in the new spec format:

$ grep -A 4 -B 10 'PR 753' .bak/in/753.yaml
      operationId: createUsersWithArrayInput
      produces:
        - application/xml
        - application/json
      parameters:
        - in: body
          name: body
          description: |
              "List of user object.

              Showing PR 753 example"
          required: true
          schema:
            type: array
            items:

$ grep -A 4 -B 10 'PR 753' .bak/out/753/openapi/openapi.yaml
  /user/createWithArray:
    post:
      tags:
      - user
      summary: Creates list of users with given input array
      operationId: createUsersWithArrayInput
      requestBody:
        description: |
          "List of user object.

          Showing PR 753 example"
        content:
          '*/*':
            schema:
              type: array

Other generators could potentially be broken by a change in DefaultCodegen as well, although I can't think of any others off the top of my head. Even code generators may output different templates where such an escape may not be ideal. For instance, if a generator outputs openapi.yaml for version 3.0.1 (as above) alongside code. Or if a non-HTML based README is generated, if descriptions are expected to be unencoded and HTML doc outputs re-encode (the change here might result in a double-encoded entity).

A better place for escaping textual data for templates is in the template itself.

I think a lambda would be best suited for this, especially because the issue doesn't necessarily seem to be with HTML escaping, but that the template isn't wrapping the quote characters with the new line. That is, calling escapeText might be sufficient for descriptions up to 5 or 10 lines, but fails for much longer descriptions.

I've created numerous mustache lambdas. You could follow the pattern of the AbstractCSharpCodegen to implement something similarly. Example:

    private void addMustacheLambdas(Map<String, Object> objs) {

        Map<String, Mustache.Lambda> lambdas = new ImmutableMap.Builder<String, Mustache.Lambda>()
                .put("multiline_comment_4", new IndentedLambda(4, " ", "///"))
                .build();

        if (objs.containsKey("lambda")) {
            LOGGER.warn("A property named 'lambda' already exists. Mustache lambdas renamed from 'lambda' to '_lambda'.");
            objs.put("_lambda", lambdas);
        } else {
            objs.put("lambda", lambdas);
        }
    }

And in processOpts of your generator:

addMustacheLambdas(additionalProperties);

This would require a modification to IndentedLambda:

diff --git a/modules/openapi-generator/src/main/java/org/openapitools/codegen/mustache/IndentedLambda.java b/modules/openapi-generator/src/main/java/org/openapitools/codegen/mustache/IndentedLambda.java
index d1b0b3826..3db3e10f9 100644
--- a/modules/openapi-generator/src/main/java/org/openapitools/codegen/mustache/IndentedLambda.java
+++ b/modules/openapi-generator/src/main/java/org/openapitools/codegen/mustache/IndentedLambda.java
@@ -43,13 +43,14 @@ import java.io.Writer;
  */
 public class IndentedLambda implements Mustache.Lambda {
     private final int prefixSpaceCount;
+    private final String prefix;
     private int spaceCode;

     /**
      * Constructs a new instance of {@link IndentedLambda}, with an indent count of 4 spaces
      */
     public IndentedLambda() {
-        this(4, " ");
+        this(4, " ", null);
     }

     /**
@@ -57,17 +58,20 @@ public class IndentedLambda implements Mustache.Lambda {
      *
      * @param prefixSpaceCount   The number of indented characters to apply as a prefix to a fragment.
      * @param indentionCharacter String representation of the character used in the indent (e.g. " ", "\t", ".").
+     * @param prefix             An optional prefix to prepend before the line (useful for multi-line comments.
      */
-    public IndentedLambda(int prefixSpaceCount, String indentionCharacter) {
-        this(prefixSpaceCount, Character.codePointAt(indentionCharacter, 0));
+    public IndentedLambda(int prefixSpaceCount, String indentionCharacter, String prefix) {
+        this(prefixSpaceCount, Character.codePointAt(indentionCharacter, 0), prefix);
     }

     /**
      * Constructs a new instance of {@link IndentedLambda}
      *
      * @param prefixSpaceCount The number of indented characters to apply as a prefix to a fragment.
+     * @param indentionCodePoint Code point of the single character used for indentation.
+     * @param prefix             An optional prefix to prepend before the line (useful for multi-line comments.
      */
-    private IndentedLambda(int prefixSpaceCount, int indentionCodePoint) {
+    private IndentedLambda(int prefixSpaceCount, int indentionCodePoint, String prefix) {
         if (prefixSpaceCount <= 0) {
             throw new IllegalArgumentException("prefixSpaceCount must be greater than 0");
         }
@@ -78,6 +82,7 @@ public class IndentedLambda implements Mustache.Lambda {

         this.prefixSpaceCount = prefixSpaceCount;
         this.spaceCode = indentionCodePoint;
+        this.prefix = prefix;
     }

     @Override
@@ -96,6 +101,7 @@ public class IndentedLambda implements Mustache.Lambda {
             // So, we want to skip the first line.
             if (i > 0) {
                 sb.append(prefixedIndention);
+                if (prefix != null) sb.append(prefix);
             }

             sb.append(line);

But could then be used like so:

    {{#allParams}}
    /// <param name="{{paramName}}">{{#lambda.multiline_comment_4}}{{description}}{{/lambda.multiline_comment_4}}{{^required}} (optional{{#defaultValue}}, default to {{.}}{{/defaultValue}}){{/required}}</param>
    {{/allParams}}

As we support new templating engines, Mustache lambdas obviously won't be reusable. It might make sense to extract multi-line indentation and multi-line description quoting logic to an infrastructural service that can be consumed by the Mustache lambda or any other template engine extensions we might have in the future.

@Peaches491
Copy link
Contributor Author

@jimschubert Thank you for the thorough explanation! Implemented the changes as you've suggested worked flawlessly. 👍

@Peaches491 Peaches491 changed the title escapeText on all references to description [cpp-restsdk] Support multi-line descriptions Aug 8, 2018
@jimschubert
Copy link
Member

@Danielku15 any concerns with this approach? If not, I think we can merge.

@Danielku15
Copy link
Contributor

@jimschubert No concerns, change look good to me.

@jimschubert jimschubert merged commit 987fd77 into OpenAPITools:master Aug 9, 2018
jimschubert added a commit to jimschubert/openapi-generator that referenced this pull request Aug 14, 2018
* master: (32 commits)
  Fixed date formatting in typescript node client (OpenAPITools#786)
  better explain usage (OpenAPITools#794)
  Fix float/double default value in C# generator (OpenAPITools#791)
  Enhancements to documentation generators (samples, default values, etc) (OpenAPITools#790)
  Remove duplicate variable declaration (OpenAPITools#792)
  Issue 758 root resource (OpenAPITools#771)
  Do not declare destructor as default when destructor is explicitly declared. (OpenAPITools#732)
  Fix C# client enum issue (OpenAPITools#774)
  [JavaScript] Update vulnerable dependencies (OpenAPITools#784)
  [Ruby] Fix method split (OpenAPITools#780)
  [Java][jaxrs-jersey] add sample with jaxrs-jersey + openapi v3 (OpenAPITools#778)
  update groupId in pom (OpenAPITools#779)
  [cpp-restsdk] Support multi-line descriptions (OpenAPITools#753)
  [Core] Resolve Inline Models (OpenAPITools#736)
  [gradle] Support nullable system property values (OpenAPITools#764)
  Correct URL for openapi-generator.cli.sh in README.md (OpenAPITools#770)
  Fixed the generation of model properties whose data type is a composed (allOf) schema (OpenAPITools#704)
  [JAX-RS][Spec] Add samples to CircleCI (OpenAPITools#759)
  minor update to python generator usage (OpenAPITools#762)
  [C++][Restbed/Pistache] Added fix for byte array (OpenAPITools#752)
  ...
@wing328 wing328 added this to the 3.2.1 milestone Aug 14, 2018
A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this pull request Feb 27, 2019
* Update IndentedLambda to take optional prefix

* Add `multiline_comment_4` to CppRestSdkClient

* Update cpp-restsdk example
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.

None yet

4 participants