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

Remove null -> [] slice hack #45294

Merged
merged 3 commits into from
Aug 26, 2017
Merged

Conversation

liggitt
Copy link
Member

@liggitt liggitt commented May 3, 2017

Closes #44593

When 1.6 added protobuf storage, the storage layer lost the ability to persist slice fields with empty but non-null values.

As a workaround, we tried to convert empty slice fields to [], rather than null. Compressing null -> [] was just as much of an API breakage as [] -> null, but was hoped to cause fewer problems in clients that don't do null checks.

Because of conversion optimizations around converting lists of objects, the null -> [] hack was discovered to only apply to individual get requests, not to a list of objects. 1.6 and 1.7 was released with this behavior, and the world didn't explode. 1.7 documented the breaking API change that null and [] should be considered equivalent, unless otherwise noted on a particular field.

This PR:

  • Reverts the earlier attempt (Ensure slices are serialized as zero-length, not null #43422) at ensuring non-null json slice output in conversion
  • Makes results of get consistent with the results of list (which helps naive clients that do deepequal comparisons of objects obtained via list/watch and get), and allows empty slice fields to be returned as null
Protobuf serialization does not distinguish between `[]` and `null`.
API fields previously capable of storing and returning either `[]` and `null` via JSON API requests (for example, the Endpoints `subsets` field) can now store only `null` when created using the protobuf content-type or stored in etcd using protobuf serialization (the default in 1.6+). JSON API clients should tolerate `null` values for such fields, and treat `null` and `[]` as equivalent in meaning unless specifically documented otherwise for a particular field.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 3, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 3, 2017
@liggitt liggitt changed the title Ensure null non-omitempty slice fields marshal to [] WIP - Ensure null non-omitempty slice fields marshal to [] May 3, 2017
@dims
Copy link
Member

dims commented May 3, 2017

/unassign @dims

@liggitt
Copy link
Member Author

liggitt commented May 3, 2017

@smarterclayton the type aliasing needed to get custom json encoding is throwing off the protobuf encoder:

-	Subsets []EndpointSubset `json:"subsets" protobuf:"bytes,2,rep,name=subsets"`
+	Subsets EndpointSubsetList `json:"subsets" protobuf:"bytes,2,rep,name=subsets"`
 }
 
+// EndpointSubsetList represents a list of EndpointSubset objects.
+type EndpointSubsetList []EndpointSubset

is resulting in generated protobuf code like this:

-                       m.Subsets = append(m.Subsets, EndpointSubset{})
+                       m.Subsets = append(m.Subsets, EndpointSubsetList{})

any ideas?

@smarterclayton
Copy link
Contributor

smarterclayton commented May 4, 2017 via email

@liggitt liggitt assigned smarterclayton and thockin and unassigned deads2k May 11, 2017
@liggitt liggitt changed the title WIP - Ensure null non-omitempty slice fields marshal to [] Ensure null non-omitempty slice fields marshal to [] May 11, 2017
@liggitt
Copy link
Member Author

liggitt commented May 11, 2017

@smarterclayton - got this working, PTAL

@liggitt
Copy link
Member Author

liggitt commented May 11, 2017

@wojtek-t PTAL at toUnstructured changes in the "Fix unstructured marshaler to handle all JSON types" commit

@wojtek-t
Copy link
Member

@wojtek-t PTAL at toUnstructured changes in the "Fix unstructured marshaler to handle all JSON types" commit

Those changes (last commit) LGTM.

@smarterclayton
Copy link
Contributor

LGTM. I have no objections and follow Jordan's reasoning on this.

@k8s-github-robot k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 19, 2017
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 25, 2017
@smarterclayton
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 25, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, smarterclayton, thockin

Associated issue: 44593

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@liggitt liggitt added this to the v1.8 milestone Aug 25, 2017
@liggitt
Copy link
Member Author

liggitt commented Aug 25, 2017

/retest

1 similar comment
@liggitt
Copy link
Member Author

liggitt commented Aug 26, 2017

/retest

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

2 similar comments
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Aug 26, 2017

@liggitt: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
Jenkins verification 7d9f089ad1dd65f6f708cff548b21652d3a596a2 link @k8s-bot verify test this

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 217513e into kubernetes:master Aug 26, 2017
@liggitt liggitt deleted the proto-slices branch August 28, 2017 00:11
net added a commit to net/libcluster that referenced this pull request Jan 19, 2018
bitwalker pushed a commit to bitwalker/libcluster that referenced this pull request Jan 21, 2018
akhilerm pushed a commit to akhilerm/apimachinery that referenced this pull request Sep 20, 2022
Automatic merge from submit-queue (batch tested with PRs 45345, 49470, 49407, 49448, 49486)

Fix unstructured marshaler to handle all JSON types

Split from kubernetes/kubernetes#45294

/assign @wojtek-t

Kubernetes-commit: 17b9c21a6aca1da03ce0ca1eb6e79eeebf133e3a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants