IBM / keyprotect-nodejs-client

Nodejs SDK for interacting with the IBMCloud KeyProtect service.
Apache License 2.0
0 stars 10 forks source link

Fields `id`, `name`, `type` in `KeyWithPayload` or `KeyRepresentation` should not be optional #12

Open CarstenLeue opened 3 years ago

CarstenLeue commented 3 years ago

Fields id, name, type in KeyWithPayload or KeyRepresentation are declared optional:

        /** Specifies the MIME type that represents the key resource. Currently, only the default is supported. */
        type?: string;
        /** The v4 UUID used to uniquely identify the resource, as specified by RFC 4122. */
        id?: string;
        /** A human-readable name to assign to your key.
         *
         *  To protect your privacy, do not use personal data, such as your name or location as the name for your key.
         */
        name?: string;

But why would these fields ever be null for a newly created key or for a listing of a key. It is my understanding of the data model that these fields are an intrinsic part of every key and that they would always be defined.

Defining them as optional makes the client code more complex, because either a developer has to override the type hint or would have to write conditional code to handle the null situation.

padamstx commented 2 years ago

The properties mentioned above are defined as optional in the API definition, therefore the SDK generator includes the "?" marker after the field names. Logically, those properties could or should be defined as "required". However, if that change was made to the API definition, then that would constitute a breaking change to the API and the re-generated SDK code would potentially cause a breaking change for SDK users. Note that changing an optional property to required is one of a number of different types of API changes that can and likely will be a breaking change. So, probably to leave it as is at this point

CarstenLeue commented 2 years ago

Note that changing an optional property to required is one of a number of different types of API changes that can and likely will be a breaking change.

While I agree with this statement for contravariant input properties, I wonder if this is really true for the covariant return values. Note that the properties I am referring to are response properties. What kind of incompatibility would you expect?

CarstenLeue commented 2 years ago

The properties mentioned above are defined as optional in the API definition, therefore the SDK generator includes the "?" marker after the field names.

From my perspective I am a user of the SDK and I do not really care how the SDK has been generated, e.g. if a shortcoming exposed in the SDK is a consequence of one of its transitive dependencies. I think that the SDK should present a model that describes the state of a resource as conveniently as possible to the user of the SDK. In this case it's very inconvenient to mark an identifier field of a newly created resource as optional.

May I suggest you use this issue to create a follow up issue against the API definition of the service, so this will be fixed "eventually"?

padamstx commented 2 years ago

While I agree with this statement for contravariant input properties, I wonder if this is really true for the covariant return values. Note that the properties I am referring to are response properties. What kind of incompatibility would you expect?

If the properties in question are contained in a schema that is used ONLY within an operation response, then it should be ok to change them from "optional" to "required" and it shouldn't cause any breaking change. Apologies, I should have been clearer in my comment above.