mirror of https://github.com/halo-dev/halo
Fix the problem of incorrect old data passed to watcher during updates (#4959)
#### What type of PR is this?
/kind bug
/area core
/milestone 2.11.0
#### What this PR does / why we need it:
This PR resolves the problem of incorrect old data passed to watcher during updates. As shown in the following line, the old value should be `old` instead of `extension` from outside.
7a84f55300/application/src/main/java/run/halo/app/extension/ReactiveExtensionClientImpl.java (L172)
#### Does this PR introduce a user-facing change?
```release-note
None
```
pull/4941/head
parent
7a84f55300
commit
5208b5c925
|
@ -17,6 +17,7 @@ import com.fasterxml.jackson.databind.node.TextNode;
|
||||||
import java.io.IOException;
|
import java.io.IOException;
|
||||||
import java.time.Instant;
|
import java.time.Instant;
|
||||||
import java.util.Map;
|
import java.util.Map;
|
||||||
|
import java.util.Objects;
|
||||||
import java.util.Set;
|
import java.util.Set;
|
||||||
|
|
||||||
/**
|
/**
|
||||||
|
@ -117,6 +118,23 @@ class JsonExtension implements Extension {
|
||||||
return new ObjectNodeMetadata(metadataNode);
|
return new ObjectNodeMetadata(metadataNode);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@Override
|
||||||
|
public boolean equals(Object o) {
|
||||||
|
if (this == o) {
|
||||||
|
return true;
|
||||||
|
}
|
||||||
|
if (o == null || getClass() != o.getClass()) {
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
JsonExtension that = (JsonExtension) o;
|
||||||
|
return Objects.equals(objectNode, that.objectNode);
|
||||||
|
}
|
||||||
|
|
||||||
|
@Override
|
||||||
|
public int hashCode() {
|
||||||
|
return Objects.hash(objectNode);
|
||||||
|
}
|
||||||
|
|
||||||
class ObjectNodeMetadata implements MetadataOperator {
|
class ObjectNodeMetadata implements MetadataOperator {
|
||||||
|
|
||||||
private final ObjectNode objectNode;
|
private final ObjectNode objectNode;
|
||||||
|
|
|
@ -4,12 +4,13 @@ import static org.apache.commons.lang3.RandomStringUtils.randomAlphabetic;
|
||||||
import static org.springframework.util.StringUtils.hasText;
|
import static org.springframework.util.StringUtils.hasText;
|
||||||
|
|
||||||
import com.fasterxml.jackson.databind.ObjectMapper;
|
import com.fasterxml.jackson.databind.ObjectMapper;
|
||||||
|
import com.fasterxml.jackson.databind.node.ObjectNode;
|
||||||
import java.time.Duration;
|
import java.time.Duration;
|
||||||
import java.time.Instant;
|
import java.time.Instant;
|
||||||
import java.util.Comparator;
|
import java.util.Comparator;
|
||||||
|
import java.util.HashSet;
|
||||||
import java.util.List;
|
import java.util.List;
|
||||||
import java.util.Objects;
|
import java.util.Objects;
|
||||||
import java.util.concurrent.atomic.AtomicBoolean;
|
|
||||||
import java.util.function.Predicate;
|
import java.util.function.Predicate;
|
||||||
import org.springframework.dao.DataIntegrityViolationException;
|
import org.springframework.dao.DataIntegrityViolationException;
|
||||||
import org.springframework.data.util.Predicates;
|
import org.springframework.data.util.Predicates;
|
||||||
|
@ -137,42 +138,31 @@ public class ReactiveExtensionClientImpl implements ReactiveExtensionClient {
|
||||||
@SuppressWarnings("unchecked")
|
@SuppressWarnings("unchecked")
|
||||||
public <E extends Extension> Mono<E> update(E extension) {
|
public <E extends Extension> Mono<E> update(E extension) {
|
||||||
// Refactor the atomic reference if we have a better solution.
|
// Refactor the atomic reference if we have a better solution.
|
||||||
final var statusChangeOnly = new AtomicBoolean(false);
|
return getLatest(extension).flatMap(old -> {
|
||||||
return getLatest(extension)
|
var oldJsonExt = new JsonExtension(objectMapper, old);
|
||||||
.map(old -> new JsonExtension(objectMapper, old))
|
var newJsonExt = new JsonExtension(objectMapper, extension);
|
||||||
.flatMap(oldJsonExt -> {
|
// reset some mandatory fields
|
||||||
var newJsonExt = new JsonExtension(objectMapper, extension);
|
var oldMetadata = oldJsonExt.getMetadata();
|
||||||
// reset some mandatory fields
|
var newMetadata = newJsonExt.getMetadata();
|
||||||
var oldMetadata = oldJsonExt.getMetadata();
|
newMetadata.setCreationTimestamp(oldMetadata.getCreationTimestamp());
|
||||||
var newMetadata = newJsonExt.getMetadata();
|
newMetadata.setGenerateName(oldMetadata.getGenerateName());
|
||||||
newMetadata.setCreationTimestamp(oldMetadata.getCreationTimestamp());
|
|
||||||
newMetadata.setGenerateName(oldMetadata.getGenerateName());
|
|
||||||
|
|
||||||
var oldObjectNode = oldJsonExt.getInternal().deepCopy();
|
if (Objects.equals(oldJsonExt, newJsonExt)) {
|
||||||
var newObjectNode = newJsonExt.getInternal().deepCopy();
|
// skip updating if not data changed.
|
||||||
if (Objects.equals(oldObjectNode, newObjectNode)) {
|
return Mono.just(extension);
|
||||||
// if no data were changed, just skip updating.
|
}
|
||||||
return Mono.empty();
|
|
||||||
}
|
var onlyStatusChanged =
|
||||||
// check status is changed
|
isOnlyStatusChanged(oldJsonExt.getInternal(), newJsonExt.getInternal());
|
||||||
oldObjectNode.remove("status");
|
|
||||||
newObjectNode.remove("status");
|
var store = this.converter.convertTo(newJsonExt);
|
||||||
if (Objects.equals(oldObjectNode, newObjectNode)) {
|
var updated = client.update(store.getName(), store.getVersion(), store.getData())
|
||||||
statusChangeOnly.set(true);
|
.map(ext -> converter.convertFrom((Class<E>) extension.getClass(), ext));
|
||||||
}
|
if (!onlyStatusChanged) {
|
||||||
return Mono.just(newJsonExt);
|
updated = updated.doOnNext(ext -> watchers.onUpdate(old, ext));
|
||||||
})
|
}
|
||||||
.map(converter::convertTo)
|
return updated;
|
||||||
.flatMap(extensionStore -> client.update(extensionStore.getName(),
|
});
|
||||||
extensionStore.getVersion(),
|
|
||||||
extensionStore.getData()))
|
|
||||||
.map(updated -> converter.convertFrom((Class<E>) extension.getClass(), updated))
|
|
||||||
.doOnNext(updated -> {
|
|
||||||
if (!statusChangeOnly.get()) {
|
|
||||||
watchers.onUpdate(extension, updated);
|
|
||||||
}
|
|
||||||
})
|
|
||||||
.switchIfEmpty(Mono.defer(() -> Mono.just(extension)));
|
|
||||||
}
|
}
|
||||||
|
|
||||||
private Mono<? extends Extension> getLatest(Extension extension) {
|
private Mono<? extends Extension> getLatest(Extension extension) {
|
||||||
|
@ -199,4 +189,26 @@ public class ReactiveExtensionClientImpl implements ReactiveExtensionClient {
|
||||||
this.watchers.addWatcher(watcher);
|
this.watchers.addWatcher(watcher);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
private static boolean isOnlyStatusChanged(ObjectNode oldNode, ObjectNode newNode) {
|
||||||
|
if (Objects.equals(oldNode, newNode)) {
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
// WARNING!!!
|
||||||
|
// Do not edit the ObjectNode
|
||||||
|
var oldFields = new HashSet<String>();
|
||||||
|
var newFields = new HashSet<String>();
|
||||||
|
oldNode.fieldNames().forEachRemaining(oldFields::add);
|
||||||
|
newNode.fieldNames().forEachRemaining(newFields::add);
|
||||||
|
oldFields.remove("status");
|
||||||
|
newFields.remove("status");
|
||||||
|
if (!Objects.equals(oldFields, newFields)) {
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
for (var field : oldFields) {
|
||||||
|
if (!Objects.equals(oldNode.get(field), newNode.get(field))) {
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
return true;
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
|
@ -1,12 +1,21 @@
|
||||||
package run.halo.app.extension;
|
package run.halo.app.extension;
|
||||||
|
|
||||||
|
import lombok.Data;
|
||||||
|
import lombok.EqualsAndHashCode;
|
||||||
|
import lombok.ToString;
|
||||||
|
|
||||||
@GVK(group = "fake.halo.run",
|
@GVK(group = "fake.halo.run",
|
||||||
version = "v1alpha1",
|
version = "v1alpha1",
|
||||||
kind = "Fake",
|
kind = "Fake",
|
||||||
plural = "fakes",
|
plural = "fakes",
|
||||||
singular = "fake")
|
singular = "fake")
|
||||||
|
@Data
|
||||||
|
@ToString(callSuper = true)
|
||||||
|
@EqualsAndHashCode(callSuper = true)
|
||||||
public class FakeExtension extends AbstractExtension {
|
public class FakeExtension extends AbstractExtension {
|
||||||
|
|
||||||
|
private FakeStatus status = new FakeStatus();
|
||||||
|
|
||||||
public static FakeExtension createFake(String name) {
|
public static FakeExtension createFake(String name) {
|
||||||
var metadata = new Metadata();
|
var metadata = new Metadata();
|
||||||
metadata.setName(name);
|
metadata.setName(name);
|
||||||
|
@ -15,4 +24,8 @@ public class FakeExtension extends AbstractExtension {
|
||||||
return fake;
|
return fake;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@Data
|
||||||
|
public static class FakeStatus {
|
||||||
|
private String state;
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
|
@ -455,6 +455,37 @@ class ReactiveExtensionClientTest {
|
||||||
verify(storeClient, never()).update(any(), any(), any());
|
verify(storeClient, never()).update(any(), any(), any());
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
void shouldUpdateIfExtensionStatusChangedOnly() {
|
||||||
|
var fake = createFakeExtension("fake", 2L);
|
||||||
|
fake.getStatus().setState("new-state");
|
||||||
|
var storeName = "/registry/fake.halo.run/fakes/fake";
|
||||||
|
when(converter.convertTo(any())).thenReturn(
|
||||||
|
createExtensionStore(storeName, 2L));
|
||||||
|
when(storeClient.update(any(), any(), any())).thenReturn(
|
||||||
|
Mono.just(createExtensionStore(storeName, 2L)));
|
||||||
|
when(storeClient.fetchByName(storeName)).thenReturn(
|
||||||
|
Mono.just(createExtensionStore(storeName, 1L)));
|
||||||
|
|
||||||
|
var oldFake = createFakeExtension("fake", 2L);
|
||||||
|
oldFake.getStatus().setState("old-state");
|
||||||
|
|
||||||
|
var updatedFake = createFakeExtension("fake", 3L);
|
||||||
|
when(converter.convertFrom(same(FakeExtension.class), any()))
|
||||||
|
.thenReturn(oldFake)
|
||||||
|
.thenReturn(updatedFake);
|
||||||
|
|
||||||
|
StepVerifier.create(client.update(fake))
|
||||||
|
.expectNext(updatedFake)
|
||||||
|
.verifyComplete();
|
||||||
|
|
||||||
|
verify(storeClient).fetchByName(storeName);
|
||||||
|
verify(converter).convertTo(isA(JsonExtension.class));
|
||||||
|
verify(converter, times(2)).convertFrom(same(FakeExtension.class), any());
|
||||||
|
verify(storeClient)
|
||||||
|
.update(eq("/registry/fake.halo.run/fakes/fake"), eq(2L), any());
|
||||||
|
}
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
void shouldUpdateUnstructuredSuccessfully() throws JsonProcessingException {
|
void shouldUpdateUnstructuredSuccessfully() throws JsonProcessingException {
|
||||||
var fake = createUnstructured();
|
var fake = createUnstructured();
|
||||||
|
@ -539,6 +570,13 @@ class ReactiveExtensionClientTest {
|
||||||
verify(watcher, never()).onUpdate(any(), any());
|
verify(watcher, never()).onUpdate(any(), any());
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
void shouldNotWatchOnUpdateIfExtensionStatusChangeOnly() {
|
||||||
|
shouldUpdateIfExtensionStatusChangedOnly();
|
||||||
|
|
||||||
|
verify(watcher, never()).onUpdate(any(), any());
|
||||||
|
}
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
void shouldWatchOnDeleteSuccessfully() {
|
void shouldWatchOnDeleteSuccessfully() {
|
||||||
doNothing().when(watcher).onDelete(any());
|
doNothing().when(watcher).onDelete(any());
|
||||||
|
|
Loading…
Reference in New Issue