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

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

Open
CarstenLeue opened this issue Nov 29, 2021 · 4 comments

Comments

@CarstenLeue
Copy link

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
Copy link
Member

padamstx commented Dec 6, 2021

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
Copy link
Author

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
Copy link
Author

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
Copy link
Member

padamstx commented Dec 7, 2021

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.

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

No branches or pull requests

2 participants