fix #4441: correcting patch base handling

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

kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/dsl/EditReplacePatchable.java

@@ -63,20 +63,19 @@ public interface EditReplacePatchable<T>
   T accept(Consumer<T> 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.
    * <p>
    * It is the same as calling {@link #patch(PatchContext, Object)} with {@link PatchType#JSON} specified.
    * <p>
-   * 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.
    * <p>
    * 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).
    *
    * <ul>
-   * <li>{@link PatchType#JSON} - will create a JSON patch against the latest server state
+   * <li>{@link PatchType#JSON} - will create a JSON patch against the current item
    * <li>{@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.
    * <li>{@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.
    * <p>
-   * 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.
    * <p>
    * 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.
    * <p>
-   * 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.
    * <p>
-   * 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.
    * <p>
    * Consider using edit instead.
    *
@@ -160,7 +160,7 @@ default T patch(String patch) {
    * resource(item).patch(PatchContext.of(PatchType.SERVER_SIDE_APPLY))
    *
    * <ul>
-   * <li>{@link PatchType#JSON} - will create a JSON patch against the latest server state
+   * <li>{@link PatchType#JSON} - will create a JSON patch using the latest server state as the base
    * <li>{@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.
    * <li>{@link PatchType#STRATEGIC_MERGE} - will send the serialization of the item as a STRATEGIC MERGE patch.

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<T> fromServer() {
+  public BaseOperation<T, L, R> 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() {

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