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

Multipart File Generation as InputStream #118

Closed
melloware opened this issue Aug 15, 2022 · 21 comments
Closed

Multipart File Generation as InputStream #118

melloware opened this issue Aug 15, 2022 · 21 comments
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@melloware
Copy link
Contributor

Current Multipart Generation is generating the call with a File object when an InputStream would be preferred.

Current:

@FormParam("file")
@org.jboss.resteasy.annotations.providers.multipart.PartFilename("fileFile")
@org.jboss.resteasy.annotations.providers.multipart.PartType(MediaType.APPLICATION_OCTET_STREAM)
public File _file;

When ideally for more flexible client rather than requiring a File object requiring an InputStream so it can be streamed from say S3 or Azure Blob store.

Wanted:

@FormParam("file")
@PartType(MediaType.APPLICATION_OCTET_STREAM)
public InputStream _file;

Full current example.

    @PUT
    @Consumes({"multipart/form-data"})
    @Produces({"application/json"})
    @GeneratedMethod ("v2DocumentPut")
    public DocumentMetadata v2DocumentPut(
        @org.jboss.resteasy.annotations.providers.multipart.MultipartForm V2DocumentPutMultipartForm multipartForm
    );

    public static class V2DocumentPutMultipartForm {
        @FormParam("file")
        @org.jboss.resteasy.annotations.providers.multipart.PartFilename("fileFile")
        @org.jboss.resteasy.annotations.providers.multipart.PartType(MediaType.APPLICATION_OCTET_STREAM)
        public File _file;
        @FormParam("storagePath")
        @org.jboss.resteasy.annotations.providers.multipart.PartType(MediaType.TEXT_PLAIN)
        public String storagePath;
        @FormParam("storageContainer")
        @org.jboss.resteasy.annotations.providers.multipart.PartType(MediaType.TEXT_PLAIN)
        public String storageContainer;
    }
@melloware melloware changed the title Multipart Generation as InputStream Multipart File Generation as InputStream Aug 15, 2022
@ricardozanini
Copy link
Member

@melloware, thanks for opening this issue.

Can you send a PR? This is a reasonably simple request. We can keep both options online with a codegen property. wdyt?

@ricardozanini ricardozanini added enhancement New feature or request good first issue Good for newcomers labels Aug 15, 2022
@melloware
Copy link
Contributor Author

Yep let me work on that. I may have a question on how to make it a switchable property. But let me submit a PR for you to review.

@melloware
Copy link
Contributor Author

OK i found multipartFormdataPojo.qute but I can't make heads nor tails of where p.dataType is coming from that its File.

    public static class {op.operationIdCamelCase}MultipartForm {
    {#for p in op.formParams}
        @FormParam("{p.baseName}")
    {#if p.isFile || (p.isString && p.dataFormat == 'base64')}
        @org.jboss.resteasy.annotations.providers.multipart.PartFilename("{p.baseName}File")
        @org.jboss.resteasy.annotations.providers.multipart.PartType(MediaType.APPLICATION_OCTET_STREAM)
    {#else if p.isPrimitiveType or (p.isArray && p.items.isPrimitiveType)}
        @org.jboss.resteasy.annotations.providers.multipart.PartType(MediaType.TEXT_PLAIN)
    {#else}
        @org.jboss.resteasy.annotations.providers.multipart.PartType(MediaType.APPLICATION_JSON)
    {/if}
        public {p.dataType} {p.paramName};
    {/for}
    }

@ricardozanini
Copy link
Member

@melloware, I'll check in the codebase. These variables usually come from the bundle mapped by the openapi-generator library. Unfortunately, they are poorly documented, so we debug the DefaultGenerator class to find out.

@Orbifoldt, since you introduce this feature, can you please take a look?

@Orbifoldt
Copy link
Contributor

@melloware Yeah its tricky to find it out, I have never seen any documentation on which properties you are supposed to have access to... When you debug the code generation done by the openapi-generator you can let it print all information on the operations of your openapi spec. Then for each operation you have access to the parameters used by that operation. To get the form parameters of some operation op you use op.formParams. This then contains the parameters that are part of the request body with type multipart/form-data.

As you can see the qute template consists of two parts: (1) an if expression where we set up the annotations, and (2) the declaration of the parameter.

For example, this yaml (taken from here)

requestBody:
  content: 
    multipart/form-data: # Media type
      schema:            # Request payload
        type: object
        properties:      # Request parts
          profileImage:  
            type: string
            format: binary

gives us an entry in the op.formParams array that looks like:

{
  "isFormParam": true,
  "isQueryParam": false,
  "isPathParam": false,
  "isHeaderParam": false,
  "isCookieParam": false,
  "isBodyParam": false,
  "isContainer": false,
  "isCollectionFormatMulti": false,
  "isPrimitiveType": true,
  "isModel": false,
  "isExplode": false,
  "isDeepObject": false,
  "isAllowEmptyValue": false,
  "baseName": "profileImage",
  "paramName": "profileImage",
  "dataType": "File",
  "dataFormat": "binary",
  "baseType": "File",
  "example": "new File(\"/path/to/file\")",
  "jsonSchema": "{\r\n  \"type\" : \"string\",\r\n  \"format\" : \"binary\"\r\n}",
  "isString": false,
  "isNumeric": false,
  "isInteger": false,
  "isLong": false,
  "isNumber": false,
  "isFloat": false,
  "isDouble": false,
  "isDecimal": false,
  "isByteArray": false,
  "isBinary": true,
  "isBoolean": false,
  "isDate": false,
  "isDateTime": false,
  "isUuid": false,
  "isUri": false,
  "isEmail": false,
  "isFreeFormObject": false,
  "isAnyType": false,
  "isShort": false,
  "isUnboundedInteger": false,
  "isArray": false,
  "isMap": false,
  "isFile": true,
  "isEnum": false,
  "additionalPropertiesIsAnyType": false,
  "hasVars": false,
  "vars": [],
  "requiredVars": [],
  "vendorExtensions": {},
  "hasValidation": false,
  "isNullable": false,
  "isDeprecated": false,
  "required": false,
  "exclusiveMaximum": false,
  "exclusiveMinimum": false,
  "uniqueItems": false,
  "isNull": false,
  "hasRequired": false,
  "hasDiscriminatorWithNonEmptyMapping": false,
  "hasMultipleTypes": false,
  "complexType": "File"
}

If you analyse this you see that for this parameter the property isFile has value true. (In above example we had format: binary. An alternative way to upload a file is using the format: base64, in that case you'll have "isFile": false, so that's the reason for the p.isString && p.dataFormat == 'base64' case in the if expression). So that explains (1).

Here you also see dataType is File, which is used in part (2).

Hopefully this at least helps in understanding what's going on :) . I'll come back to you for the question how we can then replace the dataType of the file

@Orbifoldt
Copy link
Contributor

Orbifoldt commented Aug 16, 2022

Ok, so I see a few options:

  1. We add the custom properties typeMappings and importMappings to this extension and forward their values to the corresponding properties part of that default code gen in the openapi generator. To achieve what we want we should set typeMappings='File=InputStream' and importMappings='File=java.io.InputStream'. (here File is the openapi type)
  2. Same as 1., but use fully qualified name in the typeMapping, so typeMappings='File=java.io.InputStream'. Then you don't need to use the importMappings
  3. Abstract option 1 away behind some custom property fileDataType whose value is either File or InputStream. Then we let our extension itself set the importmapping and typemapping when we instantiate the generator.

The nice thing about doing it as in option 1 (or 2) is that it can also be reused for other types, see for example this example: https://openapi-generator.tech/docs/usage/#type-mappings-and-import-mappings. However, it does require a bit more configuration for users.

@ricardozanini @melloware what do you think is the way to go?

@melloware
Copy link
Contributor Author

melloware commented Aug 16, 2022

I like either but the simpler the better so typeMappings='File=java.io.InputStream I am good with.

@ricardozanini
Copy link
Member

Thanks for the great summary, @Orbifoldt! I believe we can use the FQN, hence option 2.

@melloware
Copy link
Contributor Author

@Orbifoldt any update on this?

@Orbifoldt
Copy link
Contributor

I started working on this @melloware @ricardozanini , see #143

@melloware
Copy link
Contributor Author

Closed with #143

@melloware melloware reopened this Sep 26, 2022
@melloware
Copy link
Contributor Author

tested this today and it works perfectly!

@melloware
Copy link
Contributor Author

melloware commented Oct 20, 2022

Any chance we can get a release I would like to use this plugin not in SNAPSHOT form???

@ricardozanini
Copy link
Member

@melloware, we are releasing one after #158

@ricardozanini
Copy link
Member

@melloware https://github.com/quarkiverse/quarkus-openapi-generator/releases/tag/0.12.0

Please, next time, don't let this rest too much time; we don't have a strict release schedule. This means that we can release any time the community needs.

@melloware
Copy link
Contributor Author

Thank you !!!

@melloware
Copy link
Contributor Author

@Orbifoldt I am seeing these warnings now though is this expected? Everything is working properly.

I have

quarkus.openapi-generator.codegen.spec.documents_openapi_yml.type-mappings.File=InputStream
quarkus.openapi-generator.codegen.spec.documents_openapi_yml.import-mappings.File=java.io.InputStream
[WARNING] [io.quarkus.config] Unrecognized configuration key "quarkus.openapi-generator.codegen.spec.documents_openapi_yml.type-mappings.File" was provided; it will be ignored; verify that the dependency extension for this configuration is set or that you did not make a typo

[WARNING] [io.quarkus.config] Unrecognized configuration key "quarkus.openapi-generator.codegen.spec.documents_openapi_yml.import-mappings.File" was provided; it will be ignored; verify that the dependency extension for this configuration is set or that you did not make a typo

@Orbifoldt
Copy link
Contributor

Orbifoldt commented Oct 22, 2022

@melloware It seems to me a false-positive warning, I checked the unit/integration tests and there I see that warning there too (e.g. io.quarkiverse.openapi.generator.it.TypeAndImportMappingTest#canMapTypesAndImportToDifferentValues). I am still not 100% on how quarkus extensions should deal with config...

@ricardozanini Do you maybe know how to fix or suppress this warning? I did declare the config items in the SpecItemConfig class, and it doesn't complain about other items. Perhaps their type being Optional<Map<String, String>> is not correctly matched?

Edit: Oh just as I sent this comment I changed the type to just be Map<String, String>, then did a clean compile, whereafter the warning seems to be gone. Is that intended behaviour or a bug? Not sure on what the convention is for an optional map.

@ricardozanini
Copy link
Member

Do you mind sending a PR? If I would guess, I'd say that not using Optional for maps is the way to go since a map can be empty.

@hbelmiro
Copy link
Contributor

Yes, @ricardozanini is right. I've just tried here and Quarkus doesn't work with Optional<Map<String, String>>. If the config is absent, Quarkus converts it to an empty Map.

@Orbifoldt
Copy link
Contributor

I fixed it in #165

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

No branches or pull requests

4 participants