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

[REQ][Javascript] BOUNTY oneOf support in javascript client generator #13539

Closed
jrtcppv opened this issue Sep 27, 2022 · 21 comments · Fixed by #13561
Closed

[REQ][Javascript] BOUNTY oneOf support in javascript client generator #13539

jrtcppv opened this issue Sep 27, 2022 · 21 comments · Fixed by #13561

Comments

@jrtcppv
Copy link

jrtcppv commented Sep 27, 2022

Is your feature request related to a problem? Please describe.

As already discussed in issue #10514, anyOf, allOf, and oneOf schema features are unsupported in most client generators. Our team needs support for the oneOf feature in the Javascript generator.

Describe the solution you'd like

Client side input and output checks on models that include oneOf in their definitions.

Describe alternatives you've considered

Not using oneOf for validation.

Additional context

Following in the footsteps of #10812, offering $1000 (USD) and gratitude to anyone who resolves this.

@wing328
Copy link
Member

wing328 commented Sep 28, 2022

Thanks for sponsoring the fix. May I know if you have a spec handy to reproduce the issue and validate the fix?

(the spec can help clearly define the scope of the fix)

UPDATE: working on it.

@wing328
Copy link
Member

wing328 commented Sep 30, 2022

@jrtcppv I've filed #13561 to add oneOf support to the JS client. Can you please give it a try when you've time?

@jrtcppv
Copy link
Author

jrtcppv commented Sep 30, 2022

Hi, really appreciate the quick turnaround on this. I will take a closer look this weekend and make sure it works for us. If you want to look at our raw schema, you can access it at https://cloud.tator.io/schema. Sorry that schema is so huge, the model I would point you to is Color which is one of several other types, RgbColor, RgbaColor, or HexColor. If we could get the minItems, maxItems, minimum and maximum schema checks in as well that would be great, but I understand I did not include those in the original request, so no big deal if you can't get them in. Thank you!

@wing328
Copy link
Member

wing328 commented Oct 1, 2022

If we could get the minItems, maxItems, minimum and maximum schema checks in as well that would be great

I assume the oneOf schemas are models instead of primitive types. I'll add primitive type support over the weekend.

@jrtcppv
Copy link
Author

jrtcppv commented Oct 1, 2022

@wing328 The generated output in the javascript-es6 folder looks great, however we do enable the usePromises option and the javascript-promise-es6 output doesn't seem to contain the same level of schema checks. Is that option the only difference between the two? Just wondering if we could get the same type of output with that option enabled, I took a quick look at superagent callbacks but I think promises are preferred on our end as they are a browser standard.

Also, I tried building your branch from source and generating the client using our schema, and it doesn't look like either of those! I am invoking it as follows after building with mvn clean install:

./bin/utils/openapi-generator-cli.sh generate -c ~/config.json -i ~/schema.yaml -g javascript -o ~/js_out

And the contents of config.json are:

{
  "projectName": "tator",
  "projectVersion": "0.0.5",
  "projectDescription": "JavaScript client for Tator.",
  "licenseName": "MIT",
  "usePromises": true,
  "legacyDiscriminatorBehavior": false
}

Let me know if I am doing something wrong there, I don't see any schema check in Color.js except a mention in the comments of the oneOf options.

@wing328
Copy link
Member

wing328 commented Oct 2, 2022

javascript-promise-es6 output doesn't seem to contain the same level of schema checks

I've not yet updated the javascript-promise-es6 samples in my previous commit. Just pushed the updated samples via 61378ff

@wing328
Copy link
Member

wing328 commented Oct 2, 2022

Using the spec in https://cloud.tator.io/schema, I got NPE:

Exception in thread "main" java.lang.RuntimeException: Could not process model 'EncodeConfig'.Please make sure that your schema is correct!
	at org.openapitools.codegen.DefaultGenerator.generateModels(DefaultGenerator.java:518)
	at org.openapitools.codegen.DefaultGenerator.generate(DefaultGenerator.java:912)
	at org.openapitools.codegen.cmd.Generate.execute(Generate.java:465)
	at org.openapitools.codegen.cmd.OpenApiGeneratorCommand.run(OpenApiGeneratorCommand.java:32)
	at org.openapitools.codegen.OpenAPIGenerator.main(OpenAPIGenerator.java:66)
Caused by: java.lang.NullPointerException
	at org.openapitools.codegen.DefaultCodegen.fromModel(DefaultCodegen.java:2870)
	at org.openapitools.codegen.languages.JavascriptClientCodegen.fromModel(JavascriptClientCodegen.java:855)
	at org.openapitools.codegen.DefaultGenerator.processModels(DefaultGenerator.java:1291)
	at org.openapitools.codegen.DefaultGenerator.generateModels(DefaultGenerator.java:513)
	... 4 more

I'm investigating why.

Are you using that as well? Just want to make sure we're using the same spec.

UPDATE: NPE fixed via 215fbc5

@jrtcppv
Copy link
Author

jrtcppv commented Oct 2, 2022

Yes forgot to mention that I had to delete additionalProperties: true in both the EncodeConfig and ResolutionConfig models. I am not sure why these cause an issue. I believe they also cause an issue in the python generator.

@wing328
Copy link
Member

wing328 commented Oct 2, 2022

I've used your spec and pushed the auto-generated JS client to a repo. Here is what Color.js looks like: https://github.com/wing328/tator-js-client/blob/main/src/model/Color.js

Let's another minor issue that I just discovered and working on a fix now.

@wing328
Copy link
Member

wing328 commented Oct 2, 2022

Just pushed our another fix. Now npm install and test run without issue in https://github.com/wing328/tator-js-client/

    ✓ should have the property segmentEndFrames (base name: "segment_end_frames")
    ✓ should have the property file (base name: "file")

  VideoDefinition
    ✓ should create an instance of VideoDefinition
    ✓ should have the property path (base name: "path")
    ✓ should have the property size (base name: "size")
    ✓ should have the property bitRate (base name: "bit_rate")
    ✓ should have the property codec (base name: "codec")
    ✓ should have the property resolution (base name: "resolution")
    ✓ should have the property segmentInfo (base name: "segment_info")
    ✓ should have the property host (base name: "host")
    ✓ should have the property httpAuth (base name: "http_auth")
    ✓ should have the property codecMime (base name: "codec_mime")
    ✓ should have the property codecDescription (base name: "codec_description")


  1119 passing (8s)

@wing328
Copy link
Member

wing328 commented Oct 2, 2022

Heads up: I'm going to add a fromJSON(str) method to the oneOf model as well to make it easier for developers to create the oneOf model directly from JSON string.

@jrtcppv
Copy link
Author

jrtcppv commented Oct 2, 2022

Wow this looks great! The fromJSON method would actually be really helpful, I am wondering if a toJSON might also be useful to get to a normal JSON object? We worked around that in one case by using the WithHttpInfo methods and accessing response.body directly.

Is there a dev guide on how to run openapi-generator-cli after building? We expect to add some more oneOf models down the road but that might be a while

I will PM you for payment instructions. Thanks again, can't say how much we appreciate the quick turnaround and thorough work.

@wing328
Copy link
Member

wing328 commented Oct 2, 2022

I am wondering if a toJSON might also be useful to get to a normal JSON object?

We already override toJSON in the oneOf models to make it works seamlessly with the JSON.stringify(obj) method and I think that's how a "common" JS developer normally uses to turn an object into JSON string.

Let me brainstorm a bit more to see what we can do to make JS developer life easier when consuming the auto-generated JS SDK.

@wing328
Copy link
Member

wing328 commented Oct 2, 2022

Is there a dev guide on how to run openapi-generator-cli after building? We expect to add some more oneOf models down the road but that might be a while

We don't have an official dev guide at the moment. We usually show some examples on how to use the openapi-generator-cli via the JAVA JAR or the Docker image. If you need help on that, please PM me.

@wing328
Copy link
Member

wing328 commented Oct 2, 2022

UPDATE: I've merged #13561 into master. I'll work on fromJSON (or maybe toJSON) in a separate PR instead.

@wing328
Copy link
Member

wing328 commented Oct 3, 2022

UPDATE: I've filed 2 more follow-up PRs

For adding a method to convert an object into JSON string, I've looked into some popular JS libraries and none of them offer such method. My guess is that JS developers get used to the JSON.stringify method.

UPDATE: both PRs have been merged.

@jrtcppv
Copy link
Author

jrtcppv commented Dec 29, 2022

@wing328 I tried using this in our application and there appears to be a problem handling certain combinations of string and other types. One of the components in our schema AttributeValue includes strings, booleans, numbers, or numerical arrays, and the generated output looks like the following:

/**
 * Tator REST API
 * Interface to the Tator backend.
 *
 * The version of the OpenAPI document: v1
 * 
 *
 * NOTE: This class is auto generated by OpenAPI Generator (https://openapi-generator.tech).
 * https://openapi-generator.tech
 * Do not edit the class manually.
 *
 */

import ApiClient from '../ApiClient';

/**
 * The AttributeValue model module.
 * @module model/AttributeValue
 * @version 0.0.5
 */
class AttributeValue {
    /**
     * Constructs a new <code>AttributeValue</code>.
     * Boolean, integer, float, string, datetime, [lon, lat], float array.
     * @alias module:model/AttributeValue
     * @param {(module:model/Boolean|module:model/Number|module:model/String|module:model/[Number])} instance The actual instance to initialize AttributeValue.
     */
    constructor(instance = null) {
        if (instance === null) {
            this.actualInstance = null;
            return;
        }
        var match = 0;
        var errorMessages = [];
        try {
            this.actualInstance = instance;
            match++;
        } catch(err) {
            // json data failed to deserialize into Boolean
            errorMessages.push("Failed to construct Boolean: " + err)
        }

        try {
            // validate array of string
            if (!(typeof instance === 'number' && instance % 1 != 0)) {
                throw new Error("Invalid array items. Must be number. Input: " + JSON.stringify(instance));
            }
            this.actualInstance = instance;
            match++;
        } catch(err) {
            // json data failed to deserialize into Number
            errorMessages.push("Failed to construct Number: " + err)
        }

        try {
            // validate array of string
            if (!(typeof instance === 'string')) {
                throw new Error("Invalid input. Must be string. Input: " + JSON.stringify(instance));
            }
            this.actualInstance = instance;
            match++;
        } catch(err) {
            // json data failed to deserialize into String
            errorMessages.push("Failed to construct String: " + err)
        }

        try {
            // validate array data type
            if (!Array.isArray(instance)) {
                throw new Error("Invalid data type. Expecting array. Input: " + instance);
            }
            if (instance.length < 1) {
                throw new Error("Invalid array size. Minimim: 1. Input: " + instance);
            }
            // validate array of string
            for (const item of instance) {
                if (!(typeof instance === 'number' && instance % 1 != 0)) {
                    throw new Error("Invalid array items. Must be number. Input: " + JSON.stringify(instance));
                }
            }
            this.actualInstance = instance;
            match++;
        } catch(err) {
            // json data failed to deserialize into [Number]
            errorMessages.push("Failed to construct [Number]: " + err)
        }

        if (match > 1) {
            throw new Error("Multiple matches found constructing `AttributeValue` with oneOf schemas Boolean, Number, String, [Number]. Input: " + JSON.stringify(instance));
        } else if (match === 0) {
            this.actualInstance = null; // clear the actual instance in case there are multiple matches
            throw new Error("No match found constructing `AttributeValue` with oneOf schemas Boolean, Number, String, [Number]. Details: " +
                            errorMessages.join(", "));
        } else { // only 1 match
            // the input is valid
        }
    }

    /**
     * Constructs a <code>AttributeValue</code> from a plain JavaScript object, optionally creating a new instance.
     * Copies all relevant properties from <code>data</code> to <code>obj</code> if supplied or a new instance if not.
     * @param {Object} data The plain JavaScript object bearing properties of interest.
     * @param {module:model/AttributeValue} obj Optional instance to populate.
     * @return {module:model/AttributeValue} The populated <code>AttributeValue</code> instance.
     */
    static constructFromObject(data, obj) {
        return new AttributeValue(data);
    }

    /**
     * Gets the actaul instance, which can be <code>Boolean</code>, <code>Number</code>, <code>String</code>, <code>[Number]</code>.
     * @return {(module:model/Boolean|module:model/Number|module:model/String|module:model/[Number])} The actual instance.
     */
    getActualInstance() {
        return this.actualInstance;
    }

    /**
     * Sets the actaul instance, which can be <code>Boolean</code>, <code>Number</code>, <code>String</code>, <code>[Number]</code>.
     * @param {(module:model/Boolean|module:model/Number|module:model/String|module:model/[Number])} obj The actual instance.
     */
    setActualInstance(obj) {
       this.actualInstance = AttributeValue.constructFromObject(obj).getActualInstance();
    }

    /**
     * Returns the JSON representation of the actual intance.
     * @return {string}
     */
    toJSON = function(){
        return this.getActualInstance();
    }

    /**
     * Create an instance of AttributeValue from a JSON string.
     * @param {string} json_string JSON string.
     * @return {module:model/AttributeValue} An instance of AttributeValue.
     */
    static fromJSON = function(json_string){
        return AttributeValue.constructFromObject(JSON.parse(json_string));
    }
}


AttributeValue.OneOf = ["Boolean", "Number", "String", "[Number]"];

export default AttributeValue;

However when I attempt to create an AttributeValue instance using a string I get an error. It seems to work okay for the other types in the console:

image

@wing328
Copy link
Member

wing328 commented Jan 5, 2023

@jrtcppv I've filed #14380 to fix the issue.

Happy New Year!

@jrtcppv
Copy link
Author

jrtcppv commented Jan 5, 2023

Excellent, thank you! I will try again in the next release.

@wing328
Copy link
Member

wing328 commented Jan 6, 2023

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants