-
Notifications
You must be signed in to change notification settings - Fork 117
Description
background
Historically, both in openapi and in backend Go code, CS & AMS used same types to represent:
- Fields you must send vs. can omit in
POSTbody when creating resource. - Fields you'll get back in
POSTresponse. Normally same as: - Fields you will always
GETvs. optional. This depends on lifecycle stage and also on cluster type in complicated ways... (the best case is omitting whole sub-structs e.g."aws": {...})
I think for practical client-development purposes, we mostly - Fields you can
PATCH— all are optional here (and Go type marks all pointers to detect omission vs. zero value after de-serialization); some are read-only and can't be patched (but Go type includes them so they can be de-serialized so validator can return error if you sent them). "kind": "FooLink"fields are not present at all but only link to other resource withkind,id, andhref. In CS at least, these still reuse theapi.FooGo structs, forcing all but these 3 fields to be optional pointers.
(This was partly motivated by making it easier for Go to sometimes inline related resources in same place vs. linking them. Backends did add a few?fetchFoo=trueparams like that, but they are small minority.)
☹️ Problem: The combination of all these, is that practically nothing is required in openapi
And for consumers of the API, nearly all fields being optional conveys zero information.
This is a hard problem to attack!
I don't know if it's realistic to move the needle on actual type level, but it's systematically lacking documentation UI would be glad to get even in the form of comments, for a start 🙏
Proposed first step:
At least in metamodel where we make up the rules, we can add formalized notations for all this.
Defining unrelated models e.g. PostCluster vs. PatchCluster here would be a mistake IMHO. It's valuable to know it's same resource with same fields, they just behave differently under different verbs.
-
1. POST body: in
resourcedefinitions, we haveinoption. But I don't think this says anything about POST vs. PATCH bodies, and it doesn't exist forclassfiles... I think POST needs 3 distinct behaviors:@Add(required)— it's an error if you omit this field.@Add(optional)— default if not specified? probably doesn't need syntax.@Add(never)— it's an error if you send this field (e.g.metrics).
Do we need support for separate APIs taking different subsets of same type? iirc AMS has 2 endpoints for creating cluster vs. register offline cluster, but that's rare. Let's ignore for now but keep in mind future possibility of more "verbs".
-
2. can ignore, assume POST response == GET response.
-
3. GET: in
resourcedefinitions, we haveoutoption. But I think GET needs 3 distinct behaviors.@Get(required)— it's an error if you omit this.@Get(optional)— default if not specified? probably doesn't need syntax.@Get(never)— it's an error if you send it (e.g.metrics).
Is there value in separating
@Getvs@Listannotations? Not initially. -
4. PATCH: nothing is required, so I think needs 2 behaviors:
@Patch(optional)@Patch(never)— it's an error if you send it (e.g.metrics).
I'm not sure what should be default. Frequently you can PATCH a field iff you can POST it, but sometimes it's only day-1 or only day-2 (and tends to get relaxed over time).
-
5. We already have
linknotation e.g.link Region CloudRegion.
The syntaxes proposed above are "strawman" suggestions. I'm not even sure they should use annotation @ style.
Future possibilities given such metadata
out of scope here, just mentioning to show this could be used in various ways...
-
openapi: append this data to
description -
openapi: emit custom
x-...fields- UI typescript: generate separate types from each openapi type, per these custom fields?
-
openapi: emit separate types for different scenarios?? [disruptive to existing consumers, including AMS code generation]
-
backend: auto-generate validations of required/never fields under various verbs.
Either as code, or as custom Go struct tags that can be introspected at runtime.