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

Proposal: don't return JSONBase in LIST responses #1223

Closed
bgrant0607 opened this issue Sep 9, 2014 · 7 comments
Closed

Proposal: don't return JSONBase in LIST responses #1223

bgrant0607 opened this issue Sep 9, 2014 · 7 comments
Labels
area/api Indicates an issue on api area.

Comments

@bgrant0607
Copy link
Member

This has been discussed in several places.

Right now JSONBase, which includes Kind, ID, CreationTimestamp, SelfLink, ResourceVersion, and APIVersion, is returned in most API responses, including lists (GET of collection URLs), in particular. Most of this information is relevant to specific API objects rather than to lists of multiple objects.

As we make several others changes, such as moving labels and/or other metadata into the object base (#1200, #1201), adding names (#1216), etc., proper use of JSONBase will be more important.

Proposal: Remove JSONBase everywhere except for API objects (including Kubelet-specific API objects, once it has its own objects distinct from the apiserver) -- in particular, remove it from the List types.

Also, I'm not sure what we do now, but we should return at least the JSONBase portion of the created object with the POST response (which would be facilitated by nesting JSONBase in a named field). I'd return just ResourceVersion with other mutating operations (PUT, DELETE), whatever other information is needed to scope it (e.g., kind -- see #1184), and APIVersion, so clients can figure out what to do in the case of a version mismatch.

We could also rename JSONBase to APIObjectBase or somesuch, but that's less critical.

@bgrant0607 bgrant0607 added the area/api Indicates an issue on api area. label Sep 9, 2014
@lavalamp
Copy link
Member

lavalamp commented Sep 9, 2014

Also, I'm not sure what we do now, but we should return at least the JSONBase portion of the created object with the POST response (which would be facilitated by nesting JSONBase in a named field).

What does this mean? We return the entire object, which includes the fields in the JSONBase.

I'd return just ResourceVersion with other mutating operations (PUT, DELETE)

Why? It's extra work to return less of the information.

I'm not sure I understand all of your motivations, but I'd make a counter proposal: Replace all of our XList types with a single List type, which contains an array of EmbeddedObjects.

Please note that the fields in JSONBase are all ",omitempty" which means that in practice, they will be unset for XList objects.

Also note that APIVersion and Kind fields must be there to enable our decoding & versioning.

@bgrant0607
Copy link
Member Author

@lavalamp

First, the list operations, which is what this issue is mainly about: An array of objects is the right idea, but what information would be returned in the case of an empty list or error? I think there's still some minimal info we should return.

Re. omitempty: I don't think we should specify JSONBase if the fields aren't populated in practice. That's confusing and would make automatic documentation generation more difficult.

Do we return the objects in DELETE responses? I didn't think so.

Returning whole objects in POST and PUT responses is ok, I suppose, though it seems like a waste of bandwidth, other than the creation/mutation metadata. The client may want to get output-only fields and default values, but much of the current state may not even be initialized/updated at the time of response.

@smarterclayton
Copy link
Contributor

In delete we return api.Status objects, but not the object we delete (delete can contain the resourceVersion for read-your-writes).

Having typed list objects is nice - I can buy though that only a subset of metadata described for regular resources is present. However, there are some lists which are essentially objects and are versioned as such (i'm thinking about the list of pods scheduled on a host or the list of endpoints for a service).

@brendandburns
Copy link
Contributor

We need to at least have a Kind and API version fields, SelfLink seems
reasonable, so that we know how to decode, so that leaves 3 irrelevant
fields: (ID, timestamp, resource version), and I can hypothesize a
reasonable value for timestamp (most recent timestamp of things being
listed).

Do we really need a separate object so that we don't have two irrelevant
null fields?

On Mon, Sep 8, 2014 at 8:19 PM, Clayton Coleman notifications@github.com
wrote:

In delete we return api.Status objects, but not the object we delete
(delete can contain the resourceVersion for read-your-writes).

Having typed list objects is nice - I can buy though that only a subset of
metadata described for regular resources is present. However, there are
some lists which are essentially objects and are versioned as such (i'm
thinking about the list of pods scheduled on a host or the list of
endpoints for a service).


Reply to this email directly or view it on GitHub
#1223 (comment)
.

@bgrant0607
Copy link
Member Author

Ok, fine, maybe we could just document that ID (and, later, name and uid, and labels and other metadata if we add them to JSONBase) is not populated in LIST operations. What value would be synthesized for timestamp?

@smarterclayton
Copy link
Contributor

I've got proposals for this coming tomorrow - I'm starting to lean a bit towards Brian's point for lists that aren't real objects themselves

On Sep 8, 2014, at 11:41 PM, bgrant0607 notifications@github.com wrote:

Ok, fine, maybe we could just document that ID (and, later, name and uid, and labels and other metadata if we add them to JSONBase) is not populated in LIST operations. What value would be synthesized for timestamp?


Reply to this email directly or view it on GitHub.

@smarterclayton
Copy link
Contributor

Covered by API v1beta3 #1225

ehashman pushed a commit to ehashman/kubernetes that referenced this issue Apr 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api Indicates an issue on api area.
Projects
None yet
Development

No branches or pull requests

4 participants