From 17ab36d158e78c2c50db5c4258fb76e3d100250f Mon Sep 17 00:00:00 2001 From: Steve Hawkins Date: Fri, 23 Sep 2022 10:12:00 -0400 Subject: [PATCH] fix #4441: correcting patch base handling --- CHANGELOG.md | 1 + .../client/dsl/EditReplacePatchable.java | 24 +++++++++---------- .../client/dsl/internal/BaseOperation.java | 10 ++++---- .../client/mock/ConfigMapCrudTest.java | 10 +++++++- 4 files changed, 28 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0e5b4787b7..27ab104899 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ * Fix #4369: Informers will retry with a backoff on list/watch failure as they did in 5.12 and prior. * Fix #4350: SchemaSwap annotation is now repeatable and is applied multiple times if classes are used more than once in the class hierarchy. * Fix #3733: The authentication command from the .kube/config won't be discarded if no arguments are specified +* Fix #4441: corrected patch base handling for the patch methods available from a Resource - resource(item).patch() will be evaluated as resource(latest).patch(item). Also undeprecated patch(item), which is consistent with leaving patch(context, item) undeprecated as well. For consistency with the other operations (such as edit), patch(item) will use the context item as the base when available, or the server side item when not. This means that patch(item) is only the same as resource(item).patch() when the patch(item) is called when the context item is missing or is the same as the latest. #### Improvements * Fix #4348: Introduce specific annotations for the generators diff --git a/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/dsl/EditReplacePatchable.java b/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/dsl/EditReplacePatchable.java index c47ff3ad27..da2a76c427 100644 --- a/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/dsl/EditReplacePatchable.java +++ b/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/dsl/EditReplacePatchable.java @@ -63,20 +63,19 @@ public interface EditReplacePatchable T accept(Consumer function); /** - * Update field(s) of a resource using a JSON patch. + * Update field(s) of a resource using a JSON patch. The patch is computed against the context item. *

* It is the same as calling {@link #patch(PatchContext, Object)} with {@link PatchType#JSON} specified. *

- * WARNING: This may overwrite concurrent changes (between when you obtained your item and the current state) in an unexpected - * way. + * WARNING: If no context item is available the latest version of that resource will be used as the base + * to compute the diff. If you did not intend for the latest version to be your base, this may overwrite + * concurrent changes (between when you obtained your item and the current state) in an unexpected way. *

* Consider using edit, which allows for a known base, and a builder instead. * * @param item to be patched with patched values * @return returns deserialized version of api server response - * @deprecated use resource(item).patch() or edit instead */ - @Deprecated default T patch(T item) { return patch(PatchContext.of(PatchType.JSON), item); } @@ -85,7 +84,7 @@ default T patch(T item) { * Update field(s) of a resource using type specified in {@link PatchContext}(defaults to strategic merge if not specified). * *

    - *
  • {@link PatchType#JSON} - will create a JSON patch against the latest server state + *
  • {@link PatchType#JSON} - will create a JSON patch against the current item *
  • {@link PatchType#JSON_MERGE} - will send the serialization of the item as a JSON MERGE patch. * Set the resourceVersion to null to prevent optimistic locking. *
  • {@link PatchType#STRATEGIC_MERGE} - will send the serialization of the item as a STRATEGIC MERGE patch. @@ -130,7 +129,7 @@ default T patch(String patch) { /** * Does a PATCH request to the /status subresource ignoring changes to anything except the status stanza. *

    - * This method has the same patching behavior as {@link #patch(PatchContext, Object)}, with + * This method has the same patching behavior as {@link #patch(PatchContext)}, with * {@link PatchType#JSON_MERGE} but against the status subresource. *

    * Set the resourceVersion to null to prevent optimistic locking. @@ -140,12 +139,13 @@ default T patch(String patch) { T patchStatus(); /** - * Update field(s) of a resource using a JSON patch. + * Update field(s) of a resource using a JSON patch which will be computed using the latest + * server state as the base. *

    - * It is the same as calling {@link #patch(PatchContext, Object)} with {@link PatchType#JSON} specified. + * It is the same as calling {@link #patch(PatchContext)} with {@link PatchType#JSON} specified. *

    - * WARNING: This may overwrite concurrent changes (between when you obtained your item and the current state) in an unexpected - * way. + * WARNING: If you did not intend for the latest version to be your base, this may overwrite + * concurrent changes (between when you obtained your item and the current state) in an unexpected way. *

    * Consider using edit instead. * @@ -160,7 +160,7 @@ default T patch(String patch) { * resource(item).patch(PatchContext.of(PatchType.SERVER_SIDE_APPLY)) * *

      - *
    • {@link PatchType#JSON} - will create a JSON patch against the latest server state + *
    • {@link PatchType#JSON} - will create a JSON patch using the latest server state as the base *
    • {@link PatchType#JSON_MERGE} - will send the serialization of the item as a JSON MERGE patch. * Set the resourceVersion to null to prevent optimistic locking. *
    • {@link PatchType#STRATEGIC_MERGE} - will send the serialization of the item as a STRATEGIC MERGE patch. diff --git a/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/BaseOperation.java b/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/BaseOperation.java index 4dd50c5a06..28ec1394a6 100755 --- a/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/BaseOperation.java +++ b/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/BaseOperation.java @@ -276,7 +276,7 @@ public R load(String path) { } @Override - public ExtensibleResource fromServer() { + public BaseOperation fromServer() { return newInstance(context.withReloadingFromServer(true)); } @@ -522,17 +522,19 @@ public T patchStatus(T item) { @Override public T patchStatus() { - return patchStatus(getNonNullItem()); + // fromServer shouldn't be necessary here as we're using a merge patch, but + // just in case that changes we want consistency with the other patch methods + return this.fromServer().patchStatus(getNonNullItem()); } @Override public T patch() { - return patch(getNonNullItem()); + return this.fromServer().patch(getNonNullItem()); } @Override public T patch(PatchContext patchContext) { - return patch(patchContext, getNonNullItem()); + return this.fromServer().patch(patchContext, getNonNullItem()); } protected T getNonNullItem() { diff --git a/kubernetes-tests/src/test/java/io/fabric8/kubernetes/client/mock/ConfigMapCrudTest.java b/kubernetes-tests/src/test/java/io/fabric8/kubernetes/client/mock/ConfigMapCrudTest.java index 60999e0382..1297bb406f 100644 --- a/kubernetes-tests/src/test/java/io/fabric8/kubernetes/client/mock/ConfigMapCrudTest.java +++ b/kubernetes-tests/src/test/java/io/fabric8/kubernetes/client/mock/ConfigMapCrudTest.java @@ -135,9 +135,17 @@ void testPatchBase() { client.configMaps().inNamespace("ns1").resource(configmap2).create(); client.configMaps().inNamespace("ns1").resource(configmap2a).replace(); - // should still diff to latest + // No-op - base and item are the same ConfigMap result = client.configMaps().inNamespace("ns1").resource(configmap2).patch(configmap2); assertEquals(Collections.singletonMap("three", "3"), result.getData()); + + // this will patch the server value, so the base becomes 2a + result = client.configMaps().inNamespace("ns1").resource(configmap2).patch(); + assertEquals(Collections.singletonMap("two", "2"), result.getData()); + + // this will patch the server value, so the base becomes 2 + result = client.configMaps().inNamespace("ns1").resource(configmap2a).patch(PatchContext.of(PatchType.JSON)); + assertEquals(Collections.singletonMap("three", "3"), result.getData()); } @Test