From 2d268d2a3adc1e9a06baddd47a57699fbcd453cc Mon Sep 17 00:00:00 2001 From: vtiwari5 Date: Sun, 25 Jan 2026 23:25:20 -0500 Subject: [PATCH 1/8] Add metadata propagation in source, source transformer, mapper, sink Signed-off-by: vtiwari5 --- .../io/numaproj/numaflow/mapper/Datum.java | 18 ++ .../numaflow/mapper/HandlerDatum.java | 14 +- .../numaproj/numaflow/mapper/MapperActor.java | 11 +- .../numaflow/mapper/MapperTestKit.java | 4 + .../io/numaproj/numaflow/mapper/Message.java | 30 ++- .../numaflow/shared/SystemMetadata.java | 98 ++++++++ .../numaflow/shared/UserMetadata.java | 222 ++++++++++++++++++ .../numaflow/sinker/KeyValueGroup.java | 16 -- .../io/numaproj/numaflow/sinker/Message.java | 5 +- .../io/numaproj/numaflow/sinker/Response.java | 49 +--- .../io/numaproj/numaflow/sourcer/Message.java | 53 ++++- .../numaflow/sourcer/OutputObserverImpl.java | 2 + .../numaflow/sourcetransformer/Datum.java | 17 ++ .../sourcetransformer/HandlerDatum.java | 14 ++ .../numaflow/sourcetransformer/Message.java | 27 ++- .../SourceTransformerTestKit.java | 4 + .../sourcetransformer/TransformerActor.java | 10 +- src/main/proto/map/v1/map.proto | 3 + src/main/proto/sink/v1/sink.proto | 1 + src/main/proto/source/v1/source.proto | 3 + .../v1/sourcetransformer.proto | 3 + .../numaflow/mapper/HandlerDatumTest.java | 4 +- .../numaflow/sinker/ResponseTest.java | 9 +- 23 files changed, 533 insertions(+), 84 deletions(-) create mode 100644 src/main/java/io/numaproj/numaflow/shared/SystemMetadata.java create mode 100644 src/main/java/io/numaproj/numaflow/shared/UserMetadata.java delete mode 100644 src/main/java/io/numaproj/numaflow/sinker/KeyValueGroup.java diff --git a/src/main/java/io/numaproj/numaflow/mapper/Datum.java b/src/main/java/io/numaproj/numaflow/mapper/Datum.java index 0ef1a55f..f02ff937 100644 --- a/src/main/java/io/numaproj/numaflow/mapper/Datum.java +++ b/src/main/java/io/numaproj/numaflow/mapper/Datum.java @@ -1,6 +1,9 @@ package io.numaproj.numaflow.mapper; +import io.numaproj.numaflow.shared.SystemMetadata; +import io.numaproj.numaflow.shared.UserMetadata; + import java.time.Instant; import java.util.Map; @@ -36,4 +39,19 @@ public interface Datum { * @return returns the headers in the form of key value pair */ Map getHeaders(); + + /** + * method to get the metadata information added by the user. + * It can be appended to and passed downstream. + * + * @return returns the UserMetadata object + */ + UserMetadata getUserMetadata(); + + /** + * method to get the read-only system metadata information + * + * @return returns the SystemMetadata object + */ + SystemMetadata getSystemMetadata(); } diff --git a/src/main/java/io/numaproj/numaflow/mapper/HandlerDatum.java b/src/main/java/io/numaproj/numaflow/mapper/HandlerDatum.java index 17945596..d6228b86 100644 --- a/src/main/java/io/numaproj/numaflow/mapper/HandlerDatum.java +++ b/src/main/java/io/numaproj/numaflow/mapper/HandlerDatum.java @@ -1,6 +1,8 @@ package io.numaproj.numaflow.mapper; +import io.numaproj.numaflow.shared.SystemMetadata; +import io.numaproj.numaflow.shared.UserMetadata; import lombok.AllArgsConstructor; import java.time.Instant; @@ -13,7 +15,8 @@ class HandlerDatum implements Datum { private Instant watermark; private Instant eventTime; private Map headers; - + private UserMetadata userMetadata; + private SystemMetadata systemMetadata; @Override public Instant getWatermark() { @@ -35,4 +38,13 @@ public Map getHeaders() { return this.headers; } + @Override + public UserMetadata getUserMetadata() { + return this.userMetadata; + } + + @Override + public SystemMetadata getSystemMetadata() { + return this.systemMetadata; + } } diff --git a/src/main/java/io/numaproj/numaflow/mapper/MapperActor.java b/src/main/java/io/numaproj/numaflow/mapper/MapperActor.java index a6a56078..a517e816 100644 --- a/src/main/java/io/numaproj/numaflow/mapper/MapperActor.java +++ b/src/main/java/io/numaproj/numaflow/mapper/MapperActor.java @@ -4,12 +4,15 @@ import akka.actor.Props; import akka.japi.pf.ReceiveBuilder; import com.google.protobuf.ByteString; +import common.MetadataOuterClass; import io.numaproj.numaflow.map.v1.MapOuterClass; -import io.numaproj.numaflow.sourcetransformer.v1.Sourcetransformer; +import io.numaproj.numaflow.shared.SystemMetadata; +import io.numaproj.numaflow.shared.UserMetadata; import java.time.Instant; import java.util.ArrayList; import java.util.Arrays; +import java.util.HashMap; /** * Mapper actor that processes the map request. It invokes the mapper to process the request and @@ -48,7 +51,9 @@ private void processRequest(MapOuterClass.MapRequest mapRequest) { Instant.ofEpochSecond( mapRequest.getRequest().getEventTime().getSeconds(), mapRequest.getRequest().getEventTime().getNanos()), - mapRequest.getRequest().getHeadersMap() + mapRequest.getRequest().getHeadersMap(), + new UserMetadata(mapRequest.getRequest().getMetadata()), + new SystemMetadata(mapRequest.getRequest().getMetadata()) ); String[] keys = mapRequest.getRequest().getKeysList().toArray(new String[0]); try { @@ -89,6 +94,8 @@ private MapOuterClass.MapResponse buildResponse(MessageList messageList, String == null ? new ArrayList<>() : Arrays.asList(message.getKeys())) .addAllTags(message.getTags() == null ? new ArrayList<>() : Arrays.asList(message.getTags())) + .setMetadata(message.getUserMetadata() + == null ? MetadataOuterClass.Metadata.getDefaultInstance() : message.getUserMetadata().toProto()) .build()); }); return responseBuilder.setId(ID).build(); diff --git a/src/main/java/io/numaproj/numaflow/mapper/MapperTestKit.java b/src/main/java/io/numaproj/numaflow/mapper/MapperTestKit.java index 9567568f..71d2326f 100644 --- a/src/main/java/io/numaproj/numaflow/mapper/MapperTestKit.java +++ b/src/main/java/io/numaproj/numaflow/mapper/MapperTestKit.java @@ -7,6 +7,8 @@ import io.grpc.stub.StreamObserver; import io.numaproj.numaflow.map.v1.MapGrpc; import io.numaproj.numaflow.map.v1.MapOuterClass; +import io.numaproj.numaflow.shared.SystemMetadata; +import io.numaproj.numaflow.shared.UserMetadata; import lombok.Builder; import lombok.Getter; import lombok.extern.slf4j.Slf4j; @@ -233,5 +235,7 @@ public static class TestDatum implements Datum { private final Instant eventTime; private final Instant watermark; private final Map headers; + private final UserMetadata userMetadata; + private final SystemMetadata systemMetadata; } } diff --git a/src/main/java/io/numaproj/numaflow/mapper/Message.java b/src/main/java/io/numaproj/numaflow/mapper/Message.java index 57f939f0..518a6f62 100644 --- a/src/main/java/io/numaproj/numaflow/mapper/Message.java +++ b/src/main/java/io/numaproj/numaflow/mapper/Message.java @@ -1,7 +1,12 @@ package io.numaproj.numaflow.mapper; +import io.numaproj.numaflow.shared.UserMetadata; import lombok.Getter; +import java.util.HashMap; +import java.util.Map; +import java.util.stream.Collectors; + /** Message is used to wrap the data returned by Mapper. */ @Getter public class Message { @@ -9,19 +14,23 @@ public class Message { private final String[] keys; private final byte[] value; private final String[] tags; + private final UserMetadata userMetadata; /** - * used to create Message with value, keys and tags(used for conditional forwarding) + * used to create Message with value, keys, tags(used for conditional forwarding) and userMetadata * * @param value message value * @param keys message keys * @param tags message tags which will be used for conditional forwarding + * @param userMetadata user metadata, this is used to pass user defined metadata to the next vertex */ - public Message(byte[] value, String[] keys, String[] tags) { + public Message(byte[] value, String[] keys, String[] tags, UserMetadata userMetadata) { // defensive copy - once the Message is created, the caller should not be able to modify it. this.keys = keys == null ? null : keys.clone(); this.value = value == null ? null : value.clone(); this.tags = tags == null ? null : tags.clone(); + // Copy the data using copy constructor to prevent mutation + this.userMetadata = userMetadata == null ? null : new UserMetadata(userMetadata); } /** @@ -30,7 +39,7 @@ public Message(byte[] value, String[] keys, String[] tags) { * @param value message value */ public Message(byte[] value) { - this(value, null, null); + this(value, null, null, null); } /** @@ -40,7 +49,18 @@ public Message(byte[] value) { * @param keys message keys */ public Message(byte[] value, String[] keys) { - this(value, keys, null); + this(value, keys, null, null); + } + + /** + * used to create Message with value, keys and tags(used for conditional forwarding) + * + * @param value message value + * @param keys message keys + * @param tags message tags which will be used for conditional forwarding + */ + public Message(byte[] value, String[] keys, String[] tags) { + this(value, keys, tags, null); } /** @@ -49,6 +69,6 @@ public Message(byte[] value, String[] keys) { * @return returns the Message which will be dropped */ public static Message toDrop() { - return new Message(new byte[0], null, DROP_TAGS); + return new Message(new byte[0], null, DROP_TAGS, null); } } diff --git a/src/main/java/io/numaproj/numaflow/shared/SystemMetadata.java b/src/main/java/io/numaproj/numaflow/shared/SystemMetadata.java new file mode 100644 index 00000000..a69a2139 --- /dev/null +++ b/src/main/java/io/numaproj/numaflow/shared/SystemMetadata.java @@ -0,0 +1,98 @@ +package io.numaproj.numaflow.shared; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.stream.Collectors; + +import common.MetadataOuterClass; +import lombok.AllArgsConstructor; +import lombok.Getter; + +/** + * SystemMetadata is mapping of group name to key-value pairs + * SystemMetadata wraps system-generated metadata groups per message. + * It is read-only to UDFs + */ +@Getter +@AllArgsConstructor +public class SystemMetadata { + private final Map> data; + + /** + * Default constructor + */ + public SystemMetadata() { + this.data = new HashMap<>(); + } + + /** + * Constructor from MetadataOuterClass.Metadata + * + * @param metadata is an instance of MetadataOuterClass.Metadata which contains system metadata + */ + public SystemMetadata(MetadataOuterClass.Metadata metadata) { + if (metadata == null || metadata.getSysMetadataMap().isEmpty()) { + this.data = new HashMap<>(); + return; + } + this.data = metadata.getSysMetadataMap().entrySet().stream() + .collect(Collectors.toMap( + Map.Entry::getKey, + e -> new HashMap<>(e.getValue() + .getKeyValueMap() + .entrySet() + .stream() + .collect(Collectors.toMap( + Map.Entry::getKey, + e1 -> e1.getValue().toByteArray() + )) + ) + )); + } + + /** + * Get the list of all groups present in the user metadata + * + * @return list of group names + */ + public List getGroups() { + if (this.data == null) { + return new ArrayList<>(); + } + return new ArrayList<>(this.data.keySet()); + } + + /** + * Get a list of key names within a given group + * + * @param group is the name of the group from which to get the key names + * @return a list of key names within the group + */ + public List getKeys(String group) { + if (this.data == null || !this.data.containsKey(group)) { + return new ArrayList<>(); + } + return new ArrayList<>(this.data.get(group).keySet()); + } + + /** + * Get the value of a key in a group + * + * @param group Name of the group which contains the key holding required value + * @param key Name of the key in the group for which value is required + * @return Value of the key in the group or null if the group/key is not present + */ + public byte[] getValue(String group, String key) { + if (this.data == null) { + return null; + } + Map groupData = this.data.get(group); + if (groupData == null) { + return null; + } + byte[] value = groupData.get(key); + return value == null ? null : value.clone(); + } +} diff --git a/src/main/java/io/numaproj/numaflow/shared/UserMetadata.java b/src/main/java/io/numaproj/numaflow/shared/UserMetadata.java new file mode 100644 index 00000000..c64c8592 --- /dev/null +++ b/src/main/java/io/numaproj/numaflow/shared/UserMetadata.java @@ -0,0 +1,222 @@ +package io.numaproj.numaflow.shared; + +import common.MetadataOuterClass; +import lombok.AllArgsConstructor; +import lombok.Getter; +import com.google.protobuf.ByteString; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.stream.Collectors; + +/** + * UserMetadata is mapping of group name to key-value pairs + * UserMetadata wraps user generated metadata groups per message. + * It can be appended to and passed on to the downstream. + * {@link lombok.AllArgsConstructor} allows creation of UserMetadata from a `Map>` + * Copy constructor allows creation of UserMetadata from another UserMetadata + */ +@Getter +@AllArgsConstructor +public class UserMetadata { + private final Map> data; + + /** + * Default constructor + */ + public UserMetadata() { + this.data = new HashMap<>(); + } + + /** + * Constructor from MetadataOuterClass.Metadata + * + * @param metadata is an instance of MetadataOuterClass.Metadata which contains user metadata + */ + public UserMetadata(MetadataOuterClass.Metadata metadata) { + if (metadata == null || metadata.getUserMetadataMap().isEmpty()) { + this.data = new HashMap<>(); + return; + } + // Copy the data to prevent mutation + this.data = metadata.getUserMetadataMap() + .entrySet() + .stream() + .collect(Collectors.toMap( + Map.Entry::getKey, + e -> new HashMap<>(e.getValue().getKeyValueMap().entrySet().stream() + .collect(Collectors.toMap( + Map.Entry::getKey, + e1 -> e1.getValue().toByteArray() + )) + ) + )); + } + + /** + * Copy constructor + * + * @param userMetadata the user metadata to copy + */ + public UserMetadata(UserMetadata userMetadata) { + if (userMetadata == null || userMetadata.data == null) { + this.data = new HashMap<>(); + return; + } + + // Deep copy the data to prevent mutation + this.data = userMetadata.data.entrySet().stream() + .filter(e -> e.getValue() != null) + .collect(Collectors.toMap( + Map.Entry::getKey, + e -> e.getValue().entrySet() + .stream() + .filter(e1 -> e1.getValue() != null) + .collect(Collectors.toMap( + Map.Entry::getKey, + e1 -> e1.getValue().clone() + )) + )); + } + + /** + * Convert the user metadata to a map that can be used to create MetadataOuterClass.Metadata + * + * @return MetadataOuterClass.Metadata + */ + public MetadataOuterClass.Metadata toProto() { + if (this.data == null) { + return MetadataOuterClass.Metadata.getDefaultInstance(); + } + + Map result = new HashMap<>(); + this.data.forEach((group, kvMap) -> { + MetadataOuterClass.KeyValueGroup.Builder builder = MetadataOuterClass.KeyValueGroup.newBuilder(); + if (kvMap != null) { + kvMap.forEach((key, value) -> { + if (value == null) { + builder.putKeyValue(key, ByteString.EMPTY); + } else { + builder.putKeyValue(key, ByteString.copyFrom(value)); + } + }); + } + result.put(group, builder.build()); + }); + + return MetadataOuterClass.Metadata + .newBuilder() + .putAllUserMetadata(result) + .build(); + } + + /** + * Get the list of all groups present in the user metadata + * + * @return list of group names + */ + public List getGroups() { + if (this.data == null) { + return new ArrayList<>(); + } + return new ArrayList<>(this.data.keySet()); + } + + /** + * Delete a group from the user metadata + * + * @param group is the name of the group to delete + */ + public void deleteGroup(String group) { + if (this.data == null) { + return; + } + this.data.remove(group); + } + + /** + * Get a list of key names within a given group + * + * @param group is the name of the group from which to get the key names + * @return a list of key names within the group + */ + public List getKeys(String group) { + if (this.data == null || !this.data.containsKey(group)) { + return new ArrayList<>(); + } + return new ArrayList<>(this.data.get(group).keySet()); + } + + /** + * Delete a key from a group + * + * @param group Name of the group containing the key + * @param key Name of the key to delete + */ + public void deleteKey(String group, String key) { + if (this.data == null || !this.data.containsKey(group)) { + return; + } + this.data.get(group).remove(key); + } + + /** + * Add a key value pair to a group + * + * @param group Name of the group to which key value pairs are to be added + * @param key Name of the key in group to which the value is to be added + * @param value Value to be added to the key + */ + public void addKV(String group, String key, byte[] value) { + if (this.data == null) { + return; + } + this.data.computeIfAbsent(group, k -> new HashMap<>()).put(key, value.clone()); + } + + /** + * Add multiple key value pairs to a group + * + * @param group Name of the group to which key value pairs are to be added + * @param kv Map of key value pairs to be added to the group + */ + public void addKVs(String group, Map kv) { + if (this.data == null) { + return; + } + Map groupMap = this.data.computeIfAbsent(group, k -> new HashMap<>()); + // Copy the values to prevent mutation + kv.forEach((key, value) -> groupMap.put(key, value.clone())); + } + + /** + * Get the value of a key in a group + * + * @param group Name of the group which contains the key holding required value + * @param key Name of the key in the group for which value is required + * @return Value of the key in the group or null if the group/key is not present + */ + public byte[] getValue(String group, String key) { + if (this.data == null) { + return null; + } + Map groupData = this.data.get(group); + if (groupData == null) { + return null; + } + byte[] value = groupData.get(key); + return value == null ? null : value.clone(); + } + + /** + * Clear all the user metadata + */ + public void clear() { + if (this.data == null) { + return; + } + this.data.clear(); + } +} diff --git a/src/main/java/io/numaproj/numaflow/sinker/KeyValueGroup.java b/src/main/java/io/numaproj/numaflow/sinker/KeyValueGroup.java deleted file mode 100644 index abc0db08..00000000 --- a/src/main/java/io/numaproj/numaflow/sinker/KeyValueGroup.java +++ /dev/null @@ -1,16 +0,0 @@ -package io.numaproj.numaflow.sinker; - -import lombok.Builder; -import lombok.Getter; - -import java.util.HashMap; - -/** - * KeyValueGroup is a map of key-value pairs for a given group. - * Used as part of {@link io.numaproj.numaflow.sinker.Message}. - */ -@Getter -@Builder -public class KeyValueGroup { - private final HashMap keyValue; -} diff --git a/src/main/java/io/numaproj/numaflow/sinker/Message.java b/src/main/java/io/numaproj/numaflow/sinker/Message.java index d8ede1cf..d62c7b47 100644 --- a/src/main/java/io/numaproj/numaflow/sinker/Message.java +++ b/src/main/java/io/numaproj/numaflow/sinker/Message.java @@ -1,10 +1,9 @@ package io.numaproj.numaflow.sinker; +import io.numaproj.numaflow.shared.UserMetadata; import lombok.Builder; import lombok.Getter; -import java.util.HashMap; - /** * Message contains information that needs to be sent to the OnSuccess sink. * The message can be different from the original message that was sent to primary sink. @@ -14,7 +13,7 @@ public class Message { private final byte[] value; private final String key; - private final HashMap userMetadata; + private final UserMetadata userMetadata; } diff --git a/src/main/java/io/numaproj/numaflow/sinker/Response.java b/src/main/java/io/numaproj/numaflow/sinker/Response.java index b0c2b217..bb60eb40 100644 --- a/src/main/java/io/numaproj/numaflow/sinker/Response.java +++ b/src/main/java/io/numaproj/numaflow/sinker/Response.java @@ -7,10 +7,6 @@ import lombok.AllArgsConstructor; import lombok.Getter; -import java.util.Collections; -import java.util.Map; -import java.util.stream.Collectors; - /** * Response is used to send response from the user defined sinker. It contains the id of the * message, success status, an optional error message and a fallback status. Various static factory @@ -100,45 +96,14 @@ public static Response responseOnSuccess(String id, Message onSuccessMessage) { if (onSuccessMessage == null) { return new Response(id, false, null, false, false, null, true, null); } else { - - Map pbUserMetadata = MetadataOuterClass.Metadata - .getDefaultInstance() - .getUserMetadataMap(); - - if (onSuccessMessage.getUserMetadata() != null) { - pbUserMetadata = - onSuccessMessage.getUserMetadata() - .entrySet() - .stream() - .filter(e -> e.getKey() != null && e.getValue() != null) - .collect(Collectors.toMap( - Map.Entry::getKey, - e -> MetadataOuterClass.KeyValueGroup.newBuilder() - .putAllKeyValue(e.getValue().getKeyValue() == null - ? Collections.emptyMap() - : e.getValue() - .getKeyValue() - .entrySet() - .stream() - .filter(kv -> kv.getKey() != null - && kv.getValue() != null) - .collect(Collectors.toMap( - Map.Entry::getKey, - kv -> ByteString.copyFrom(kv.getValue()) - )) - ) - .build() - )); - } - - MetadataOuterClass.Metadata pbMetadata = MetadataOuterClass.Metadata.newBuilder() - .putAllUserMetadata(pbUserMetadata) - .build(); - SinkResponse.Result.Message pbOnSuccessMessage = SinkResponse.Result.Message.newBuilder() - .addKeys(onSuccessMessage.getKey() == null ? "" : onSuccessMessage.getKey()) - .setValue(onSuccessMessage.getValue() == null ? ByteString.EMPTY : ByteString.copyFrom(onSuccessMessage.getValue())) - .setMetadata(pbMetadata) + .addKeys(onSuccessMessage.getKey() + == null ? "" : onSuccessMessage.getKey()) + .setValue(onSuccessMessage.getValue() + == null ? ByteString.EMPTY : ByteString.copyFrom(onSuccessMessage.getValue())) + .setMetadata(onSuccessMessage.getUserMetadata() + == null ? MetadataOuterClass.Metadata.getDefaultInstance() + : onSuccessMessage.getUserMetadata().toProto()) .build(); return new Response(id, false, null, false, false, null, true, pbOnSuccessMessage); diff --git a/src/main/java/io/numaproj/numaflow/sourcer/Message.java b/src/main/java/io/numaproj/numaflow/sourcer/Message.java index cc8502bf..310666c1 100644 --- a/src/main/java/io/numaproj/numaflow/sourcer/Message.java +++ b/src/main/java/io/numaproj/numaflow/sourcer/Message.java @@ -2,6 +2,8 @@ import java.time.Instant; import java.util.Map; + +import io.numaproj.numaflow.shared.UserMetadata; import lombok.Getter; /** Message is used to wrap the data returned by Sourcer. */ @@ -13,6 +15,7 @@ public class Message { private final Offset offset; private final Instant eventTime; private final Map headers; + private final UserMetadata userMetadata; /** * used to create Message with value, offset and eventTime. @@ -22,7 +25,7 @@ public class Message { * @param eventTime message eventTime */ public Message(byte[] value, Offset offset, Instant eventTime) { - this(value, offset, eventTime, null, null); + this(value, offset, eventTime, null, null, null); } /** @@ -34,7 +37,7 @@ public Message(byte[] value, Offset offset, Instant eventTime) { * @param keys message keys */ public Message(byte[] value, Offset offset, Instant eventTime, String[] keys) { - this(value, offset, eventTime, keys, null); + this(value, offset, eventTime, keys, null, null); } /** @@ -46,7 +49,33 @@ public Message(byte[] value, Offset offset, Instant eventTime, String[] keys) { * @param headers message headers */ public Message(byte[] value, Offset offset, Instant eventTime, Map headers) { - this(value, offset, eventTime, null, headers); + this(value, offset, eventTime, null, headers, null); + } + + /** + * used to create Message with value, offset, eventTime, keys and userMetadata. + * + * @param value message value + * @param offset message offset + * @param eventTime message eventTime + * @param keys message keys + * @param userMetadata message userMetadata + */ + public Message(byte[] value, Offset offset, Instant eventTime, String[] keys, UserMetadata userMetadata) { + this(value, offset, eventTime, keys, null, userMetadata); + } + + /** + * used to create Message with value, offset, eventTime, headers and userMetadata. + * + * @param value message value + * @param offset message offset + * @param eventTime message eventTime + * @param headers message headers + * @param userMetadata message userMetadata + */ + public Message(byte[] value, Offset offset, Instant eventTime, Map headers, UserMetadata userMetadata) { + this(value, offset, eventTime, null, headers, userMetadata); } /** @@ -58,8 +87,22 @@ public Message(byte[] value, Offset offset, Instant eventTime, Map headers) { + this(value, offset, eventTime, keys, headers, null); + } + + /** + * used to create Message with value, offset, eventTime, keys, headers and userMetadata. + * + * @param value message value + * @param offset message offset + * @param eventTime message eventTime + * @param keys message keys + * @param headers message headers + * @param userMetadata message userMetadata + */ public Message( - byte[] value, Offset offset, Instant eventTime, String[] keys, Map headers) { + byte[] value, Offset offset, Instant eventTime, String[] keys, Map headers, UserMetadata userMetadata) { // defensive copy - once the Message is created, the caller should not be able to modify it. this.keys = keys == null ? null : keys.clone(); this.value = value == null ? null : value.clone(); @@ -67,5 +110,7 @@ public Message( this.offset = offset == null ? null : new Offset(offset.getValue(), offset.getPartitionId()); // The Instant class in Java is already immutable. this.eventTime = eventTime; + // Copy the data using copy constructor to prevent mutation + this.userMetadata = userMetadata == null ? null : new UserMetadata(userMetadata); } } diff --git a/src/main/java/io/numaproj/numaflow/sourcer/OutputObserverImpl.java b/src/main/java/io/numaproj/numaflow/sourcer/OutputObserverImpl.java index 14a23543..3bc9a7c1 100644 --- a/src/main/java/io/numaproj/numaflow/sourcer/OutputObserverImpl.java +++ b/src/main/java/io/numaproj/numaflow/sourcer/OutputObserverImpl.java @@ -2,6 +2,7 @@ import com.google.protobuf.ByteString; import com.google.protobuf.Timestamp; +import common.MetadataOuterClass; import io.grpc.stub.StreamObserver; import io.numaproj.numaflow.source.v1.SourceOuterClass; import lombok.AllArgsConstructor; @@ -46,6 +47,7 @@ private SourceOuterClass.ReadResponse buildResponse(Message message) { .setPartitionId(message.getOffset().getPartitionId())) .putAllHeaders(message.getHeaders() != null ? message.getHeaders() : new HashMap<>()) + .setMetadata(message.getUserMetadata() == null ? MetadataOuterClass.Metadata.getDefaultInstance() : message.getUserMetadata().toProto()) .build()); return builder.build(); diff --git a/src/main/java/io/numaproj/numaflow/sourcetransformer/Datum.java b/src/main/java/io/numaproj/numaflow/sourcetransformer/Datum.java index 0f2b3953..eb45cdc2 100644 --- a/src/main/java/io/numaproj/numaflow/sourcetransformer/Datum.java +++ b/src/main/java/io/numaproj/numaflow/sourcetransformer/Datum.java @@ -1,6 +1,9 @@ package io.numaproj.numaflow.sourcetransformer; +import io.numaproj.numaflow.shared.SystemMetadata; +import io.numaproj.numaflow.shared.UserMetadata; + import java.time.Instant; import java.util.Map; @@ -36,4 +39,18 @@ public interface Datum { * @return returns the headers in the form of key value pair */ Map getHeaders(); + + /** + * method to get user defined metadata information of the payload + * + * @return user metadata + */ + UserMetadata getUserMetadata(); + + /** + * method to get system metadata information of the payload + * + * @return system metadata + */ + SystemMetadata getSystemMetadata(); } diff --git a/src/main/java/io/numaproj/numaflow/sourcetransformer/HandlerDatum.java b/src/main/java/io/numaproj/numaflow/sourcetransformer/HandlerDatum.java index e3774ed8..d4bbbd2d 100644 --- a/src/main/java/io/numaproj/numaflow/sourcetransformer/HandlerDatum.java +++ b/src/main/java/io/numaproj/numaflow/sourcetransformer/HandlerDatum.java @@ -1,6 +1,8 @@ package io.numaproj.numaflow.sourcetransformer; +import io.numaproj.numaflow.shared.SystemMetadata; +import io.numaproj.numaflow.shared.UserMetadata; import lombok.AllArgsConstructor; import java.time.Instant; @@ -13,6 +15,8 @@ class HandlerDatum implements Datum { private Instant watermark; private Instant eventTime; private Map headers; + private UserMetadata userMetadata; + private SystemMetadata systemMetadata; @Override @@ -34,4 +38,14 @@ public Instant getEventTime() { public Map getHeaders() { return this.headers; } + + @Override + public UserMetadata getUserMetadata() { + return this.userMetadata; + } + + @Override + public SystemMetadata getSystemMetadata() { + return this.systemMetadata; + } } diff --git a/src/main/java/io/numaproj/numaflow/sourcetransformer/Message.java b/src/main/java/io/numaproj/numaflow/sourcetransformer/Message.java index 6aefd0e7..f1c3244d 100644 --- a/src/main/java/io/numaproj/numaflow/sourcetransformer/Message.java +++ b/src/main/java/io/numaproj/numaflow/sourcetransformer/Message.java @@ -1,6 +1,8 @@ package io.numaproj.numaflow.sourcetransformer; import java.time.Instant; + +import io.numaproj.numaflow.shared.UserMetadata; import lombok.Getter; /** Message is used to wrap the data return by SourceTransformer functions. */ @@ -11,22 +13,37 @@ public class Message { private final byte[] value; private final Instant eventTime; private final String[] tags; + private final UserMetadata userMetadata; /** - * used to create Message with value, eventTime, keys and tags(used for conditional forwarding) + * used to create Message with value, eventTime, keys, tags(used for conditional forwarding) and userMetadata * * @param value message value * @param eventTime message eventTime * @param keys message keys * @param tags message tags which will be used for conditional forwarding + * @param userMetadata user metadata */ - public Message(byte[] value, Instant eventTime, String[] keys, String[] tags) { + public Message(byte[] value, Instant eventTime, String[] keys, String[] tags, UserMetadata userMetadata) { // defensive copy - once the Message is created, the caller should not be able to modify it. this.keys = keys == null ? null : keys.clone(); this.value = value == null ? null : value.clone(); this.tags = tags == null ? null : tags.clone(); // The Instant class in Java is already immutable. this.eventTime = eventTime; + this.userMetadata = userMetadata == null ? null : new UserMetadata(userMetadata); + } + + /** + * used to create Message with value, eventTime, keys, tags(used for conditional forwarding) + * + * @param value message value + * @param eventTime message eventTime + * @param keys message keys + * @param tags message tags which will be used for conditional forwarding + */ + public Message(byte[] value, Instant eventTime, String[] keys, String[] tags) { + this(value, eventTime, keys, tags, null); } /** @@ -36,7 +53,7 @@ public Message(byte[] value, Instant eventTime, String[] keys, String[] tags) { * @param eventTime message eventTime */ public Message(byte[] value, Instant eventTime) { - this(value, eventTime, null, null); + this(value, eventTime, null, null, null); } /** @@ -47,7 +64,7 @@ public Message(byte[] value, Instant eventTime) { * @param keys message keys */ public Message(byte[] value, Instant eventTime, String[] keys) { - this(value, eventTime, keys, null); + this(value, eventTime, keys, null, null); } /** @@ -59,6 +76,6 @@ public Message(byte[] value, Instant eventTime, String[] keys) { * @return returns the Message which will be dropped */ public static Message toDrop(Instant eventTime) { - return new Message(new byte[0], eventTime, null, DROP_TAGS); + return new Message(new byte[0], eventTime, null, DROP_TAGS, null); } } diff --git a/src/main/java/io/numaproj/numaflow/sourcetransformer/SourceTransformerTestKit.java b/src/main/java/io/numaproj/numaflow/sourcetransformer/SourceTransformerTestKit.java index bc600a94..1bbf886c 100644 --- a/src/main/java/io/numaproj/numaflow/sourcetransformer/SourceTransformerTestKit.java +++ b/src/main/java/io/numaproj/numaflow/sourcetransformer/SourceTransformerTestKit.java @@ -5,6 +5,8 @@ import io.grpc.ManagedChannel; import io.grpc.ManagedChannelBuilder; import io.grpc.stub.StreamObserver; +import io.numaproj.numaflow.shared.SystemMetadata; +import io.numaproj.numaflow.shared.UserMetadata; import io.numaproj.numaflow.sourcetransformer.v1.SourceTransformGrpc; import io.numaproj.numaflow.sourcetransformer.v1.Sourcetransformer; import lombok.Builder; @@ -233,5 +235,7 @@ public static class TestDatum implements Datum { private final Instant eventTime; private final Instant watermark; private final Map headers; + private final UserMetadata userMetadata; + private final SystemMetadata systemMetadata; } } diff --git a/src/main/java/io/numaproj/numaflow/sourcetransformer/TransformerActor.java b/src/main/java/io/numaproj/numaflow/sourcetransformer/TransformerActor.java index 08f67a06..4fd9a7ae 100644 --- a/src/main/java/io/numaproj/numaflow/sourcetransformer/TransformerActor.java +++ b/src/main/java/io/numaproj/numaflow/sourcetransformer/TransformerActor.java @@ -5,6 +5,9 @@ import akka.japi.pf.ReceiveBuilder; import com.google.protobuf.ByteString; import com.google.protobuf.Timestamp; +import common.MetadataOuterClass; +import io.numaproj.numaflow.shared.SystemMetadata; +import io.numaproj.numaflow.shared.UserMetadata; import io.numaproj.numaflow.sourcetransformer.v1.Sourcetransformer; import java.time.Instant; @@ -67,7 +70,9 @@ private void processRequest(Sourcetransformer.SourceTransformRequest transformRe Instant.ofEpochSecond( request.getEventTime().getSeconds(), request.getEventTime().getNanos()), - request.getHeadersMap() + request.getHeadersMap(), + new UserMetadata(request.getMetadata()), + new SystemMetadata(request.getMetadata()) ); String[] keys = request.getKeysList().toArray(new String[0]); try { @@ -117,6 +122,9 @@ private Sourcetransformer.SourceTransformResponse buildResponse( == null ? new ArrayList<>() : Arrays.asList(message.getKeys())) .addAllTags(message.getTags() == null ? new ArrayList<>() : Arrays.asList(message.getTags())) + .setMetadata(message.getUserMetadata() + == null ? MetadataOuterClass.Metadata.getDefaultInstance() + : message.getUserMetadata().toProto()) .build()); }); return responseBuilder.setId(ID).build(); diff --git a/src/main/proto/map/v1/map.proto b/src/main/proto/map/v1/map.proto index 82636e3f..e72acf3e 100644 --- a/src/main/proto/map/v1/map.proto +++ b/src/main/proto/map/v1/map.proto @@ -4,6 +4,7 @@ option java_package = "io.numaproj.numaflow.map.v1"; import "google/protobuf/timestamp.proto"; import "google/protobuf/empty.proto"; +import "common/metadata.proto"; package map.v1; @@ -25,6 +26,7 @@ message MapRequest { google.protobuf.Timestamp event_time = 3; google.protobuf.Timestamp watermark = 4; map headers = 5; + common.Metadata metadata = 6; } Request request = 1; // This ID is used to uniquely identify a map request @@ -56,6 +58,7 @@ message MapResponse { repeated string keys = 1; bytes value = 2; repeated string tags = 3; + common.Metadata metadata = 4; } repeated Result results = 1; // This ID is used to refer the responses to the request it corresponds to. diff --git a/src/main/proto/sink/v1/sink.proto b/src/main/proto/sink/v1/sink.proto index b1484e4a..4fb516b4 100644 --- a/src/main/proto/sink/v1/sink.proto +++ b/src/main/proto/sink/v1/sink.proto @@ -27,6 +27,7 @@ message SinkRequest { google.protobuf.Timestamp watermark = 4; string id = 5; map headers = 6; + common.Metadata metadata = 7; } // Required field indicating the request. Request request = 1; diff --git a/src/main/proto/source/v1/source.proto b/src/main/proto/source/v1/source.proto index a1fbdfeb..2de4add8 100644 --- a/src/main/proto/source/v1/source.proto +++ b/src/main/proto/source/v1/source.proto @@ -4,6 +4,7 @@ option java_package = "io.numaproj.numaflow.source.v1"; import "google/protobuf/timestamp.proto"; import "google/protobuf/empty.proto"; +import "common/metadata.proto"; package source.v1; @@ -82,6 +83,8 @@ message ReadResponse { // Headers are the metadata associated with the datum. // e.g. Kafka and Redis Stream message usually include information about the headers. map headers = 5; + // Metadata is the metadata of the message + common.Metadata metadata = 6; } message Status { // Code to indicate the status of the response. diff --git a/src/main/proto/sourcetransform/v1/sourcetransformer.proto b/src/main/proto/sourcetransform/v1/sourcetransformer.proto index 45b7022f..dda7cc6a 100644 --- a/src/main/proto/sourcetransform/v1/sourcetransformer.proto +++ b/src/main/proto/sourcetransform/v1/sourcetransformer.proto @@ -4,6 +4,7 @@ option java_package = "io.numaproj.numaflow.sourcetransformer.v1"; import "google/protobuf/timestamp.proto"; import "google/protobuf/empty.proto"; +import "common/metadata.proto"; package sourcetransformer.v1; @@ -37,6 +38,7 @@ message SourceTransformRequest { map headers = 5; // This ID is used to uniquely identify a transform request string id = 6; + common.Metadata metadata = 7; } Request request = 1; optional Handshake handshake = 2; @@ -51,6 +53,7 @@ message SourceTransformResponse { bytes value = 2; google.protobuf.Timestamp event_time = 3; repeated string tags = 4; + common.Metadata metadata = 5; } repeated Result results = 1; // This ID is used to refer the responses to the request it corresponds to. diff --git a/src/test/java/io/numaproj/numaflow/mapper/HandlerDatumTest.java b/src/test/java/io/numaproj/numaflow/mapper/HandlerDatumTest.java index 9b15bcce..6ce76eed 100644 --- a/src/test/java/io/numaproj/numaflow/mapper/HandlerDatumTest.java +++ b/src/test/java/io/numaproj/numaflow/mapper/HandlerDatumTest.java @@ -1,5 +1,7 @@ package io.numaproj.numaflow.mapper; +import io.numaproj.numaflow.shared.SystemMetadata; +import io.numaproj.numaflow.shared.UserMetadata; import org.junit.Test; import java.time.Instant; @@ -14,7 +16,7 @@ public void testHandlerDatum() { Instant eventTime = Instant.now(); HashMap headers = new HashMap<>(); headers.put("header1", "value1"); - HandlerDatum datum = new HandlerDatum("asdf".getBytes(), watermark, eventTime, headers); + HandlerDatum datum = new HandlerDatum("asdf".getBytes(), watermark, eventTime, headers, new UserMetadata(), new SystemMetadata()); assertEquals(watermark, datum.getWatermark()); assertEquals(eventTime, datum.getEventTime()); assertEquals(headers, datum.getHeaders()); diff --git a/src/test/java/io/numaproj/numaflow/sinker/ResponseTest.java b/src/test/java/io/numaproj/numaflow/sinker/ResponseTest.java index 841cfb85..0873bf6c 100644 --- a/src/test/java/io/numaproj/numaflow/sinker/ResponseTest.java +++ b/src/test/java/io/numaproj/numaflow/sinker/ResponseTest.java @@ -2,6 +2,7 @@ import com.google.protobuf.ByteString; import common.MetadataOuterClass; +import io.numaproj.numaflow.shared.UserMetadata; import io.numaproj.numaflow.sink.v1.SinkOuterClass; import org.junit.Test; @@ -25,16 +26,16 @@ public void test_addResponse() { Response response4 = Response.responseFailure(defaultId, "failure"); assertEquals(defaultId, response4.getId()); - HashMap userMetadata = new HashMap<>(); - userMetadata.put("group1", KeyValueGroup.builder().build()); + Map> userMetadata = new HashMap<>(); + userMetadata.put("group1", new HashMap<>()); HashMap kvg1 = new HashMap<>(Map.ofEntries( entry("key1", "val1".getBytes()) )); kvg1.put("key2", null); - userMetadata.put("group2", KeyValueGroup.builder().keyValue(kvg1).build()); + userMetadata.put("group2", kvg1); userMetadata.put("group3", null); - Message onSuccessMessage1 = new Message("onSuccessValue".getBytes(), null, userMetadata); + Message onSuccessMessage1 = new Message("onSuccessValue".getBytes(), null, new UserMetadata(userMetadata)); Response response5 = Response.responseOnSuccess(defaultId, onSuccessMessage1); assertEquals(defaultId, response5.getId()); From 73a7cf150a251076c14666ab2ae32ba8d395deeb Mon Sep 17 00:00:00 2001 From: vtiwari5 Date: Mon, 26 Jan 2026 11:00:11 -0500 Subject: [PATCH 2/8] Tests for UserMetadata and SystemMetadata classes Signed-off-by: vtiwari5 --- .../numaflow/shared/SystemMetadata.java | 55 +- .../numaflow/shared/UserMetadata.java | 117 ++-- .../numaflow/shared/SystemMetadataTest.java | 284 ++++++++++ .../numaflow/shared/UserMetadataTest.java | 510 ++++++++++++++++++ 4 files changed, 923 insertions(+), 43 deletions(-) create mode 100644 src/test/java/io/numaproj/numaflow/shared/SystemMetadataTest.java create mode 100644 src/test/java/io/numaproj/numaflow/shared/UserMetadataTest.java diff --git a/src/main/java/io/numaproj/numaflow/shared/SystemMetadata.java b/src/main/java/io/numaproj/numaflow/shared/SystemMetadata.java index a69a2139..b2857dbc 100644 --- a/src/main/java/io/numaproj/numaflow/shared/SystemMetadata.java +++ b/src/main/java/io/numaproj/numaflow/shared/SystemMetadata.java @@ -7,7 +7,6 @@ import java.util.stream.Collectors; import common.MetadataOuterClass; -import lombok.AllArgsConstructor; import lombok.Getter; /** @@ -16,7 +15,6 @@ * It is read-only to UDFs */ @Getter -@AllArgsConstructor public class SystemMetadata { private final Map> data; @@ -27,6 +25,32 @@ public SystemMetadata() { this.data = new HashMap<>(); } + /** + * An all args constructor that filters out null values and copies the values to prevent mutation. + * If the data is null or empty, it initializes an empty HashMap data. + * For each entry in {@code data}, it creates a new HashMap and copies the key-value pairs + * from the entry to the new HashMap. It also filters out any null values. + * Empty hashmap values are allowed as entries in the data map. + * + * @param data is a map of group name to key-value pairs + */ + public SystemMetadata(Map> data) { + if (data == null || data.isEmpty()) { + this.data = new HashMap<>(); + return; + } + this.data = data.entrySet().stream() + .filter(e -> e.getValue() != null) + .collect(Collectors.toMap(Map.Entry::getKey, + entry -> entry.getValue().entrySet().stream() + .filter(e1 -> e1.getValue() != null) + .collect(Collectors.toMap( + Map.Entry::getKey, + e1-> e1.getValue().clone() + )) + )); + } + /** * Constructor from MetadataOuterClass.Metadata * @@ -52,15 +76,31 @@ public SystemMetadata(MetadataOuterClass.Metadata metadata) { )); } + /** + * Get the data as a map. + * Returns a deep copy of the data to prevent mutation. + * + * @return a deep copy of the data + */ + public Map> getData() { + // Deep copy the data to prevent mutation + return this.data.entrySet().stream() + // No null checks required as the constructor ensures that the data is valid + .collect(Collectors.toMap(Map.Entry::getKey, + entry -> entry.getValue().entrySet().stream() + .collect(Collectors.toMap( + Map.Entry::getKey, + e1-> e1.getValue().clone() + )) + )); + } + /** * Get the list of all groups present in the user metadata * * @return list of group names */ public List getGroups() { - if (this.data == null) { - return new ArrayList<>(); - } return new ArrayList<>(this.data.keySet()); } @@ -71,7 +111,7 @@ public List getGroups() { * @return a list of key names within the group */ public List getKeys(String group) { - if (this.data == null || !this.data.containsKey(group)) { + if (!this.data.containsKey(group)) { return new ArrayList<>(); } return new ArrayList<>(this.data.get(group).keySet()); @@ -85,9 +125,6 @@ public List getKeys(String group) { * @return Value of the key in the group or null if the group/key is not present */ public byte[] getValue(String group, String key) { - if (this.data == null) { - return null; - } Map groupData = this.data.get(group); if (groupData == null) { return null; diff --git a/src/main/java/io/numaproj/numaflow/shared/UserMetadata.java b/src/main/java/io/numaproj/numaflow/shared/UserMetadata.java index c64c8592..9ecf1a51 100644 --- a/src/main/java/io/numaproj/numaflow/shared/UserMetadata.java +++ b/src/main/java/io/numaproj/numaflow/shared/UserMetadata.java @@ -1,7 +1,6 @@ package io.numaproj.numaflow.shared; import common.MetadataOuterClass; -import lombok.AllArgsConstructor; import lombok.Getter; import com.google.protobuf.ByteString; @@ -15,23 +14,52 @@ * UserMetadata is mapping of group name to key-value pairs * UserMetadata wraps user generated metadata groups per message. * It can be appended to and passed on to the downstream. - * {@link lombok.AllArgsConstructor} allows creation of UserMetadata from a `Map>` - * Copy constructor allows creation of UserMetadata from another UserMetadata + * Note: All the null checks have been added within the constructors to ensure + * that the data and its entries are always valid. */ -@Getter -@AllArgsConstructor public class UserMetadata { private final Map> data; /** - * Default constructor + * Default constructor for UserMetadata. + * Initializes an empty HashMap data. */ public UserMetadata() { this.data = new HashMap<>(); } + + /** + * An all args constructor that filters out null values and copies the values to prevent mutation. + * If the data is null or empty, it initializes an empty HashMap data. + * For each entry in {@code data}, it creates a new HashMap and copies the key-value pairs + * from the entry to the new HashMap. It also filters out any null values. + * Empty hashmap values are allowed as entries in the data map. + * + * @param data is a map of group name to key-value pairs + */ + public UserMetadata(Map> data) { + if (data == null || data.isEmpty()) { + this.data = new HashMap<>(); + return; + } + this.data = data.entrySet().stream() + .filter(e -> e.getValue() != null) + .collect(Collectors.toMap(Map.Entry::getKey, + entry -> entry.getValue().entrySet().stream() + .filter(e1 -> e1.getValue() != null) + .collect(Collectors.toMap( + Map.Entry::getKey, + e1-> e1.getValue().clone() + )) + )); + } + /** - * Constructor from MetadataOuterClass.Metadata + * Constructor from MetadataOuterClass.Metadata. + * Initializes an empty HashMap data if the metadata passed is null or empty. + * For each entry in {@code metadata.getUserMetadataMap()}, it creates a new HashMap and copies the key-value pairs + * from the entry to the new HashMap. It also filters out any null values. * * @param metadata is an instance of MetadataOuterClass.Metadata which contains user metadata */ @@ -46,17 +74,23 @@ public UserMetadata(MetadataOuterClass.Metadata metadata) { .stream() .collect(Collectors.toMap( Map.Entry::getKey, - e -> new HashMap<>(e.getValue().getKeyValueMap().entrySet().stream() + e -> e.getValue() + .getKeyValueMap().entrySet().stream() + .filter(e1 -> e1.getValue() != null) .collect(Collectors.toMap( Map.Entry::getKey, e1 -> e1.getValue().toByteArray() )) ) - )); + ); } /** - * Copy constructor + * Copy constructor for UserMetadata. + * Returns a UserMetadata with empty HashMap data if the userMetadata passed is null or + * {@code userMetadata.data} is null. + * For each entry in {@code userMetadata.data}, it creates a new HashMap and copies the key-value pairs + * from the entry to the new HashMap. It also filters out any null values. * * @param userMetadata the user metadata to copy */ @@ -83,14 +117,13 @@ public UserMetadata(UserMetadata userMetadata) { /** * Convert the user metadata to a map that can be used to create MetadataOuterClass.Metadata + * For each entry in {@code data}, it creates a new MetadataOuterClass.KeyValueGroup and copies the key-value pairs + * from the entry to the new MetadataOuterClass.KeyValueGroup. + * It also filters out any null values. * * @return MetadataOuterClass.Metadata */ public MetadataOuterClass.Metadata toProto() { - if (this.data == null) { - return MetadataOuterClass.Metadata.getDefaultInstance(); - } - Map result = new HashMap<>(); this.data.forEach((group, kvMap) -> { MetadataOuterClass.KeyValueGroup.Builder builder = MetadataOuterClass.KeyValueGroup.newBuilder(); @@ -112,15 +145,30 @@ public MetadataOuterClass.Metadata toProto() { .build(); } + /** + * Get the data as a map. + * Returns a deep copy of the data to prevent mutation. + * + * @return a deep copy of the data + */ + public Map> getData() { + // Deep copy the data to prevent mutation + return this.data.entrySet().stream() + .collect(Collectors.toMap(Map.Entry::getKey, + entry -> entry.getValue().entrySet().stream() + .collect(Collectors.toMap( + Map.Entry::getKey, + e1-> e1.getValue().clone() + )) + )); + } + /** * Get the list of all groups present in the user metadata * * @return list of group names */ public List getGroups() { - if (this.data == null) { - return new ArrayList<>(); - } return new ArrayList<>(this.data.keySet()); } @@ -130,9 +178,6 @@ public List getGroups() { * @param group is the name of the group to delete */ public void deleteGroup(String group) { - if (this.data == null) { - return; - } this.data.remove(group); } @@ -143,7 +188,7 @@ public void deleteGroup(String group) { * @return a list of key names within the group */ public List getKeys(String group) { - if (this.data == null || !this.data.containsKey(group)) { + if (!this.data.containsKey(group)) { return new ArrayList<>(); } return new ArrayList<>(this.data.get(group).keySet()); @@ -156,7 +201,7 @@ public List getKeys(String group) { * @param key Name of the key to delete */ public void deleteKey(String group, String key) { - if (this.data == null || !this.data.containsKey(group)) { + if (!this.data.containsKey(group)) { return; } this.data.get(group).remove(key); @@ -164,31 +209,40 @@ public void deleteKey(String group, String key) { /** * Add a key value pair to a group + * Note: If the value is null, the key/value pair will not be added to the group * * @param group Name of the group to which key value pairs are to be added * @param key Name of the key in group to which the value is to be added * @param value Value to be added to the key */ public void addKV(String group, String key, byte[] value) { - if (this.data == null) { - return; + // null values are not added + if (group != null && key != null && value != null) { + this.data + .computeIfAbsent(group, k -> new HashMap<>()) + .put(key, value.clone()); } - this.data.computeIfAbsent(group, k -> new HashMap<>()).put(key, value.clone()); } /** * Add multiple key value pairs to a group + * Note: If the value is null, the key/value pair will not be added to the group * * @param group Name of the group to which key value pairs are to be added * @param kv Map of key value pairs to be added to the group */ public void addKVs(String group, Map kv) { - if (this.data == null) { + if (group == null || kv == null) { return; } Map groupMap = this.data.computeIfAbsent(group, k -> new HashMap<>()); - // Copy the values to prevent mutation - kv.forEach((key, value) -> groupMap.put(key, value.clone())); + kv.forEach((key, value) -> { + // null values are not added + if (value != null) { + // clone the value to prevent mutation + groupMap.put(key, value.clone()); + } + }); } /** @@ -199,14 +253,12 @@ public void addKVs(String group, Map kv) { * @return Value of the key in the group or null if the group/key is not present */ public byte[] getValue(String group, String key) { - if (this.data == null) { - return null; - } Map groupData = this.data.get(group); if (groupData == null) { return null; } byte[] value = groupData.get(key); + // null value should not be present but check added for safety return value == null ? null : value.clone(); } @@ -214,9 +266,6 @@ public byte[] getValue(String group, String key) { * Clear all the user metadata */ public void clear() { - if (this.data == null) { - return; - } this.data.clear(); } } diff --git a/src/test/java/io/numaproj/numaflow/shared/SystemMetadataTest.java b/src/test/java/io/numaproj/numaflow/shared/SystemMetadataTest.java new file mode 100644 index 00000000..b880319b --- /dev/null +++ b/src/test/java/io/numaproj/numaflow/shared/SystemMetadataTest.java @@ -0,0 +1,284 @@ +package io.numaproj.numaflow.shared; + +import com.google.protobuf.ByteString; +import common.MetadataOuterClass; +import org.junit.Test; + +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import static org.junit.Assert.*; + +public class SystemMetadataTest { + + @Test + public void testDefaultConstructor() { + SystemMetadata metadata = new SystemMetadata(); + assertNotNull(metadata.getData()); + assertTrue(metadata.getData().isEmpty()); + } + + @Test + public void testMapConstructor_withValidData() { + Map> data = new HashMap<>(); + Map group1 = new HashMap<>(); + group1.put("key1", "value1".getBytes()); + Map group2 = new HashMap<>(); + group2.put("keyA", "valueA".getBytes()); + data.put("group1", group1); + data.put("group2", group2); + + SystemMetadata metadata = new SystemMetadata(data); + + assertEquals(2, metadata.getData().size()); + assertArrayEquals("value1".getBytes(), metadata.getData().get("group1").get("key1")); + assertArrayEquals("valueA".getBytes(), metadata.getData().get("group2").get("keyA")); + } + + @Test + public void testMapConstructor_withNullData() { + SystemMetadata metadata = new SystemMetadata((Map>) null); + assertNotNull(metadata.getData()); + assertTrue(metadata.getData().isEmpty()); + } + + @Test + public void testMapConstructor_withEmptyData() { + Map> data = new HashMap<>(); + SystemMetadata metadata = new SystemMetadata(data); + assertNotNull(metadata.getData()); + assertTrue(metadata.getData().isEmpty()); + } + + @Test + public void testMapConstructor_filtersNullValues() { + Map> data = new HashMap<>(); + Map group1 = new HashMap<>(); + group1.put("key1", "value1".getBytes()); + group1.put("key2", null); + data.put("group1", group1); + data.put("group2", null); + + SystemMetadata metadata = new SystemMetadata(data); + + assertEquals(1, metadata.getData().size()); + assertEquals(1, metadata.getData().get("group1").size()); + assertArrayEquals("value1".getBytes(), metadata.getData().get("group1").get("key1")); + assertNull(metadata.getData().get("group1").get("key2")); + } + + @Test + public void testMapConstructor_preventsMutation() { + Map> data = new HashMap<>(); + Map group1 = new HashMap<>(); + byte[] originalValue = "value1".getBytes(); + group1.put("key1", originalValue); + data.put("group1", group1); + + SystemMetadata metadata = new SystemMetadata(data); + + // Modify original byte array + originalValue[0] = 'X'; + + // Verify metadata is not affected + assertArrayEquals("value1".getBytes(), metadata.getData().get("group1").get("key1")); + } + + @Test + public void testProtoConstructor_withValidMetadata() { + MetadataOuterClass.KeyValueGroup kvGroup1 = MetadataOuterClass.KeyValueGroup.newBuilder() + .putKeyValue("key1", ByteString.copyFromUtf8("value1")) + .build(); + + MetadataOuterClass.KeyValueGroup kvGroup2 = MetadataOuterClass.KeyValueGroup.newBuilder() + .putKeyValue("keyA", ByteString.copyFromUtf8("valueA")) + .build(); + + MetadataOuterClass.Metadata protoMetadata = MetadataOuterClass.Metadata.newBuilder() + .putSysMetadata("group1", kvGroup1) + .putSysMetadata("group2", kvGroup2) + .build(); + + SystemMetadata metadata = new SystemMetadata(protoMetadata); + + assertEquals(2, metadata.getData().size()); + assertArrayEquals("value1".getBytes(), metadata.getData().get("group1").get("key1")); + assertArrayEquals("valueA".getBytes(), metadata.getData().get("group2").get("keyA")); + } + + @Test + public void testProtoConstructor_withNullMetadata() { + SystemMetadata metadata = new SystemMetadata((MetadataOuterClass.Metadata) null); + assertNotNull(metadata.getData()); + assertTrue(metadata.getData().isEmpty()); + } + + @Test + public void testProtoConstructor_withEmptyMetadata() { + MetadataOuterClass.Metadata protoMetadata = MetadataOuterClass.Metadata.newBuilder().build(); + SystemMetadata metadata = new SystemMetadata(protoMetadata); + assertNotNull(metadata.getData()); + assertTrue(metadata.getData().isEmpty()); + } + + @Test + public void testGetData_returnsDeepCopy() { + Map> inputData = new HashMap<>(); + Map group1 = new HashMap<>(); + group1.put("key1", "value1".getBytes()); + inputData.put("group1", group1); + + SystemMetadata metadata = new SystemMetadata(inputData); + Map> dataCopy = metadata.getData(); + + // Modify the returned copy + dataCopy.put("group2", new HashMap<>()); + dataCopy.get("group1").put("key2", "value2".getBytes()); + dataCopy.get("group1").get("key1")[0] = 'X'; + + // Verify original metadata is not affected + assertEquals(1, metadata.getGroups().size()); + assertEquals(1, metadata.getKeys("group1").size()); + assertArrayEquals("value1".getBytes(), metadata.getValue("group1", "key1")); + } + + @Test + public void testGetData_emptyMetadata() { + SystemMetadata metadata = new SystemMetadata(); + Map> data = metadata.getData(); + + assertNotNull(data); + assertTrue(data.isEmpty()); + } + + @Test + public void testGetGroups() { + Map> data = new HashMap<>(); + data.put("group1", new HashMap<>()); + data.put("group2", new HashMap<>()); + data.put("group3", new HashMap<>()); + + SystemMetadata metadata = new SystemMetadata(data); + List groups = metadata.getGroups(); + + assertNotNull(groups); + assertEquals(3, groups.size()); + assertTrue(groups.contains("group1")); + assertTrue(groups.contains("group2")); + assertTrue(groups.contains("group3")); + } + + @Test + public void testGetGroups_emptyMetadata() { + SystemMetadata metadata = new SystemMetadata(); + List groups = metadata.getGroups(); + + assertNotNull(groups); + assertTrue(groups.isEmpty()); + } + + @Test + public void testGetKeys() { + Map> data = new HashMap<>(); + Map group1 = new HashMap<>(); + group1.put("key1", "value1".getBytes()); + group1.put("key2", "value2".getBytes()); + group1.put("key3", "value3".getBytes()); + data.put("group1", group1); + + SystemMetadata metadata = new SystemMetadata(data); + List keys = metadata.getKeys("group1"); + + assertNotNull(keys); + assertEquals(3, keys.size()); + assertTrue(keys.contains("key1")); + assertTrue(keys.contains("key2")); + assertTrue(keys.contains("key3")); + } + + @Test + public void testGetKeys_nonExistentGroup() { + SystemMetadata metadata = new SystemMetadata(); + List keys = metadata.getKeys("nonExistent"); + + assertNotNull(keys); + assertTrue(keys.isEmpty()); + } + + @Test + public void testGetKeys_emptyGroup() { + Map> data = new HashMap<>(); + data.put("emptyGroup", new HashMap<>()); + + SystemMetadata metadata = new SystemMetadata(data); + List keys = metadata.getKeys("emptyGroup"); + + assertNotNull(keys); + assertTrue(keys.isEmpty()); + } + + @Test + public void testGetValue() { + Map> data = new HashMap<>(); + Map group1 = new HashMap<>(); + group1.put("key1", "value1".getBytes()); + data.put("group1", group1); + + SystemMetadata metadata = new SystemMetadata(data); + byte[] value = metadata.getValue("group1", "key1"); + + assertNotNull(value); + assertArrayEquals("value1".getBytes(), value); + } + + @Test + public void testGetValue_nonExistentGroup() { + SystemMetadata metadata = new SystemMetadata(); + byte[] value = metadata.getValue("nonExistent", "key1"); + + assertNull(value); + } + + @Test + public void testGetValue_nonExistentKey() { + Map> data = new HashMap<>(); + Map group1 = new HashMap<>(); + group1.put("key1", "value1".getBytes()); + data.put("group1", group1); + + SystemMetadata metadata = new SystemMetadata(data); + byte[] value = metadata.getValue("group1", "nonExistent"); + + assertNull(value); + } + + @Test + public void testGetValue_returnsClone() { + Map> data = new HashMap<>(); + Map group1 = new HashMap<>(); + group1.put("key1", "value1".getBytes()); + data.put("group1", group1); + + SystemMetadata metadata = new SystemMetadata(data); + byte[] value = metadata.getValue("group1", "key1"); + value[0] = 'X'; + + // Verify internal data is not affected + assertArrayEquals("value1".getBytes(), metadata.getValue("group1", "key1")); + } + + @Test + public void testGetValue_withEmptyByteArray() { + Map> data = new HashMap<>(); + Map group1 = new HashMap<>(); + group1.put("emptyKey", new byte[0]); + data.put("group1", group1); + + SystemMetadata metadata = new SystemMetadata(data); + byte[] value = metadata.getValue("group1", "emptyKey"); + + assertNotNull(value); + assertEquals(0, value.length); + } +} diff --git a/src/test/java/io/numaproj/numaflow/shared/UserMetadataTest.java b/src/test/java/io/numaproj/numaflow/shared/UserMetadataTest.java new file mode 100644 index 00000000..46c22408 --- /dev/null +++ b/src/test/java/io/numaproj/numaflow/shared/UserMetadataTest.java @@ -0,0 +1,510 @@ +package io.numaproj.numaflow.shared; + +import com.google.protobuf.ByteString; +import common.MetadataOuterClass; +import org.junit.Test; + +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import static org.junit.Assert.*; + +public class UserMetadataTest { + + @Test + public void testDefaultConstructor() { + UserMetadata metadata = new UserMetadata(); + assertNotNull(metadata.getData()); + assertTrue(metadata.getData().isEmpty()); + } + + @Test + public void testMapConstructor_withValidData() { + Map> data = new HashMap<>(); + Map group1 = new HashMap<>(); + group1.put("key1", "value1".getBytes()); + group1.put("key2", "value2".getBytes()); + data.put("group1", group1); + + UserMetadata metadata = new UserMetadata(data); + + assertNotNull(metadata.getData()); + assertEquals(1, metadata.getData().size()); + assertTrue(metadata.getData().containsKey("group1")); + assertArrayEquals("value1".getBytes(), metadata.getData().get("group1").get("key1")); + assertArrayEquals("value2".getBytes(), metadata.getData().get("group1").get("key2")); + } + + @Test + public void testMapConstructor_withNullData() { + UserMetadata metadata = new UserMetadata((Map>) null); + assertNotNull(metadata.getData()); + assertTrue(metadata.getData().isEmpty()); + } + + @Test + public void testMapConstructor_withEmptyData() { + Map> data = new HashMap<>(); + UserMetadata metadata = new UserMetadata(data); + assertNotNull(metadata.getData()); + assertTrue(metadata.getData().isEmpty()); + } + + @Test + public void testMapConstructor_filtersNullValues() { + Map> data = new HashMap<>(); + Map group1 = new HashMap<>(); + group1.put("key1", "value1".getBytes()); + group1.put("key2", null); + data.put("group1", group1); + data.put("group2", null); + + UserMetadata metadata = new UserMetadata(data); + + assertEquals(1, metadata.getData().size()); + assertEquals(1, metadata.getData().get("group1").size()); + assertArrayEquals("value1".getBytes(), metadata.getData().get("group1").get("key1")); + assertNull(metadata.getData().get("group1").get("key2")); + } + + @Test + public void testMapConstructor_preventsMutation() { + Map> data = new HashMap<>(); + Map group1 = new HashMap<>(); + byte[] originalValue = "value1".getBytes(); + group1.put("key1", originalValue); + data.put("group1", group1); + + UserMetadata metadata = new UserMetadata(data); + + // Modify original byte array + originalValue[0] = 'X'; + + // Verify metadata is not affected + assertArrayEquals("value1".getBytes(), metadata.getData().get("group1").get("key1")); + } + + @Test + public void testProtoConstructor_withValidMetadata() { + MetadataOuterClass.KeyValueGroup kvGroup = MetadataOuterClass.KeyValueGroup.newBuilder() + .putKeyValue("key1", ByteString.copyFromUtf8("value1")) + .putKeyValue("key2", ByteString.copyFromUtf8("value2")) + .build(); + + MetadataOuterClass.Metadata protoMetadata = MetadataOuterClass.Metadata.newBuilder() + .putUserMetadata("group1", kvGroup) + .build(); + + UserMetadata metadata = new UserMetadata(protoMetadata); + + assertNotNull(metadata.getData()); + assertEquals(1, metadata.getData().size()); + assertTrue(metadata.getData().containsKey("group1")); + assertArrayEquals("value1".getBytes(), metadata.getData().get("group1").get("key1")); + assertArrayEquals("value2".getBytes(), metadata.getData().get("group1").get("key2")); + } + + @Test + public void testProtoConstructor_withNullMetadata() { + UserMetadata metadata = new UserMetadata((MetadataOuterClass.Metadata) null); + assertNotNull(metadata.getData()); + assertTrue(metadata.getData().isEmpty()); + } + + @Test + public void testProtoConstructor_withEmptyMetadata() { + MetadataOuterClass.Metadata protoMetadata = MetadataOuterClass.Metadata.newBuilder().build(); + UserMetadata metadata = new UserMetadata(protoMetadata); + assertNotNull(metadata.getData()); + assertTrue(metadata.getData().isEmpty()); + } + + @Test + public void testCopyConstructor_withValidUserMetadata() { + UserMetadata original = new UserMetadata(); + original.addKV("group1", "key1", "value1".getBytes()); + original.addKV("group1", "key2", "value2".getBytes()); + original.addKV("group2", "keyA", "valueA".getBytes()); + original.addKV("group2", "keyB", null); + + UserMetadata copy = new UserMetadata(original); + + assertNotNull(copy.getData()); + assertEquals(2, copy.getData().size()); + assertArrayEquals("value1".getBytes(), copy.getData().get("group1").get("key1")); + assertArrayEquals("value2".getBytes(), copy.getData().get("group1").get("key2")); + assertArrayEquals("valueA".getBytes(), copy.getData().get("group2").get("keyA")); + assertNull(copy.getData().get("group2").get("keyB")); + } + + @Test + public void testCopyConstructor_withNullUserMetadata() { + UserMetadata metadata = new UserMetadata((UserMetadata) null); + assertNotNull(metadata.getData()); + assertTrue(metadata.getData().isEmpty()); + } + + @Test + public void testCopyConstructor_preventsMutation() { + UserMetadata original = new UserMetadata(); + byte[] originalValue = "value1".getBytes(); + original.addKV("group1", "key1", originalValue); + + UserMetadata copy = new UserMetadata(original); + + // Modify original + original.addKV("group1", "key2", "value2".getBytes()); + + // Verify copy is not affected + assertEquals(1, copy.getData().get("group1").size()); + assertNull(copy.getData().get("group1").get("key2")); + } + + @Test + public void testToProto() { + UserMetadata metadata = new UserMetadata(); + metadata.addKV("group1", "key1", "value1".getBytes()); + metadata.addKV("group1", "key2", "value2".getBytes()); + metadata.addKV("group2", "keyA", "valueA".getBytes()); + metadata.addKV("group2", "keyB", null); + + MetadataOuterClass.Metadata proto = metadata.toProto(); + + assertNotNull(proto); + assertEquals(2, proto.getUserMetadataMap().size()); + assertTrue(proto.getUserMetadataMap().containsKey("group1")); + assertTrue(proto.getUserMetadataMap().containsKey("group2")); + + MetadataOuterClass.KeyValueGroup group1 = proto.getUserMetadataMap().get("group1"); + assertEquals(ByteString.copyFromUtf8("value1"), group1.getKeyValueMap().get("key1")); + assertEquals(ByteString.copyFromUtf8("value2"), group1.getKeyValueMap().get("key2")); + + MetadataOuterClass.KeyValueGroup group2 = proto.getUserMetadataMap().get("group2"); + assertEquals(ByteString.copyFromUtf8("valueA"), group2.getKeyValueMap().get("keyA")); + assertNull(group2.getKeyValueMap().get("keyB")); + } + + @Test + public void testToProto_emptyMetadata() { + UserMetadata metadata = new UserMetadata(); + MetadataOuterClass.Metadata proto = metadata.toProto(); + + assertNotNull(proto); + assertTrue(proto.getUserMetadataMap().isEmpty()); + } + + @Test + public void testGetData_returnsDeepCopy() { + UserMetadata metadata = new UserMetadata(); + metadata.addKV("group1", "key1", "value1".getBytes()); + + Map> dataCopy = metadata.getData(); + + // Modify the returned copy + dataCopy.put("group2", new HashMap<>()); + dataCopy.get("group1").put("key2", "value2".getBytes()); + dataCopy.get("group1").get("key1")[0] = 'X'; + + // Verify original metadata is not affected + assertEquals(1, metadata.getGroups().size()); + assertEquals(1, metadata.getKeys("group1").size()); + assertArrayEquals("value1".getBytes(), metadata.getValue("group1", "key1")); + } + + @Test + public void testGetData_emptyMetadata() { + UserMetadata metadata = new UserMetadata(); + Map> data = metadata.getData(); + + assertNotNull(data); + assertTrue(data.isEmpty()); + } + + @Test + public void testGetGroups() { + UserMetadata metadata = new UserMetadata(); + metadata.addKV("group1", "key1", "value1".getBytes()); + metadata.addKV("group2", "key2", "value2".getBytes()); + metadata.addKV("group3", "key3", "value3".getBytes()); + + List groups = metadata.getGroups(); + + assertNotNull(groups); + assertEquals(3, groups.size()); + assertTrue(groups.contains("group1")); + assertTrue(groups.contains("group2")); + assertTrue(groups.contains("group3")); + } + + @Test + public void testGetGroups_emptyMetadata() { + UserMetadata metadata = new UserMetadata(); + List groups = metadata.getGroups(); + + assertNotNull(groups); + assertTrue(groups.isEmpty()); + } + + @Test + public void testDeleteGroup() { + UserMetadata metadata = new UserMetadata(); + metadata.addKV("group1", "key1", "value1".getBytes()); + metadata.addKV("group2", "key2", "value2".getBytes()); + + metadata.deleteGroup("group1"); + + assertEquals(1, metadata.getData().size()); + assertFalse(metadata.getData().containsKey("group1")); + assertTrue(metadata.getData().containsKey("group2")); + } + + @Test + public void testDeleteGroup_nonExistentGroup() { + UserMetadata metadata = new UserMetadata(); + metadata.addKV("group1", "key1", "value1".getBytes()); + + // Should not throw exception + metadata.deleteGroup("nonExistent"); + + assertEquals(1, metadata.getData().size()); + } + + @Test + public void testGetKeys() { + UserMetadata metadata = new UserMetadata(); + metadata.addKV("group1", "key1", "value1".getBytes()); + metadata.addKV("group1", "key2", "value2".getBytes()); + metadata.addKV("group1", "key3", "value3".getBytes()); + + List keys = metadata.getKeys("group1"); + + assertNotNull(keys); + assertEquals(3, keys.size()); + assertTrue(keys.contains("key1")); + assertTrue(keys.contains("key2")); + assertTrue(keys.contains("key3")); + } + + @Test + public void testGetKeys_nonExistentGroup() { + UserMetadata metadata = new UserMetadata(); + + List keys = metadata.getKeys("nonExistent"); + + assertNotNull(keys); + assertTrue(keys.isEmpty()); + } + + @Test + public void testDeleteKey() { + UserMetadata metadata = new UserMetadata(); + metadata.addKV("group1", "key1", "value1".getBytes()); + metadata.addKV("group1", "key2", "value2".getBytes()); + + metadata.deleteKey("group1", "key1"); + + assertEquals(1, metadata.getData().get("group1").size()); + assertNull(metadata.getData().get("group1").get("key1")); + assertNotNull(metadata.getData().get("group1").get("key2")); + } + + @Test + public void testDeleteKey_nonExistentGroup() { + UserMetadata metadata = new UserMetadata(); + + // Should not throw exception + metadata.deleteKey("nonExistent", "key1"); + + assertTrue(metadata.getData().isEmpty()); + } + + @Test + public void testDeleteKey_nonExistentKey() { + UserMetadata metadata = new UserMetadata(); + metadata.addKV("group1", "key1", "value1".getBytes()); + + // Should not throw exception + metadata.deleteKey("group1", "nonExistent"); + + assertEquals(1, metadata.getData().get("group1").size()); + } + + @Test + public void testAddKV() { + UserMetadata metadata = new UserMetadata(); + + metadata.addKV("group1", "key1", "value1".getBytes()); + + assertEquals(1, metadata.getData().size()); + assertArrayEquals("value1".getBytes(), metadata.getData().get("group1").get("key1")); + } + + @Test + public void testAddKV_nullValue() { + UserMetadata metadata = new UserMetadata(); + + metadata.addKV("group1", "key1", null); + metadata.addKV(null, "key1", "value1".getBytes()); + metadata.addKV("group1", null, "value1".getBytes()); + + assertTrue(metadata.getData().isEmpty()); + } + + @Test + public void testAddKV_preventsMutation() { + UserMetadata metadata = new UserMetadata(); + byte[] value = "value1".getBytes(); + + metadata.addKV("group1", "key1", value); + + // Modify original byte array + value[0] = 'X'; + + // Verify metadata is not affected + assertArrayEquals("value1".getBytes(), metadata.getData().get("group1").get("key1")); + } + + @Test + public void testAddKV_overwritesExistingKey() { + UserMetadata metadata = new UserMetadata(); + metadata.addKV("group1", "key1", "value1".getBytes()); + + metadata.addKV("group1", "key1", "newValue".getBytes()); + + assertArrayEquals("newValue".getBytes(), metadata.getData().get("group1").get("key1")); + } + + @Test + public void testAddKVs() { + UserMetadata metadata = new UserMetadata(); + Map kv = new HashMap<>(); + kv.put("key1", "value1".getBytes()); + kv.put("key2", "value2".getBytes()); + + metadata.addKVs("group1", kv); + + assertEquals(1, metadata.getData().size()); + assertEquals(2, metadata.getData().get("group1").size()); + assertArrayEquals("value1".getBytes(), metadata.getData().get("group1").get("key1")); + assertArrayEquals("value2".getBytes(), metadata.getData().get("group1").get("key2")); + } + + @Test + public void testAddKVs_filtersNullValues() { + UserMetadata metadata = new UserMetadata(); + Map kv = new HashMap<>(); + kv.put("key1", "value1".getBytes()); + kv.put("key2", null); + + metadata.addKVs("group1", kv); + metadata.addKVs(null, kv); + metadata.addKVs("group1", null); + + assertEquals(1, metadata.getData().get("group1").size()); + assertArrayEquals("value1".getBytes(), metadata.getData().get("group1").get("key1")); + assertNull(metadata.getData().get("group1").get("key2")); + } + + @Test + public void testAddKVs_preventsMutation() { + UserMetadata metadata = new UserMetadata(); + byte[] value = "value1".getBytes(); + Map kv = new HashMap<>(); + kv.put("key1", value); + + metadata.addKVs("group1", kv); + + // Modify original byte array + value[0] = 'X'; + + // Verify metadata is not affected + assertArrayEquals("value1".getBytes(), metadata.getData().get("group1").get("key1")); + } + + @Test + public void testGetValue() { + UserMetadata metadata = new UserMetadata(); + metadata.addKV("group1", "key1", "value1".getBytes()); + + byte[] value = metadata.getValue("group1", "key1"); + + assertNotNull(value); + assertArrayEquals("value1".getBytes(), value); + } + + @Test + public void testGetValue_nonExistentGroup() { + UserMetadata metadata = new UserMetadata(); + + byte[] value = metadata.getValue("nonExistent", "key1"); + + assertNull(value); + } + + @Test + public void testGetValue_nonExistentKey() { + UserMetadata metadata = new UserMetadata(); + metadata.addKV("group1", "key1", "value1".getBytes()); + + byte[] value = metadata.getValue("group1", "nonExistent"); + + assertNull(value); + } + + @Test + public void testGetValue_returnsClone() { + UserMetadata metadata = new UserMetadata(); + metadata.addKV("group1", "key1", "value1".getBytes()); + + byte[] value = metadata.getValue("group1", "key1"); + value[0] = 'X'; + + // Verify internal data is not affected + assertArrayEquals("value1".getBytes(), metadata.getValue("group1", "key1")); + } + + @Test + public void testClear() { + UserMetadata metadata = new UserMetadata(); + metadata.addKV("group1", "key1", "value1".getBytes()); + metadata.addKV("group2", "key2", "value2".getBytes()); + + metadata.clear(); + + assertNotNull(metadata.getData()); + assertTrue(metadata.getData().isEmpty()); + } + + @Test + public void testClear_emptyMetadata() { + UserMetadata metadata = new UserMetadata(); + + // Should not throw exception + metadata.clear(); + + assertTrue(metadata.getData().isEmpty()); + } + + @Test + public void testRoundTrip_toProtoAndBack() { + UserMetadata original = new UserMetadata(); + original.addKV("group1", "key1", "value1".getBytes()); + original.addKV("group1", "key2", "value2".getBytes()); + original.addKV("group2", "keyA", "valueA".getBytes()); + + MetadataOuterClass.Metadata proto = original.toProto(); + UserMetadata reconstructed = new UserMetadata(proto); + + assertEquals(original.getData().size(), reconstructed.getData().size()); + assertArrayEquals( + original.getValue("group1", "key1"), + reconstructed.getValue("group1", "key1")); + assertArrayEquals( + original.getValue("group1", "key2"), + reconstructed.getValue("group1", "key2")); + assertArrayEquals( + original.getValue("group2", "keyA"), + reconstructed.getValue("group2", "keyA")); + } +} From 04243c0030338272a1d6e0863d3498728a44e73c Mon Sep 17 00:00:00 2001 From: vtiwari5 Date: Mon, 26 Jan 2026 11:38:33 -0500 Subject: [PATCH 3/8] Add comments regarding null checks Signed-off-by: vtiwari5 --- .../numaproj/numaflow/shared/SystemMetadata.java | 6 ++++-- .../io/numaproj/numaflow/shared/UserMetadata.java | 14 ++++---------- 2 files changed, 8 insertions(+), 12 deletions(-) diff --git a/src/main/java/io/numaproj/numaflow/shared/SystemMetadata.java b/src/main/java/io/numaproj/numaflow/shared/SystemMetadata.java index b2857dbc..00045724 100644 --- a/src/main/java/io/numaproj/numaflow/shared/SystemMetadata.java +++ b/src/main/java/io/numaproj/numaflow/shared/SystemMetadata.java @@ -61,8 +61,10 @@ public SystemMetadata(MetadataOuterClass.Metadata metadata) { this.data = new HashMap<>(); return; } - this.data = metadata.getSysMetadataMap().entrySet().stream() - .collect(Collectors.toMap( + this.data = metadata.getSysMetadataMap() + .entrySet().stream() + // No null checks here as protobuf contract ensures that the data has no null values + .collect(Collectors.toMap( Map.Entry::getKey, e -> new HashMap<>(e.getValue() .getKeyValueMap() diff --git a/src/main/java/io/numaproj/numaflow/shared/UserMetadata.java b/src/main/java/io/numaproj/numaflow/shared/UserMetadata.java index 9ecf1a51..ec496777 100644 --- a/src/main/java/io/numaproj/numaflow/shared/UserMetadata.java +++ b/src/main/java/io/numaproj/numaflow/shared/UserMetadata.java @@ -72,11 +72,11 @@ public UserMetadata(MetadataOuterClass.Metadata metadata) { this.data = metadata.getUserMetadataMap() .entrySet() .stream() + // No null checks here as protobuf contract ensures that the data has no null values .collect(Collectors.toMap( Map.Entry::getKey, e -> e.getValue() .getKeyValueMap().entrySet().stream() - .filter(e1 -> e1.getValue() != null) .collect(Collectors.toMap( Map.Entry::getKey, e1 -> e1.getValue().toByteArray() @@ -127,15 +127,9 @@ public MetadataOuterClass.Metadata toProto() { Map result = new HashMap<>(); this.data.forEach((group, kvMap) -> { MetadataOuterClass.KeyValueGroup.Builder builder = MetadataOuterClass.KeyValueGroup.newBuilder(); - if (kvMap != null) { - kvMap.forEach((key, value) -> { - if (value == null) { - builder.putKeyValue(key, ByteString.EMPTY); - } else { - builder.putKeyValue(key, ByteString.copyFrom(value)); - } - }); - } + // No null checks required as the constructor and add methods ensures that the data is valid + kvMap.forEach((key, value) -> builder.putKeyValue(key, ByteString.copyFrom(value))); + result.put(group, builder.build()); }); From dc00bc750d450c87514d52a6cdf8f5a0056be68f Mon Sep 17 00:00:00 2001 From: Vaibhav Tiwari Date: Mon, 26 Jan 2026 18:14:51 -0500 Subject: [PATCH 4/8] Add tests for verifying behaviour with usermetadata in source, source transform, mapper and sinker Signed-off-by: Vaibhav Tiwari --- .../io/numaproj/numaflow/sinker/Datum.java | 17 +++++++ .../numaflow/sinker/HandlerDatum.java | 16 ++++++- .../io/numaproj/numaflow/sinker/Service.java | 7 ++- .../numaflow/sinker/SinkerTestKit.java | 5 ++ .../io/numaproj/numaflow/sourcer/Message.java | 14 +++++- .../numaproj/numaflow/mapper/MessageTest.java | 7 +++ .../numaproj/numaflow/mapper/ServerTest.java | 46 ++++++++++++++++++- .../numaflow/sinker/DatumStreamImplTest.java | 12 +++++ .../numaflow/sinker/HandlerDatumTest.java | 28 ++++++++--- .../numaproj/numaflow/sinker/ServerTest.java | 35 +++++++++++--- .../numaproj/numaflow/sourcer/ServerTest.java | 22 ++++++++- .../sourcetransformer/ServerTest.java | 46 ++++++++++++++++++- 12 files changed, 237 insertions(+), 18 deletions(-) diff --git a/src/main/java/io/numaproj/numaflow/sinker/Datum.java b/src/main/java/io/numaproj/numaflow/sinker/Datum.java index 68cc91a4..7b5b1200 100644 --- a/src/main/java/io/numaproj/numaflow/sinker/Datum.java +++ b/src/main/java/io/numaproj/numaflow/sinker/Datum.java @@ -1,5 +1,8 @@ package io.numaproj.numaflow.sinker; +import io.numaproj.numaflow.shared.SystemMetadata; +import io.numaproj.numaflow.shared.UserMetadata; + import java.time.Instant; import java.util.Map; @@ -48,4 +51,18 @@ public interface Datum { * @return returns the headers in the form of key value pair */ Map getHeaders(); + + /** + * method to get user defined metadata information of the payload + * + * @return user metadata + */ + UserMetadata getUserMetadata(); + + /** + * method to get system metadata information of the payload + * + * @return system metadata + */ + SystemMetadata getSystemMetadata(); } diff --git a/src/main/java/io/numaproj/numaflow/sinker/HandlerDatum.java b/src/main/java/io/numaproj/numaflow/sinker/HandlerDatum.java index 60e9a2b2..c5fbcbaf 100644 --- a/src/main/java/io/numaproj/numaflow/sinker/HandlerDatum.java +++ b/src/main/java/io/numaproj/numaflow/sinker/HandlerDatum.java @@ -1,5 +1,7 @@ package io.numaproj.numaflow.sinker; +import io.numaproj.numaflow.shared.SystemMetadata; +import io.numaproj.numaflow.shared.UserMetadata; import lombok.AllArgsConstructor; import java.time.Instant; @@ -9,13 +11,15 @@ class HandlerDatum implements Datum { // EOF_DATUM is used to indicate the end of the stream. - static final HandlerDatum EOF_DATUM = new HandlerDatum(null, null, null, null, null, null); + static final HandlerDatum EOF_DATUM = new HandlerDatum(null, null, null, null, null, null, null, null); private String[] keys; private byte[] value; private Instant watermark; private Instant eventTime; private String id; private Map headers; + private UserMetadata userMetadata; + private SystemMetadata systemMetadata; @Override public String[] getKeys() { @@ -46,4 +50,14 @@ public String getId() { public Map getHeaders() { return this.headers; } + + @Override + public UserMetadata getUserMetadata() { + return this.userMetadata; + } + + @Override + public SystemMetadata getSystemMetadata() { + return this.systemMetadata; + } } diff --git a/src/main/java/io/numaproj/numaflow/sinker/Service.java b/src/main/java/io/numaproj/numaflow/sinker/Service.java index 5458a3f9..22bc6fdd 100644 --- a/src/main/java/io/numaproj/numaflow/sinker/Service.java +++ b/src/main/java/io/numaproj/numaflow/sinker/Service.java @@ -9,6 +9,8 @@ import io.grpc.protobuf.StatusProto; import io.grpc.stub.StreamObserver; import io.numaproj.numaflow.shared.ExceptionUtils; +import io.numaproj.numaflow.shared.SystemMetadata; +import io.numaproj.numaflow.shared.UserMetadata; import io.numaproj.numaflow.sink.v1.SinkGrpc; import io.numaproj.numaflow.sink.v1.SinkOuterClass; import java.time.Instant; @@ -193,7 +195,10 @@ private HandlerDatum constructHandlerDatum(SinkOuterClass.SinkRequest d) { d.getRequest().getEventTime().getSeconds(), d.getRequest().getEventTime().getNanos()), d.getRequest().getId(), - d.getRequest().getHeadersMap()); + d.getRequest().getHeadersMap(), + new UserMetadata(d.getRequest().getMetadata()), + new SystemMetadata(d.getRequest().getMetadata()) + ); } // shuts down the executor service diff --git a/src/main/java/io/numaproj/numaflow/sinker/SinkerTestKit.java b/src/main/java/io/numaproj/numaflow/sinker/SinkerTestKit.java index efa8a9d4..7d842d01 100644 --- a/src/main/java/io/numaproj/numaflow/sinker/SinkerTestKit.java +++ b/src/main/java/io/numaproj/numaflow/sinker/SinkerTestKit.java @@ -5,6 +5,8 @@ import io.grpc.ManagedChannel; import io.grpc.ManagedChannelBuilder; import io.grpc.stub.StreamObserver; +import io.numaproj.numaflow.shared.SystemMetadata; +import io.numaproj.numaflow.shared.UserMetadata; import io.numaproj.numaflow.sink.v1.SinkGrpc; import io.numaproj.numaflow.sink.v1.SinkOuterClass; import jdk.jfr.Experimental; @@ -302,5 +304,8 @@ public static class TestDatum implements Datum { private final Instant eventTime; private final Instant watermark; private final Map headers; + private final UserMetadata userMetadata; + private final SystemMetadata systemMetadata; + } } diff --git a/src/main/java/io/numaproj/numaflow/sourcer/Message.java b/src/main/java/io/numaproj/numaflow/sourcer/Message.java index 310666c1..8507209c 100644 --- a/src/main/java/io/numaproj/numaflow/sourcer/Message.java +++ b/src/main/java/io/numaproj/numaflow/sourcer/Message.java @@ -52,6 +52,18 @@ public Message(byte[] value, Offset offset, Instant eventTime, Map prevVertexUserMetadata = new HashMap<>(); + prevVertexUserMetadata.put("prev-group", + MetadataOuterClass.KeyValueGroup + .newBuilder() + .putKeyValue("prev-key", ByteString.copyFromUtf8("prev-value")) + .build() + ); + MetadataOuterClass.Metadata metadata = MetadataOuterClass.Metadata + .newBuilder() + .putAllUserMetadata(prevVertexUserMetadata) + .build(); + ByteString inValue = ByteString.copyFromUtf8("invalue"); MapOuterClass.MapRequest inDatum = MapOuterClass.MapRequest .newBuilder() @@ -71,6 +88,7 @@ public void testMapperSuccess() { .newBuilder() .setValue(inValue) .addAllKeys(List.of("test-map-key")) + .setMetadata(metadata) .build()).build(); String[] expectedKeys = new String[]{"test-map-key" + PROCESSED_KEY_SUFFIX}; @@ -106,6 +124,27 @@ public void testMapperSuccess() { assertEquals(Arrays.asList(expectedKeys), response.getResults(0).getKeysList()); assertEquals(Arrays.asList(expectedTags), response.getResults(0).getTagsList()); assertEquals(1, response.getResultsCount()); + // User metadata should be added to the response. + // It should have both the previous metadata and the metadata added by the mapper. + assertEquals(2, response.getResults(0).getMetadata().getUserMetadataMap().size()); + assertEquals("prev-value", + response.getResults(0) + .getMetadata() + .getUserMetadataMap() + .get("prev-group") + .getKeyValueMap() + .get("prev-key") + .toStringUtf8() + ); + assertEquals("udf-value", + response.getResults(0) + .getMetadata() + .getUserMetadataMap() + .get("udf-group") + .getKeyValueMap() + .get("udf-key") + .toStringUtf8() + ); } requestStreamObserver.onCompleted(); @@ -148,13 +187,18 @@ public MessageList processMessage(String[] keys, Datum datum) { .stream(keys) .map(c -> c + PROCESSED_KEY_SUFFIX) .toArray(String[]::new); + + UserMetadata userMetadata = new UserMetadata(datum.getUserMetadata()); + userMetadata.addKV("udf-group", "udf-key", "udf-value".getBytes()); + return MessageList .newBuilder() .addMessage(new Message( (new String(datum.getValue()) + PROCESSED_VALUE_SUFFIX).getBytes(), updatedKeys, - new String[]{"test-tag"})) + new String[]{"test-tag"}, + userMetadata)) .build(); } } diff --git a/src/test/java/io/numaproj/numaflow/sinker/DatumStreamImplTest.java b/src/test/java/io/numaproj/numaflow/sinker/DatumStreamImplTest.java index 9e941b3d..b9981161 100644 --- a/src/test/java/io/numaproj/numaflow/sinker/DatumStreamImplTest.java +++ b/src/test/java/io/numaproj/numaflow/sinker/DatumStreamImplTest.java @@ -1,5 +1,7 @@ package io.numaproj.numaflow.sinker; +import io.numaproj.numaflow.shared.SystemMetadata; +import io.numaproj.numaflow.shared.UserMetadata; import lombok.AllArgsConstructor; import org.junit.Test; @@ -63,6 +65,16 @@ public String getId() { public Map getHeaders() { return null; } + + @Override + public UserMetadata getUserMetadata() { + return null; + } + + @Override + public SystemMetadata getSystemMetadata() { + return null; + } } } diff --git a/src/test/java/io/numaproj/numaflow/sinker/HandlerDatumTest.java b/src/test/java/io/numaproj/numaflow/sinker/HandlerDatumTest.java index 2097b42f..a463819a 100644 --- a/src/test/java/io/numaproj/numaflow/sinker/HandlerDatumTest.java +++ b/src/test/java/io/numaproj/numaflow/sinker/HandlerDatumTest.java @@ -1,5 +1,7 @@ package io.numaproj.numaflow.sinker; +import io.numaproj.numaflow.shared.SystemMetadata; +import io.numaproj.numaflow.shared.UserMetadata; import org.junit.Test; import java.time.Instant; @@ -14,35 +16,35 @@ public class HandlerDatumTest { @Test public void testGetKeys() { String[] keys = {"key1", "key2"}; - HandlerDatum datum = new HandlerDatum(keys, null, null, null, null, null); + HandlerDatum datum = new HandlerDatum(keys, null, null, null, null, null, null, null); assertArrayEquals(keys, datum.getKeys()); } @Test public void testGetValue() { byte[] value = {1, 2, 3}; - HandlerDatum datum = new HandlerDatum(null, value, null, null, null, null); + HandlerDatum datum = new HandlerDatum(null, value, null, null, null, null, null, null); assertArrayEquals(value, datum.getValue()); } @Test public void testGetWatermark() { Instant watermark = Instant.now(); - HandlerDatum datum = new HandlerDatum(null, null, watermark, null, null, null); + HandlerDatum datum = new HandlerDatum(null, null, watermark, null, null, null, null, null); assertEquals(watermark, datum.getWatermark()); } @Test public void testGetEventTime() { Instant eventTime = Instant.now(); - HandlerDatum datum = new HandlerDatum(null, null, null, eventTime, null, null); + HandlerDatum datum = new HandlerDatum(null, null, null, eventTime, null, null, null, null); assertEquals(eventTime, datum.getEventTime()); } @Test public void testGetId() { String id = "test-id"; - HandlerDatum datum = new HandlerDatum(null, null, null, null, id, null); + HandlerDatum datum = new HandlerDatum(null, null, null, null, id, null, null, null); assertEquals(id, datum.getId()); } @@ -50,7 +52,21 @@ public void testGetId() { public void testGetHeaders() { Map headers = new HashMap<>(); headers.put("header1", "value1"); - HandlerDatum datum = new HandlerDatum(null, null, null, null, null, headers); + HandlerDatum datum = new HandlerDatum(null, null, null, null, null, headers, null, null); assertEquals(headers, datum.getHeaders()); } + + @Test + public void testGetUserMetadata() { + UserMetadata userMetadata = new UserMetadata(); + HandlerDatum datum = new HandlerDatum(null, null, null, null, null, null, userMetadata, null); + assertEquals(userMetadata, datum.getUserMetadata()); + } + + @Test + public void testGetSystemMetadata() { + SystemMetadata systemMetadata = new SystemMetadata(); + HandlerDatum datum = new HandlerDatum(null, null, null, null, null, null, null, systemMetadata); + assertEquals(systemMetadata, datum.getSystemMetadata()); + } } diff --git a/src/test/java/io/numaproj/numaflow/sinker/ServerTest.java b/src/test/java/io/numaproj/numaflow/sinker/ServerTest.java index 32de9fce..8f866b8f 100644 --- a/src/test/java/io/numaproj/numaflow/sinker/ServerTest.java +++ b/src/test/java/io/numaproj/numaflow/sinker/ServerTest.java @@ -1,19 +1,27 @@ package io.numaproj.numaflow.sinker; +import static java.util.Map.entry; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import com.google.protobuf.ByteString; +import common.MetadataOuterClass; import io.grpc.ManagedChannel; import io.grpc.inprocess.InProcessChannelBuilder; import io.grpc.inprocess.InProcessServerBuilder; import io.grpc.stub.StreamObserver; import io.grpc.testing.GrpcCleanupRule; +import io.numaproj.numaflow.shared.UserMetadata; import io.numaproj.numaflow.sink.v1.SinkGrpc; import io.numaproj.numaflow.sink.v1.SinkOuterClass; + +import java.nio.charset.StandardCharsets; import java.util.Arrays; +import java.util.HashMap; import java.util.List; +import java.util.Map; + import lombok.extern.slf4j.Slf4j; import org.junit.After; import org.junit.Before; @@ -26,6 +34,9 @@ @RunWith(JUnit4.class) public class ServerTest { private static final String processedIdSuffix = "-id-processed"; + private static final String umdGroup = "group"; + private static final String umdKey = "key"; + private static final String umdValue = "value"; @Rule public final GrpcCleanupRule grpcCleanup = new GrpcCleanupRule(); private Server server; private ManagedChannel inProcessChannel; @@ -79,7 +90,7 @@ public void sinkerSuccess() { SinkGrpc.newStub(inProcessChannel).sinkFn(outputStreamObserver); String actualId = "sink_test_id"; - String expectedId = actualId + processedIdSuffix; + String expectedId = actualId + umdValue + processedIdSuffix; // Send a handshake request SinkOuterClass.SinkRequest handshakeRequest = @@ -102,11 +113,21 @@ public void sinkerSuccess() { keys = new String[] {"invalid-key"}; } + // create user metadata to simulate it being passed from upstream + Map> userMetadataMap = Map.ofEntries( + entry(umdGroup, Map.ofEntries( + entry(umdKey, umdValue.getBytes()) + )) + ); + UserMetadata userMetadataObj = new UserMetadata(userMetadataMap); + MetadataOuterClass.Metadata metadata = userMetadataObj.toProto(); + SinkOuterClass.SinkRequest.Request request = SinkOuterClass.SinkRequest.Request.newBuilder() .setValue(ByteString.copyFromUtf8(String.valueOf(i))) .setId(actualId) .addAllKeys(List.of(keys)) + .setMetadata(metadata) .build(); SinkOuterClass.SinkRequest sinkRequest = @@ -178,20 +199,22 @@ public ResponseList processMessages(DatumIterator datumIterator) { break; } + String mdValue = new String(datum.getUserMetadata().getValue(umdGroup, umdKey), StandardCharsets.UTF_8); + if (Arrays.equals(datum.getKeys(), new String[] {"invalid-key"})) { builder.addResponse( - Response.responseFailure(datum.getId() + processedIdSuffix, "error message")); + Response.responseFailure(datum.getId() + mdValue + processedIdSuffix, "error message")); } else if (Arrays.equals(datum.getKeys(), new String[] {"fallback-key"})) { builder.addResponse( - Response.responseFallback(datum.getId() + processedIdSuffix)); + Response.responseFallback(datum.getId() + mdValue + processedIdSuffix)); } else if (Arrays.equals(datum.getKeys(), new String[] {"onsuccess-key"})) { builder.addResponse( - Response.responseOnSuccess(datum.getId() + processedIdSuffix, (Message) null)); + Response.responseOnSuccess(datum.getId() + mdValue + processedIdSuffix, (Message) null)); } else if (Arrays.equals(datum.getKeys(), new String[] {"serve-key"})) { builder.addResponse( - Response.responseServe(datum.getId() + processedIdSuffix, "serve message".getBytes())); + Response.responseServe(datum.getId() + mdValue + processedIdSuffix, "serve message".getBytes())); } else { - builder.addResponse(Response.responseOK(datum.getId() + processedIdSuffix)); + builder.addResponse(Response.responseOK(datum.getId() + mdValue + processedIdSuffix)); } } diff --git a/src/test/java/io/numaproj/numaflow/sourcer/ServerTest.java b/src/test/java/io/numaproj/numaflow/sourcer/ServerTest.java index 14c29b46..b97f8675 100644 --- a/src/test/java/io/numaproj/numaflow/sourcer/ServerTest.java +++ b/src/test/java/io/numaproj/numaflow/sourcer/ServerTest.java @@ -1,11 +1,13 @@ package io.numaproj.numaflow.sourcer; import com.google.protobuf.Empty; +import common.MetadataOuterClass; import io.grpc.ManagedChannel; import io.grpc.inprocess.InProcessChannelBuilder; import io.grpc.inprocess.InProcessServerBuilder; import io.grpc.stub.StreamObserver; import io.grpc.testing.GrpcCleanupRule; +import io.numaproj.numaflow.shared.UserMetadata; import io.numaproj.numaflow.source.v1.SourceGrpc; import io.numaproj.numaflow.source.v1.SourceOuterClass; import org.junit.After; @@ -16,6 +18,7 @@ import java.nio.ByteBuffer; import java.time.Instant; import java.util.ArrayList; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; @@ -85,6 +88,7 @@ public void TestSourcer() { int count = 0; boolean handshake = false; boolean eot = false; + ArrayList> userMetadataList = new ArrayList<>(); @Override public void onNext(SourceOuterClass.ReadResponse readResponse) { @@ -98,6 +102,7 @@ public void onNext(SourceOuterClass.ReadResponse readResponse) { return; } count++; + userMetadataList.add(readResponse.getResult().getMetadata().getUserMetadataMap()); SourceOuterClass.Offset offset = readResponse.getResult().getOffset(); offsets.add(offset); SourceOuterClass.AckRequest.Request ackRequest = SourceOuterClass.AckRequest @@ -123,6 +128,14 @@ public void onCompleted() { assertEquals(10, count); assertTrue(handshake); assertTrue(eot); + // we should get 10 userMetadata with metadata intact + assertEquals(10, userMetadataList.size()); + assertEquals( + "src-value", + userMetadataList.get(0) + .get("src-group").getKeyValueMap() + .get("src-key").toStringUtf8() + ); } }); @@ -279,11 +292,18 @@ private static class TestSourcer extends Sourcer { public TestSourcer() { Instant eventTime = Instant.ofEpochMilli(1000L); + + // create user metadata + Map> userMetadataMap = new HashMap<>(); + userMetadataMap.put("src-group", Map.of("src-key", "src-value".getBytes())); + UserMetadata userMetadata = new UserMetadata(userMetadataMap); + for (int i = 0; i < 10; i++) { messages.add(new Message( ByteBuffer.allocate(4).putInt(i).array(), new Offset(ByteBuffer.allocate(4).putInt(i).array(), 0), - eventTime + eventTime, + userMetadata )); eventTime = eventTime.plusMillis(1000L); } diff --git a/src/test/java/io/numaproj/numaflow/sourcetransformer/ServerTest.java b/src/test/java/io/numaproj/numaflow/sourcetransformer/ServerTest.java index 18645482..07066b80 100644 --- a/src/test/java/io/numaproj/numaflow/sourcetransformer/ServerTest.java +++ b/src/test/java/io/numaproj/numaflow/sourcetransformer/ServerTest.java @@ -1,10 +1,12 @@ package io.numaproj.numaflow.sourcetransformer; import com.google.protobuf.ByteString; +import common.MetadataOuterClass; import io.grpc.ManagedChannel; import io.grpc.inprocess.InProcessChannelBuilder; import io.grpc.inprocess.InProcessServerBuilder; import io.grpc.testing.GrpcCleanupRule; +import io.numaproj.numaflow.shared.UserMetadata; import io.numaproj.numaflow.sourcetransformer.v1.SourceTransformGrpc; import io.numaproj.numaflow.sourcetransformer.v1.Sourcetransformer; import org.junit.After; @@ -14,7 +16,9 @@ import java.time.Instant; import java.util.Arrays; +import java.util.HashMap; import java.util.List; +import java.util.Map; import java.util.concurrent.ExecutionException; import static org.junit.Assert.assertEquals; @@ -69,11 +73,25 @@ public void testSourceTransformerSuccess() { .build()) .build(); + // Build user metadata and use it to initialize metadata to be added to the message passed to mapper + Map prevVertexUserMetadata = new HashMap<>(); + prevVertexUserMetadata.put("prev-group", + MetadataOuterClass.KeyValueGroup + .newBuilder() + .putKeyValue("prev-key", ByteString.copyFromUtf8("prev-value")) + .build() + ); + MetadataOuterClass.Metadata metadata = MetadataOuterClass.Metadata + .newBuilder() + .putAllUserMetadata(prevVertexUserMetadata) + .build(); + ByteString inValue = ByteString.copyFromUtf8("invalue"); Sourcetransformer.SourceTransformRequest.Request inDatum = Sourcetransformer.SourceTransformRequest.Request .newBuilder() .setValue(inValue) .addAllKeys(List.of("test-st-key")) + .setMetadata(metadata) .build(); Sourcetransformer.SourceTransformRequest request = Sourcetransformer.SourceTransformRequest @@ -112,6 +130,27 @@ public void testSourceTransformerSuccess() { assertEquals(expectedValue, response.getResults(0).getValue()); assertEquals(Arrays.asList(expectedKeys), response.getResults(0).getKeysList()); assertEquals(1, response.getResultsCount()); + // User metadata should be added to the response. + // It should have both the previous metadata and the metadata added by the mapper. + assertEquals(2, response.getResults(0).getMetadata().getUserMetadataMap().size()); + assertEquals("prev-value", + response.getResults(0) + .getMetadata() + .getUserMetadataMap() + .get("prev-group") + .getKeyValueMap() + .get("prev-key") + .toStringUtf8() + ); + assertEquals("st-value", + response.getResults(0) + .getMetadata() + .getUserMetadataMap() + .get("st-group") + .getKeyValueMap() + .get("st-key") + .toStringUtf8() + ); } requestStreamObserver.onCompleted(); @@ -157,6 +196,10 @@ public MessageList processMessage(String[] keys, Datum datum) { .stream(keys) .map(c -> c + PROCESSED_KEY_SUFFIX) .toArray(String[]::new); + + UserMetadata userMetadata = new UserMetadata(datum.getUserMetadata()); + userMetadata.addKV("st-group", "st-key", "st-value".getBytes()); + return MessageList .newBuilder() .addMessage(new Message( @@ -164,7 +207,8 @@ public MessageList processMessage(String[] keys, Datum datum) { + PROCESSED_VALUE_SUFFIX).getBytes(), TEST_EVENT_TIME, updatedKeys, - new String[]{"test-tag"})) + new String[]{"test-tag"}, + userMetadata)) .build(); } } From c4be542405bd156535e46d7291f2c4532876cda9 Mon Sep 17 00:00:00 2001 From: Vaibhav Tiwari Date: Wed, 28 Jan 2026 13:55:46 -0500 Subject: [PATCH 5/8] Address code review comments Signed-off-by: Vaibhav Tiwari --- .../examples/sink/onsuccess/OnSuccess.java | 5 + .../numaflow/shared/SystemMetadata.java | 46 -- .../numaflow/shared/UserMetadata.java | 66 +-- .../io/numaproj/numaflow/sinker/Message.java | 52 +- .../io/numaproj/numaflow/sinker/Response.java | 38 +- .../io/numaproj/numaflow/sinker/Service.java | 4 +- .../numaproj/numaflow/mapper/MessageTest.java | 2 +- .../numaflow/shared/SystemMetadataTest.java | 269 +++------- .../numaflow/shared/UserMetadataTest.java | 492 +++--------------- .../numaflow/sinker/ResponseTest.java | 39 +- .../numaproj/numaflow/sinker/ServerTest.java | 8 +- .../numaproj/numaflow/sourcer/ServerTest.java | 5 +- 12 files changed, 270 insertions(+), 756 deletions(-) diff --git a/examples/src/main/java/io/numaproj/numaflow/examples/sink/onsuccess/OnSuccess.java b/examples/src/main/java/io/numaproj/numaflow/examples/sink/onsuccess/OnSuccess.java index b24342d4..0d9b8026 100644 --- a/examples/src/main/java/io/numaproj/numaflow/examples/sink/onsuccess/OnSuccess.java +++ b/examples/src/main/java/io/numaproj/numaflow/examples/sink/onsuccess/OnSuccess.java @@ -44,11 +44,16 @@ public ResponseList processMessages(DatumIterator datumIterator) { log.info("Received message: {}, id: {}, headers - {}", msg, datum.getId(), datum.getHeaders()); if (writeToPrimarySink()) { log.info("Writing to onSuccess sink: {}", datum.getId()); + // Build the onSuccess message using builder for changing values, keys or userMetadata responseListBuilder.addResponse(Response.responseOnSuccess(datum.getId(), Message.builder() .value(String.format("Successfully wrote message with ID: %s", datum.getId()).getBytes()) + .userMetadata(datum.getUserMetadata()) + .keys(datum.getKeys()) .build())); + // Or send the same values as datum using: + // responseListBuilder.addResponse(Response.responseOnSuccess(datum)); } else { log.info("Writing to fallback sink: {}", datum.getId()); responseListBuilder.addResponse(Response.responseFallback(datum.getId())); diff --git a/src/main/java/io/numaproj/numaflow/shared/SystemMetadata.java b/src/main/java/io/numaproj/numaflow/shared/SystemMetadata.java index 00045724..f82749d6 100644 --- a/src/main/java/io/numaproj/numaflow/shared/SystemMetadata.java +++ b/src/main/java/io/numaproj/numaflow/shared/SystemMetadata.java @@ -14,7 +14,6 @@ * SystemMetadata wraps system-generated metadata groups per message. * It is read-only to UDFs */ -@Getter public class SystemMetadata { private final Map> data; @@ -25,32 +24,6 @@ public SystemMetadata() { this.data = new HashMap<>(); } - /** - * An all args constructor that filters out null values and copies the values to prevent mutation. - * If the data is null or empty, it initializes an empty HashMap data. - * For each entry in {@code data}, it creates a new HashMap and copies the key-value pairs - * from the entry to the new HashMap. It also filters out any null values. - * Empty hashmap values are allowed as entries in the data map. - * - * @param data is a map of group name to key-value pairs - */ - public SystemMetadata(Map> data) { - if (data == null || data.isEmpty()) { - this.data = new HashMap<>(); - return; - } - this.data = data.entrySet().stream() - .filter(e -> e.getValue() != null) - .collect(Collectors.toMap(Map.Entry::getKey, - entry -> entry.getValue().entrySet().stream() - .filter(e1 -> e1.getValue() != null) - .collect(Collectors.toMap( - Map.Entry::getKey, - e1-> e1.getValue().clone() - )) - )); - } - /** * Constructor from MetadataOuterClass.Metadata * @@ -78,25 +51,6 @@ public SystemMetadata(MetadataOuterClass.Metadata metadata) { )); } - /** - * Get the data as a map. - * Returns a deep copy of the data to prevent mutation. - * - * @return a deep copy of the data - */ - public Map> getData() { - // Deep copy the data to prevent mutation - return this.data.entrySet().stream() - // No null checks required as the constructor ensures that the data is valid - .collect(Collectors.toMap(Map.Entry::getKey, - entry -> entry.getValue().entrySet().stream() - .collect(Collectors.toMap( - Map.Entry::getKey, - e1-> e1.getValue().clone() - )) - )); - } - /** * Get the list of all groups present in the user metadata * diff --git a/src/main/java/io/numaproj/numaflow/shared/UserMetadata.java b/src/main/java/io/numaproj/numaflow/shared/UserMetadata.java index ec496777..09780ed3 100644 --- a/src/main/java/io/numaproj/numaflow/shared/UserMetadata.java +++ b/src/main/java/io/numaproj/numaflow/shared/UserMetadata.java @@ -1,6 +1,7 @@ package io.numaproj.numaflow.shared; import common.MetadataOuterClass; +import lombok.EqualsAndHashCode; import lombok.Getter; import com.google.protobuf.ByteString; @@ -28,33 +29,6 @@ public UserMetadata() { this.data = new HashMap<>(); } - - /** - * An all args constructor that filters out null values and copies the values to prevent mutation. - * If the data is null or empty, it initializes an empty HashMap data. - * For each entry in {@code data}, it creates a new HashMap and copies the key-value pairs - * from the entry to the new HashMap. It also filters out any null values. - * Empty hashmap values are allowed as entries in the data map. - * - * @param data is a map of group name to key-value pairs - */ - public UserMetadata(Map> data) { - if (data == null || data.isEmpty()) { - this.data = new HashMap<>(); - return; - } - this.data = data.entrySet().stream() - .filter(e -> e.getValue() != null) - .collect(Collectors.toMap(Map.Entry::getKey, - entry -> entry.getValue().entrySet().stream() - .filter(e1 -> e1.getValue() != null) - .collect(Collectors.toMap( - Map.Entry::getKey, - e1-> e1.getValue().clone() - )) - )); - } - /** * Constructor from MetadataOuterClass.Metadata. * Initializes an empty HashMap data if the metadata passed is null or empty. @@ -139,24 +113,6 @@ public MetadataOuterClass.Metadata toProto() { .build(); } - /** - * Get the data as a map. - * Returns a deep copy of the data to prevent mutation. - * - * @return a deep copy of the data - */ - public Map> getData() { - // Deep copy the data to prevent mutation - return this.data.entrySet().stream() - .collect(Collectors.toMap(Map.Entry::getKey, - entry -> entry.getValue().entrySet().stream() - .collect(Collectors.toMap( - Map.Entry::getKey, - e1-> e1.getValue().clone() - )) - )); - } - /** * Get the list of all groups present in the user metadata * @@ -229,14 +185,18 @@ public void addKVs(String group, Map kv) { if (group == null || kv == null) { return; } - Map groupMap = this.data.computeIfAbsent(group, k -> new HashMap<>()); - kv.forEach((key, value) -> { - // null values are not added - if (value != null) { - // clone the value to prevent mutation - groupMap.put(key, value.clone()); - } - }); + + this.data.computeIfAbsent( + group, + k -> kv.entrySet().stream() + .filter(e -> e.getKey() != null && e.getValue() != null) + .collect( + Collectors.toMap( + Map.Entry::getKey, + e -> e.getValue().clone() + ) + ) + ); } /** diff --git a/src/main/java/io/numaproj/numaflow/sinker/Message.java b/src/main/java/io/numaproj/numaflow/sinker/Message.java index d62c7b47..f3590ded 100644 --- a/src/main/java/io/numaproj/numaflow/sinker/Message.java +++ b/src/main/java/io/numaproj/numaflow/sinker/Message.java @@ -1,9 +1,15 @@ package io.numaproj.numaflow.sinker; +import com.google.protobuf.ByteString; +import common.MetadataOuterClass; import io.numaproj.numaflow.shared.UserMetadata; +import io.numaproj.numaflow.sink.v1.SinkOuterClass; import lombok.Builder; import lombok.Getter; +import java.util.ArrayList; +import java.util.Arrays; + /** * Message contains information that needs to be sent to the OnSuccess sink. * The message can be different from the original message that was sent to primary sink. @@ -12,8 +18,52 @@ @Builder public class Message { private final byte[] value; - private final String key; + private final String[] keys; + /** + * userMetadata is the user defined metadata that is added to the onSuccess message + * This is using the common {@link UserMetadata} class to allow reusing the user metadata stored in Datum + */ private final UserMetadata userMetadata; + + /** + * Static method to create an onSuccess message from a sinker Datum object. + * + * @param datum object used to create the onSuccess message. + * The created onSuccess message will have the same value, keys and userMetadata as the original datum + * @return onSuccess message + */ + public static Message fromDatum(Datum datum) { + if (datum == null) { + return Message.builder().build(); + } + return Message.builder() + .value(datum.getValue()) + .keys(datum.getKeys()) + .userMetadata(datum.getUserMetadata()) + .build(); + } + + /** + * Static method to convert a Message object to a SinkOuterClass.SinkResponse.Result.Message object. + * If the message is null, returns the default instance of SinkOuterClass.SinkResponse.Result.Message. + * + * @param message The message object to convert into the relevant proto object + * @return The converted proto object + */ + public static SinkOuterClass.SinkResponse.Result.Message toProto(Message message) { + if (message == null) { + return SinkOuterClass.SinkResponse.Result.Message.getDefaultInstance(); + } + return SinkOuterClass.SinkResponse.Result.Message.newBuilder() + .addAllKeys(message.getKeys() + == null ? new ArrayList<>() : Arrays.asList(message.getKeys())) + .setValue(message.getValue() + == null ? ByteString.EMPTY : ByteString.copyFrom(message.getValue())) + .setMetadata(message.getUserMetadata() + == null ? MetadataOuterClass.Metadata.getDefaultInstance() + : message.getUserMetadata().toProto()) + .build(); + } } diff --git a/src/main/java/io/numaproj/numaflow/sinker/Response.java b/src/main/java/io/numaproj/numaflow/sinker/Response.java index bb60eb40..e2765c2a 100644 --- a/src/main/java/io/numaproj/numaflow/sinker/Response.java +++ b/src/main/java/io/numaproj/numaflow/sinker/Response.java @@ -7,6 +7,10 @@ import lombok.AllArgsConstructor; import lombok.Getter; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.stream.Collectors; + /** * Response is used to send response from the user defined sinker. It contains the id of the * message, success status, an optional error message and a fallback status. Various static factory @@ -22,9 +26,7 @@ public class Response { private final Boolean serve; private final byte[] serveResponse; private final Boolean onSuccess; - // FIXME: Should this be Message object from this package? That would allow parity with other SDKs (specially Go) - // Currently done this way to prevent conversion in buildResult method. - private final SinkResponse.Result.Message onSuccessMessage; + private final Message onSuccessMessage; /** * Static method to create response for successful message processing. @@ -72,15 +74,17 @@ public static Response responseServe(String id, byte[] serveResponse) { } /** - * Static method to create response for onSuccess message. Allows creation of onSuccess message - * from protobuf Message object. + * Static method to create response for onSuccess message using the Datum object. * - * @param id id of the message - * @param onSuccessMessage OnSuccessMessage object to be sent to the onSuccess sink + * @param datum Datum object using which onSuccess message is created. Can be the original datum * @return Response object with onSuccess status and onSuccess message */ - public static Response responseOnSuccess(String id, SinkResponse.Result.Message onSuccessMessage) { - return new Response(id, false, null, false, false, null, true, onSuccessMessage); + public static Response responseOnSuccess(Datum datum) { + if (datum == null) { + // NOTE: response id is null if datum is null + return new Response(null, false, null, false, false, null, true, null); + } + return responseOnSuccess(datum.getId(), Message.fromDatum(datum)); } /** @@ -93,20 +97,6 @@ public static Response responseOnSuccess(String id, SinkResponse.Result.Message * @return Response object with onSuccess status and onSuccess message */ public static Response responseOnSuccess(String id, Message onSuccessMessage) { - if (onSuccessMessage == null) { - return new Response(id, false, null, false, false, null, true, null); - } else { - SinkResponse.Result.Message pbOnSuccessMessage = SinkResponse.Result.Message.newBuilder() - .addKeys(onSuccessMessage.getKey() - == null ? "" : onSuccessMessage.getKey()) - .setValue(onSuccessMessage.getValue() - == null ? ByteString.EMPTY : ByteString.copyFrom(onSuccessMessage.getValue())) - .setMetadata(onSuccessMessage.getUserMetadata() - == null ? MetadataOuterClass.Metadata.getDefaultInstance() - : onSuccessMessage.getUserMetadata().toProto()) - .build(); - - return new Response(id, false, null, false, false, null, true, pbOnSuccessMessage); - } + return new Response(id, false, null, false, false, null, true, onSuccessMessage); } } diff --git a/src/main/java/io/numaproj/numaflow/sinker/Service.java b/src/main/java/io/numaproj/numaflow/sinker/Service.java index 22bc6fdd..3c51df58 100644 --- a/src/main/java/io/numaproj/numaflow/sinker/Service.java +++ b/src/main/java/io/numaproj/numaflow/sinker/Service.java @@ -159,9 +159,7 @@ private SinkOuterClass.SinkResponse.Result buildResult(Response response) { return SinkOuterClass.SinkResponse.Result.newBuilder() .setId(response.getId() == null ? "" : response.getId()) .setStatus(SinkOuterClass.Status.ON_SUCCESS) - .setOnSuccessMsg(response.getOnSuccessMessage() == null - ? SinkOuterClass.SinkResponse.Result.Message.getDefaultInstance() - : response.getOnSuccessMessage()) + .setOnSuccessMsg(Message.toProto(response.getOnSuccessMessage())) .build(); } else { // FIXME: Return error when error message is not set? diff --git a/src/test/java/io/numaproj/numaflow/mapper/MessageTest.java b/src/test/java/io/numaproj/numaflow/mapper/MessageTest.java index 4851299c..c95e065f 100644 --- a/src/test/java/io/numaproj/numaflow/mapper/MessageTest.java +++ b/src/test/java/io/numaproj/numaflow/mapper/MessageTest.java @@ -24,6 +24,6 @@ public void testMessage() { assertArrayEquals("asdf".getBytes(), message5.getValue()); assertArrayEquals(new String[]{"key1"}, message5.getKeys()); assertArrayEquals(new String[]{"tag1"}, message5.getTags()); - assertTrue(message5.getUserMetadata().getData().isEmpty()); + assertTrue(message5.getUserMetadata().getGroups().isEmpty()); } } diff --git a/src/test/java/io/numaproj/numaflow/shared/SystemMetadataTest.java b/src/test/java/io/numaproj/numaflow/shared/SystemMetadataTest.java index b880319b..100b7c44 100644 --- a/src/test/java/io/numaproj/numaflow/shared/SystemMetadataTest.java +++ b/src/test/java/io/numaproj/numaflow/shared/SystemMetadataTest.java @@ -4,9 +4,7 @@ import common.MetadataOuterClass; import org.junit.Test; -import java.util.HashMap; import java.util.List; -import java.util.Map; import static org.junit.Assert.*; @@ -15,74 +13,8 @@ public class SystemMetadataTest { @Test public void testDefaultConstructor() { SystemMetadata metadata = new SystemMetadata(); - assertNotNull(metadata.getData()); - assertTrue(metadata.getData().isEmpty()); - } - - @Test - public void testMapConstructor_withValidData() { - Map> data = new HashMap<>(); - Map group1 = new HashMap<>(); - group1.put("key1", "value1".getBytes()); - Map group2 = new HashMap<>(); - group2.put("keyA", "valueA".getBytes()); - data.put("group1", group1); - data.put("group2", group2); - - SystemMetadata metadata = new SystemMetadata(data); - - assertEquals(2, metadata.getData().size()); - assertArrayEquals("value1".getBytes(), metadata.getData().get("group1").get("key1")); - assertArrayEquals("valueA".getBytes(), metadata.getData().get("group2").get("keyA")); - } - - @Test - public void testMapConstructor_withNullData() { - SystemMetadata metadata = new SystemMetadata((Map>) null); - assertNotNull(metadata.getData()); - assertTrue(metadata.getData().isEmpty()); - } - - @Test - public void testMapConstructor_withEmptyData() { - Map> data = new HashMap<>(); - SystemMetadata metadata = new SystemMetadata(data); - assertNotNull(metadata.getData()); - assertTrue(metadata.getData().isEmpty()); - } - - @Test - public void testMapConstructor_filtersNullValues() { - Map> data = new HashMap<>(); - Map group1 = new HashMap<>(); - group1.put("key1", "value1".getBytes()); - group1.put("key2", null); - data.put("group1", group1); - data.put("group2", null); - - SystemMetadata metadata = new SystemMetadata(data); - - assertEquals(1, metadata.getData().size()); - assertEquals(1, metadata.getData().get("group1").size()); - assertArrayEquals("value1".getBytes(), metadata.getData().get("group1").get("key1")); - assertNull(metadata.getData().get("group1").get("key2")); - } - - @Test - public void testMapConstructor_preventsMutation() { - Map> data = new HashMap<>(); - Map group1 = new HashMap<>(); - byte[] originalValue = "value1".getBytes(); - group1.put("key1", originalValue); - data.put("group1", group1); - - SystemMetadata metadata = new SystemMetadata(data); - - // Modify original byte array - originalValue[0] = 'X'; - - // Verify metadata is not affected - assertArrayEquals("value1".getBytes(), metadata.getData().get("group1").get("key1")); + assertNotNull(metadata.getGroups()); + assertTrue(metadata.getGroups().isEmpty()); } @Test @@ -102,183 +34,126 @@ public void testProtoConstructor_withValidMetadata() { SystemMetadata metadata = new SystemMetadata(protoMetadata); - assertEquals(2, metadata.getData().size()); - assertArrayEquals("value1".getBytes(), metadata.getData().get("group1").get("key1")); - assertArrayEquals("valueA".getBytes(), metadata.getData().get("group2").get("keyA")); + assertEquals(2, metadata.getGroups().size()); + assertArrayEquals("value1".getBytes(), metadata.getValue("group1" ,"key1")); + assertArrayEquals("valueA".getBytes(), metadata.getValue("group2", "keyA")); } @Test public void testProtoConstructor_withNullMetadata() { SystemMetadata metadata = new SystemMetadata((MetadataOuterClass.Metadata) null); - assertNotNull(metadata.getData()); - assertTrue(metadata.getData().isEmpty()); + assertNotNull(metadata.getGroups()); + assertTrue(metadata.getGroups().isEmpty()); } @Test public void testProtoConstructor_withEmptyMetadata() { MetadataOuterClass.Metadata protoMetadata = MetadataOuterClass.Metadata.newBuilder().build(); SystemMetadata metadata = new SystemMetadata(protoMetadata); - assertNotNull(metadata.getData()); - assertTrue(metadata.getData().isEmpty()); + assertNotNull(metadata.getGroups()); + assertTrue(metadata.getGroups().isEmpty()); } @Test - public void testGetData_returnsDeepCopy() { - Map> inputData = new HashMap<>(); - Map group1 = new HashMap<>(); - group1.put("key1", "value1".getBytes()); - inputData.put("group1", group1); - - SystemMetadata metadata = new SystemMetadata(inputData); - Map> dataCopy = metadata.getData(); - - // Modify the returned copy - dataCopy.put("group2", new HashMap<>()); - dataCopy.get("group1").put("key2", "value2".getBytes()); - dataCopy.get("group1").get("key1")[0] = 'X'; - - // Verify original metadata is not affected - assertEquals(1, metadata.getGroups().size()); - assertEquals(1, metadata.getKeys("group1").size()); - assertArrayEquals("value1".getBytes(), metadata.getValue("group1", "key1")); - } - - @Test - public void testGetData_emptyMetadata() { + public void testGetKeys_withMissingGroup() { SystemMetadata metadata = new SystemMetadata(); - Map> data = metadata.getData(); - - assertNotNull(data); - assertTrue(data.isEmpty()); + assertNotNull(metadata.getKeys("missing")); + assertTrue(metadata.getKeys("missing").isEmpty()); } @Test - public void testGetGroups() { - Map> data = new HashMap<>(); - data.put("group1", new HashMap<>()); - data.put("group2", new HashMap<>()); - data.put("group3", new HashMap<>()); - - SystemMetadata metadata = new SystemMetadata(data); - List groups = metadata.getGroups(); - - assertNotNull(groups); - assertEquals(3, groups.size()); - assertTrue(groups.contains("group1")); - assertTrue(groups.contains("group2")); - assertTrue(groups.contains("group3")); - } + public void testGetKeys_withExistingGroup() { + MetadataOuterClass.KeyValueGroup kvGroup = MetadataOuterClass.KeyValueGroup.newBuilder() + .putKeyValue("key1", ByteString.copyFromUtf8("value1")) + .putKeyValue("key2", ByteString.copyFromUtf8("value2")) + .build(); - @Test - public void testGetGroups_emptyMetadata() { - SystemMetadata metadata = new SystemMetadata(); - List groups = metadata.getGroups(); + MetadataOuterClass.Metadata protoMetadata = MetadataOuterClass.Metadata.newBuilder() + .putSysMetadata("group1", kvGroup) + .build(); - assertNotNull(groups); - assertTrue(groups.isEmpty()); - } + SystemMetadata metadata = new SystemMetadata(protoMetadata); - @Test - public void testGetKeys() { - Map> data = new HashMap<>(); - Map group1 = new HashMap<>(); - group1.put("key1", "value1".getBytes()); - group1.put("key2", "value2".getBytes()); - group1.put("key3", "value3".getBytes()); - data.put("group1", group1); - - SystemMetadata metadata = new SystemMetadata(data); List keys = metadata.getKeys("group1"); - - assertNotNull(keys); - assertEquals(3, keys.size()); + assertEquals(2, keys.size()); assertTrue(keys.contains("key1")); assertTrue(keys.contains("key2")); - assertTrue(keys.contains("key3")); } @Test - public void testGetKeys_nonExistentGroup() { - SystemMetadata metadata = new SystemMetadata(); - List keys = metadata.getKeys("nonExistent"); + public void testGetValue_withMissingKey() { + MetadataOuterClass.KeyValueGroup kvGroup = MetadataOuterClass.KeyValueGroup.newBuilder() + .putKeyValue("key1", ByteString.copyFromUtf8("value1")) + .build(); - assertNotNull(keys); - assertTrue(keys.isEmpty()); + MetadataOuterClass.Metadata protoMetadata = MetadataOuterClass.Metadata.newBuilder() + .putSysMetadata("group1", kvGroup) + .build(); + + SystemMetadata metadata = new SystemMetadata(protoMetadata); + + assertNull(metadata.getValue("group1", "missingKey")); } @Test - public void testGetKeys_emptyGroup() { - Map> data = new HashMap<>(); - data.put("emptyGroup", new HashMap<>()); + public void testGetValue_returnsClone() { + MetadataOuterClass.KeyValueGroup kvGroup = MetadataOuterClass.KeyValueGroup.newBuilder() + .putKeyValue("key1", ByteString.copyFromUtf8("value1")) + .build(); - SystemMetadata metadata = new SystemMetadata(data); - List keys = metadata.getKeys("emptyGroup"); + MetadataOuterClass.Metadata protoMetadata = MetadataOuterClass.Metadata.newBuilder() + .putSysMetadata("group1", kvGroup) + .build(); - assertNotNull(keys); - assertTrue(keys.isEmpty()); - } + SystemMetadata metadata = new SystemMetadata(protoMetadata); - @Test - public void testGetValue() { - Map> data = new HashMap<>(); - Map group1 = new HashMap<>(); - group1.put("key1", "value1".getBytes()); - data.put("group1", group1); + byte[] got = metadata.getValue("group1", "key1"); + assertArrayEquals("value1".getBytes(), got); - SystemMetadata metadata = new SystemMetadata(data); - byte[] value = metadata.getValue("group1", "key1"); + got[0] = 'X'; - assertNotNull(value); - assertArrayEquals("value1".getBytes(), value); + assertArrayEquals("value1".getBytes(), metadata.getValue("group1", "key1")); } @Test - public void testGetValue_nonExistentGroup() { - SystemMetadata metadata = new SystemMetadata(); - byte[] value = metadata.getValue("nonExistent", "key1"); + public void testGetGroups_returnsCopy_notLiveView() { + MetadataOuterClass.KeyValueGroup kvGroup = MetadataOuterClass.KeyValueGroup.newBuilder() + .putKeyValue("key1", ByteString.copyFromUtf8("value1")) + .build(); - assertNull(value); - } + MetadataOuterClass.Metadata protoMetadata = MetadataOuterClass.Metadata.newBuilder() + .putSysMetadata("group1", kvGroup) + .build(); - @Test - public void testGetValue_nonExistentKey() { - Map> data = new HashMap<>(); - Map group1 = new HashMap<>(); - group1.put("key1", "value1".getBytes()); - data.put("group1", group1); + SystemMetadata metadata = new SystemMetadata(protoMetadata); + + List groups = metadata.getGroups(); + assertEquals(1, groups.size()); - SystemMetadata metadata = new SystemMetadata(data); - byte[] value = metadata.getValue("group1", "nonExistent"); + groups.clear(); - assertNull(value); + assertEquals(1, metadata.getGroups().size()); + assertTrue(metadata.getGroups().contains("group1")); } @Test - public void testGetValue_returnsClone() { - Map> data = new HashMap<>(); - Map group1 = new HashMap<>(); - group1.put("key1", "value1".getBytes()); - data.put("group1", group1); + public void testGetKeys_returnsCopy_notLiveView() { + MetadataOuterClass.KeyValueGroup kvGroup = MetadataOuterClass.KeyValueGroup.newBuilder() + .putKeyValue("key1", ByteString.copyFromUtf8("value1")) + .build(); - SystemMetadata metadata = new SystemMetadata(data); - byte[] value = metadata.getValue("group1", "key1"); - value[0] = 'X'; + MetadataOuterClass.Metadata protoMetadata = MetadataOuterClass.Metadata.newBuilder() + .putSysMetadata("group1", kvGroup) + .build(); - // Verify internal data is not affected - assertArrayEquals("value1".getBytes(), metadata.getValue("group1", "key1")); - } + SystemMetadata metadata = new SystemMetadata(protoMetadata); - @Test - public void testGetValue_withEmptyByteArray() { - Map> data = new HashMap<>(); - Map group1 = new HashMap<>(); - group1.put("emptyKey", new byte[0]); - data.put("group1", group1); + List keys = metadata.getKeys("group1"); + assertEquals(1, keys.size()); - SystemMetadata metadata = new SystemMetadata(data); - byte[] value = metadata.getValue("group1", "emptyKey"); + keys.clear(); - assertNotNull(value); - assertEquals(0, value.length); + assertEquals(1, metadata.getKeys("group1").size()); + assertTrue(metadata.getKeys("group1").contains("key1")); } } diff --git a/src/test/java/io/numaproj/numaflow/shared/UserMetadataTest.java b/src/test/java/io/numaproj/numaflow/shared/UserMetadataTest.java index 46c22408..1452659f 100644 --- a/src/test/java/io/numaproj/numaflow/shared/UserMetadataTest.java +++ b/src/test/java/io/numaproj/numaflow/shared/UserMetadataTest.java @@ -15,496 +15,172 @@ public class UserMetadataTest { @Test public void testDefaultConstructor() { UserMetadata metadata = new UserMetadata(); - assertNotNull(metadata.getData()); - assertTrue(metadata.getData().isEmpty()); - } + assertNotNull(metadata.getGroups()); + assertTrue(metadata.getGroups().isEmpty()); - @Test - public void testMapConstructor_withValidData() { - Map> data = new HashMap<>(); - Map group1 = new HashMap<>(); - group1.put("key1", "value1".getBytes()); - group1.put("key2", "value2".getBytes()); - data.put("group1", group1); - - UserMetadata metadata = new UserMetadata(data); - - assertNotNull(metadata.getData()); - assertEquals(1, metadata.getData().size()); - assertTrue(metadata.getData().containsKey("group1")); - assertArrayEquals("value1".getBytes(), metadata.getData().get("group1").get("key1")); - assertArrayEquals("value2".getBytes(), metadata.getData().get("group1").get("key2")); - } + assertNotNull(metadata.getKeys("missing")); + assertTrue(metadata.getKeys("missing").isEmpty()); - @Test - public void testMapConstructor_withNullData() { - UserMetadata metadata = new UserMetadata((Map>) null); - assertNotNull(metadata.getData()); - assertTrue(metadata.getData().isEmpty()); + assertNull(metadata.getValue("missing", "missing")); } @Test - public void testMapConstructor_withEmptyData() { - Map> data = new HashMap<>(); - UserMetadata metadata = new UserMetadata(data); - assertNotNull(metadata.getData()); - assertTrue(metadata.getData().isEmpty()); - } - - @Test - public void testMapConstructor_filtersNullValues() { - Map> data = new HashMap<>(); - Map group1 = new HashMap<>(); - group1.put("key1", "value1".getBytes()); - group1.put("key2", null); - data.put("group1", group1); - data.put("group2", null); - - UserMetadata metadata = new UserMetadata(data); - - assertEquals(1, metadata.getData().size()); - assertEquals(1, metadata.getData().get("group1").size()); - assertArrayEquals("value1".getBytes(), metadata.getData().get("group1").get("key1")); - assertNull(metadata.getData().get("group1").get("key2")); + public void testProtoConstructor_withNullMetadata() { + UserMetadata metadata = new UserMetadata((MetadataOuterClass.Metadata) null); + assertNotNull(metadata.getGroups()); + assertTrue(metadata.getGroups().isEmpty()); } @Test - public void testMapConstructor_preventsMutation() { - Map> data = new HashMap<>(); - Map group1 = new HashMap<>(); - byte[] originalValue = "value1".getBytes(); - group1.put("key1", originalValue); - data.put("group1", group1); - - UserMetadata metadata = new UserMetadata(data); - - // Modify original byte array - originalValue[0] = 'X'; - - // Verify metadata is not affected - assertArrayEquals("value1".getBytes(), metadata.getData().get("group1").get("key1")); + public void testProtoConstructor_withEmptyMetadata() { + MetadataOuterClass.Metadata protoMetadata = MetadataOuterClass.Metadata.newBuilder().build(); + UserMetadata metadata = new UserMetadata(protoMetadata); + assertNotNull(metadata.getGroups()); + assertTrue(metadata.getGroups().isEmpty()); } @Test public void testProtoConstructor_withValidMetadata() { - MetadataOuterClass.KeyValueGroup kvGroup = MetadataOuterClass.KeyValueGroup.newBuilder() + MetadataOuterClass.KeyValueGroup kvGroup1 = MetadataOuterClass.KeyValueGroup.newBuilder() .putKeyValue("key1", ByteString.copyFromUtf8("value1")) - .putKeyValue("key2", ByteString.copyFromUtf8("value2")) + .build(); + + MetadataOuterClass.KeyValueGroup kvGroup2 = MetadataOuterClass.KeyValueGroup.newBuilder() + .putKeyValue("keyA", ByteString.copyFromUtf8("valueA")) .build(); MetadataOuterClass.Metadata protoMetadata = MetadataOuterClass.Metadata.newBuilder() - .putUserMetadata("group1", kvGroup) + .putUserMetadata("group1", kvGroup1) + .putUserMetadata("group2", kvGroup2) .build(); UserMetadata metadata = new UserMetadata(protoMetadata); - assertNotNull(metadata.getData()); - assertEquals(1, metadata.getData().size()); - assertTrue(metadata.getData().containsKey("group1")); - assertArrayEquals("value1".getBytes(), metadata.getData().get("group1").get("key1")); - assertArrayEquals("value2".getBytes(), metadata.getData().get("group1").get("key2")); - } - - @Test - public void testProtoConstructor_withNullMetadata() { - UserMetadata metadata = new UserMetadata((MetadataOuterClass.Metadata) null); - assertNotNull(metadata.getData()); - assertTrue(metadata.getData().isEmpty()); - } - - @Test - public void testProtoConstructor_withEmptyMetadata() { - MetadataOuterClass.Metadata protoMetadata = MetadataOuterClass.Metadata.newBuilder().build(); - UserMetadata metadata = new UserMetadata(protoMetadata); - assertNotNull(metadata.getData()); - assertTrue(metadata.getData().isEmpty()); + assertEquals(2, metadata.getGroups().size()); + assertArrayEquals("value1".getBytes(), metadata.getValue("group1", "key1")); + assertArrayEquals("valueA".getBytes(), metadata.getValue("group2", "keyA")); } @Test - public void testCopyConstructor_withValidUserMetadata() { + public void testCopyConstructor_deepCopy() { UserMetadata original = new UserMetadata(); - original.addKV("group1", "key1", "value1".getBytes()); - original.addKV("group1", "key2", "value2".getBytes()); - original.addKV("group2", "keyA", "valueA".getBytes()); - original.addKV("group2", "keyB", null); + original.addKV("g", "k", "value".getBytes()); UserMetadata copy = new UserMetadata(original); - assertNotNull(copy.getData()); - assertEquals(2, copy.getData().size()); - assertArrayEquals("value1".getBytes(), copy.getData().get("group1").get("key1")); - assertArrayEquals("value2".getBytes(), copy.getData().get("group1").get("key2")); - assertArrayEquals("valueA".getBytes(), copy.getData().get("group2").get("keyA")); - assertNull(copy.getData().get("group2").get("keyB")); - } + assertArrayEquals("value".getBytes(), copy.getValue("g", "k")); - @Test - public void testCopyConstructor_withNullUserMetadata() { - UserMetadata metadata = new UserMetadata((UserMetadata) null); - assertNotNull(metadata.getData()); - assertTrue(metadata.getData().isEmpty()); - } + byte[] fromCopy = copy.getValue("g", "k"); + fromCopy[0] = 'X'; - @Test - public void testCopyConstructor_preventsMutation() { - UserMetadata original = new UserMetadata(); - byte[] originalValue = "value1".getBytes(); - original.addKV("group1", "key1", originalValue); - - UserMetadata copy = new UserMetadata(original); - - // Modify original - original.addKV("group1", "key2", "value2".getBytes()); - - // Verify copy is not affected - assertEquals(1, copy.getData().get("group1").size()); - assertNull(copy.getData().get("group1").get("key2")); + assertArrayEquals("value".getBytes(), original.getValue("g", "k")); + assertArrayEquals("value".getBytes(), copy.getValue("g", "k")); } @Test - public void testToProto() { - UserMetadata metadata = new UserMetadata(); - metadata.addKV("group1", "key1", "value1".getBytes()); - metadata.addKV("group1", "key2", "value2".getBytes()); - metadata.addKV("group2", "keyA", "valueA".getBytes()); - metadata.addKV("group2", "keyB", null); - - MetadataOuterClass.Metadata proto = metadata.toProto(); - - assertNotNull(proto); - assertEquals(2, proto.getUserMetadataMap().size()); - assertTrue(proto.getUserMetadataMap().containsKey("group1")); - assertTrue(proto.getUserMetadataMap().containsKey("group2")); - - MetadataOuterClass.KeyValueGroup group1 = proto.getUserMetadataMap().get("group1"); - assertEquals(ByteString.copyFromUtf8("value1"), group1.getKeyValueMap().get("key1")); - assertEquals(ByteString.copyFromUtf8("value2"), group1.getKeyValueMap().get("key2")); - - MetadataOuterClass.KeyValueGroup group2 = proto.getUserMetadataMap().get("group2"); - assertEquals(ByteString.copyFromUtf8("valueA"), group2.getKeyValueMap().get("keyA")); - assertNull(group2.getKeyValueMap().get("keyB")); - } - - @Test - public void testToProto_emptyMetadata() { - UserMetadata metadata = new UserMetadata(); - MetadataOuterClass.Metadata proto = metadata.toProto(); - - assertNotNull(proto); - assertTrue(proto.getUserMetadataMap().isEmpty()); - } - - @Test - public void testGetData_returnsDeepCopy() { - UserMetadata metadata = new UserMetadata(); - metadata.addKV("group1", "key1", "value1".getBytes()); - - Map> dataCopy = metadata.getData(); - - // Modify the returned copy - dataCopy.put("group2", new HashMap<>()); - dataCopy.get("group1").put("key2", "value2".getBytes()); - dataCopy.get("group1").get("key1")[0] = 'X'; - - // Verify original metadata is not affected - assertEquals(1, metadata.getGroups().size()); - assertEquals(1, metadata.getKeys("group1").size()); - assertArrayEquals("value1".getBytes(), metadata.getValue("group1", "key1")); - } - - @Test - public void testGetData_emptyMetadata() { - UserMetadata metadata = new UserMetadata(); - Map> data = metadata.getData(); - - assertNotNull(data); - assertTrue(data.isEmpty()); + public void testCopyConstructor_withNullInput() { + UserMetadata copy = new UserMetadata((UserMetadata) null); + assertNotNull(copy.getGroups()); + assertTrue(copy.getGroups().isEmpty()); } @Test - public void testGetGroups() { + public void testToProto_roundTrip() { UserMetadata metadata = new UserMetadata(); - metadata.addKV("group1", "key1", "value1".getBytes()); - metadata.addKV("group2", "key2", "value2".getBytes()); - metadata.addKV("group3", "key3", "value3".getBytes()); - - List groups = metadata.getGroups(); - - assertNotNull(groups); - assertEquals(3, groups.size()); - assertTrue(groups.contains("group1")); - assertTrue(groups.contains("group2")); - assertTrue(groups.contains("group3")); - } - - @Test - public void testGetGroups_emptyMetadata() { - UserMetadata metadata = new UserMetadata(); - List groups = metadata.getGroups(); - - assertNotNull(groups); - assertTrue(groups.isEmpty()); - } - - @Test - public void testDeleteGroup() { - UserMetadata metadata = new UserMetadata(); - metadata.addKV("group1", "key1", "value1".getBytes()); - metadata.addKV("group2", "key2", "value2".getBytes()); - - metadata.deleteGroup("group1"); - - assertEquals(1, metadata.getData().size()); - assertFalse(metadata.getData().containsKey("group1")); - assertTrue(metadata.getData().containsKey("group2")); - } - - @Test - public void testDeleteGroup_nonExistentGroup() { - UserMetadata metadata = new UserMetadata(); - metadata.addKV("group1", "key1", "value1".getBytes()); - - // Should not throw exception - metadata.deleteGroup("nonExistent"); - - assertEquals(1, metadata.getData().size()); - } + metadata.addKV("g1", "k1", "v1".getBytes()); + metadata.addKV("g1", "k2", "v2".getBytes()); + metadata.addKV("g2", "ka", "va".getBytes()); - @Test - public void testGetKeys() { - UserMetadata metadata = new UserMetadata(); - metadata.addKV("group1", "key1", "value1".getBytes()); - metadata.addKV("group1", "key2", "value2".getBytes()); - metadata.addKV("group1", "key3", "value3".getBytes()); - - List keys = metadata.getKeys("group1"); - - assertNotNull(keys); - assertEquals(3, keys.size()); - assertTrue(keys.contains("key1")); - assertTrue(keys.contains("key2")); - assertTrue(keys.contains("key3")); - } - - @Test - public void testGetKeys_nonExistentGroup() { - UserMetadata metadata = new UserMetadata(); - - List keys = metadata.getKeys("nonExistent"); - - assertNotNull(keys); - assertTrue(keys.isEmpty()); - } - - @Test - public void testDeleteKey() { - UserMetadata metadata = new UserMetadata(); - metadata.addKV("group1", "key1", "value1".getBytes()); - metadata.addKV("group1", "key2", "value2".getBytes()); - - metadata.deleteKey("group1", "key1"); - - assertEquals(1, metadata.getData().get("group1").size()); - assertNull(metadata.getData().get("group1").get("key1")); - assertNotNull(metadata.getData().get("group1").get("key2")); - } - - @Test - public void testDeleteKey_nonExistentGroup() { - UserMetadata metadata = new UserMetadata(); - - // Should not throw exception - metadata.deleteKey("nonExistent", "key1"); - - assertTrue(metadata.getData().isEmpty()); - } - - @Test - public void testDeleteKey_nonExistentKey() { - UserMetadata metadata = new UserMetadata(); - metadata.addKV("group1", "key1", "value1".getBytes()); - - // Should not throw exception - metadata.deleteKey("group1", "nonExistent"); - - assertEquals(1, metadata.getData().get("group1").size()); - } - - @Test - public void testAddKV() { - UserMetadata metadata = new UserMetadata(); - - metadata.addKV("group1", "key1", "value1".getBytes()); + MetadataOuterClass.Metadata proto = metadata.toProto(); + UserMetadata roundTrip = new UserMetadata(proto); - assertEquals(1, metadata.getData().size()); - assertArrayEquals("value1".getBytes(), metadata.getData().get("group1").get("key1")); + assertEquals(metadata.getGroups().size(), roundTrip.getGroups().size()); + assertArrayEquals("v1".getBytes(), roundTrip.getValue("g1", "k1")); + assertArrayEquals("v2".getBytes(), roundTrip.getValue("g1", "k2")); + assertArrayEquals("va".getBytes(), roundTrip.getValue("g2", "ka")); } @Test - public void testAddKV_nullValue() { + public void testAddKV_ignoresNulls() { UserMetadata metadata = new UserMetadata(); - metadata.addKV("group1", "key1", null); - metadata.addKV(null, "key1", "value1".getBytes()); - metadata.addKV("group1", null, "value1".getBytes()); + metadata.addKV(null, "k", "v".getBytes()); + metadata.addKV("g", null, "v".getBytes()); + metadata.addKV("g", "k", null); - assertTrue(metadata.getData().isEmpty()); + assertTrue(metadata.getGroups().isEmpty()); } @Test - public void testAddKV_preventsMutation() { + public void testAddKV_defensiveCopy() { UserMetadata metadata = new UserMetadata(); - byte[] value = "value1".getBytes(); - metadata.addKV("group1", "key1", value); + byte[] value = "value".getBytes(); + metadata.addKV("g", "k", value); - // Modify original byte array value[0] = 'X'; - // Verify metadata is not affected - assertArrayEquals("value1".getBytes(), metadata.getData().get("group1").get("key1")); + assertArrayEquals("value".getBytes(), metadata.getValue("g", "k")); } @Test - public void testAddKV_overwritesExistingKey() { + public void testGetValue_returnsClone() { UserMetadata metadata = new UserMetadata(); - metadata.addKV("group1", "key1", "value1".getBytes()); - - metadata.addKV("group1", "key1", "newValue".getBytes()); + metadata.addKV("g", "k", "value".getBytes()); - assertArrayEquals("newValue".getBytes(), metadata.getData().get("group1").get("key1")); - } - - @Test - public void testAddKVs() { - UserMetadata metadata = new UserMetadata(); - Map kv = new HashMap<>(); - kv.put("key1", "value1".getBytes()); - kv.put("key2", "value2".getBytes()); + byte[] got = metadata.getValue("g", "k"); + assertArrayEquals("value".getBytes(), got); - metadata.addKVs("group1", kv); + got[0] = 'X'; - assertEquals(1, metadata.getData().size()); - assertEquals(2, metadata.getData().get("group1").size()); - assertArrayEquals("value1".getBytes(), metadata.getData().get("group1").get("key1")); - assertArrayEquals("value2".getBytes(), metadata.getData().get("group1").get("key2")); + assertArrayEquals("value".getBytes(), metadata.getValue("g", "k")); } @Test - public void testAddKVs_filtersNullValues() { + public void testAddKVs_ignoresNullGroupOrMap() { UserMetadata metadata = new UserMetadata(); Map kv = new HashMap<>(); - kv.put("key1", "value1".getBytes()); - kv.put("key2", null); + kv.put("k", "v".getBytes()); - metadata.addKVs("group1", kv); metadata.addKVs(null, kv); - metadata.addKVs("group1", null); + metadata.addKVs("g", null); - assertEquals(1, metadata.getData().get("group1").size()); - assertArrayEquals("value1".getBytes(), metadata.getData().get("group1").get("key1")); - assertNull(metadata.getData().get("group1").get("key2")); + assertTrue(metadata.getGroups().isEmpty()); } @Test - public void testAddKVs_preventsMutation() { - UserMetadata metadata = new UserMetadata(); - byte[] value = "value1".getBytes(); - Map kv = new HashMap<>(); - kv.put("key1", value); - - metadata.addKVs("group1", kv); - - // Modify original byte array - value[0] = 'X'; - - // Verify metadata is not affected - assertArrayEquals("value1".getBytes(), metadata.getData().get("group1").get("key1")); - } - - @Test - public void testGetValue() { - UserMetadata metadata = new UserMetadata(); - metadata.addKV("group1", "key1", "value1".getBytes()); - - byte[] value = metadata.getValue("group1", "key1"); - - assertNotNull(value); - assertArrayEquals("value1".getBytes(), value); - } - - @Test - public void testGetValue_nonExistentGroup() { - UserMetadata metadata = new UserMetadata(); - - byte[] value = metadata.getValue("nonExistent", "key1"); - - assertNull(value); - } - - @Test - public void testGetValue_nonExistentKey() { + public void testAddKVs_filtersNullValues() { UserMetadata metadata = new UserMetadata(); - metadata.addKV("group1", "key1", "value1".getBytes()); - - byte[] value = metadata.getValue("group1", "nonExistent"); - - assertNull(value); - } - @Test - public void testGetValue_returnsClone() { - UserMetadata metadata = new UserMetadata(); - metadata.addKV("group1", "key1", "value1".getBytes()); + Map kv = new HashMap<>(); + kv.put("k1", "v1".getBytes()); + kv.put("k2", null); - byte[] value = metadata.getValue("group1", "key1"); - value[0] = 'X'; + metadata.addKVs("g", kv); - // Verify internal data is not affected - assertArrayEquals("value1".getBytes(), metadata.getValue("group1", "key1")); + List keys = metadata.getKeys("g"); + assertEquals(1, keys.size()); + assertTrue(keys.contains("k1")); + assertArrayEquals("v1".getBytes(), metadata.getValue("g", "k1")); + assertNull(metadata.getValue("g", "k2")); } @Test - public void testClear() { + public void testDeleteKeyAndDeleteGroupAndClear() { UserMetadata metadata = new UserMetadata(); - metadata.addKV("group1", "key1", "value1".getBytes()); - metadata.addKV("group2", "key2", "value2".getBytes()); + metadata.addKV("g1", "k1", "v1".getBytes()); + metadata.addKV("g2", "k2", "v2".getBytes()); - metadata.clear(); + metadata.deleteKey("g1", "k1"); + assertNull(metadata.getValue("g1", "k1")); - assertNotNull(metadata.getData()); - assertTrue(metadata.getData().isEmpty()); - } + metadata.deleteKey("missingGroup", "any"); // no-op - @Test - public void testClear_emptyMetadata() { - UserMetadata metadata = new UserMetadata(); + metadata.deleteGroup("g2"); + assertTrue(metadata.getKeys("g2").isEmpty()); + assertNull(metadata.getValue("g2", "k2")); - // Should not throw exception metadata.clear(); - - assertTrue(metadata.getData().isEmpty()); - } - - @Test - public void testRoundTrip_toProtoAndBack() { - UserMetadata original = new UserMetadata(); - original.addKV("group1", "key1", "value1".getBytes()); - original.addKV("group1", "key2", "value2".getBytes()); - original.addKV("group2", "keyA", "valueA".getBytes()); - - MetadataOuterClass.Metadata proto = original.toProto(); - UserMetadata reconstructed = new UserMetadata(proto); - - assertEquals(original.getData().size(), reconstructed.getData().size()); - assertArrayEquals( - original.getValue("group1", "key1"), - reconstructed.getValue("group1", "key1")); - assertArrayEquals( - original.getValue("group1", "key2"), - reconstructed.getValue("group1", "key2")); - assertArrayEquals( - original.getValue("group2", "keyA"), - reconstructed.getValue("group2", "keyA")); + assertTrue(metadata.getGroups().isEmpty()); } } diff --git a/src/test/java/io/numaproj/numaflow/sinker/ResponseTest.java b/src/test/java/io/numaproj/numaflow/sinker/ResponseTest.java index 0873bf6c..36626cfd 100644 --- a/src/test/java/io/numaproj/numaflow/sinker/ResponseTest.java +++ b/src/test/java/io/numaproj/numaflow/sinker/ResponseTest.java @@ -6,6 +6,8 @@ import io.numaproj.numaflow.sink.v1.SinkOuterClass; import org.junit.Test; +import java.nio.charset.StandardCharsets; +import java.util.Arrays; import java.util.HashMap; import java.util.Map; @@ -17,50 +19,57 @@ public class ResponseTest { @Test public void test_addResponse() { String defaultId = "id"; + // test fallback response creation Response response1 = Response.responseFallback(defaultId); assertEquals(defaultId, response1.getId()); + // test ok response creation Response response2 = Response.responseOK(defaultId); assertEquals(defaultId, response2.getId()); + // test serve response creation Response response3 = Response.responseServe(defaultId, "serve".getBytes()); assertEquals(defaultId, response3.getId()); + // test failure response creation Response response4 = Response.responseFailure(defaultId, "failure"); assertEquals(defaultId, response4.getId()); - Map> userMetadata = new HashMap<>(); - userMetadata.put("group1", new HashMap<>()); + // test onSuccess response creation with on success message containing user metadata and no keys HashMap kvg1 = new HashMap<>(Map.ofEntries( entry("key1", "val1".getBytes()) )); kvg1.put("key2", null); - userMetadata.put("group2", kvg1); - userMetadata.put("group3", null); + UserMetadata userMetadata = new UserMetadata(); + userMetadata.addKV("group1", "key2", null); + userMetadata.addKVs("group2", kvg1); + userMetadata.addKVs("group3", null); Message onSuccessMessage1 = new Message("onSuccessValue".getBytes(), null, new UserMetadata(userMetadata)); Response response5 = Response.responseOnSuccess(defaultId, onSuccessMessage1); assertEquals(defaultId, response5.getId()); - assertEquals("", response5.getOnSuccessMessage().getKeys(0)); + assertNull(response5.getOnSuccessMessage().getKeys()); + assertEquals("onSuccessValue", new String(response5.getOnSuccessMessage().getValue(), StandardCharsets.UTF_8)); + assertEquals(userMetadata.toProto(), response5.getOnSuccessMessage().getUserMetadata().toProto()); + // test onSuccess response creation with on success message containing no user metadata and no keys Message onSuccessMessage2 = new Message("onSuccessValue".getBytes(), null, null); Response response6 = Response.responseOnSuccess(defaultId, onSuccessMessage2); assertEquals(defaultId, response6.getId()); - assertEquals(MetadataOuterClass.Metadata.newBuilder() - .putAllUserMetadata(MetadataOuterClass.Metadata - .getDefaultInstance() - .getUserMetadataMap()).build(), - response6.getOnSuccessMessage().getMetadata()); + assertNull(response6.getOnSuccessMessage().getUserMetadata()); - Message onSuccessMessage3 = new Message(null, "key", null); + // test onSuccess response creation with on success message containing keys but no user metadata or value + Message onSuccessMessage3 = new Message(null, new String[] {"key"}, null); Response response7 = Response.responseOnSuccess(defaultId, onSuccessMessage3); assertEquals(defaultId, response7.getId()); - assertEquals(ByteString.copyFrom("".getBytes()), response7.getOnSuccessMessage().getValue()); - assertEquals("key", response7.getOnSuccessMessage().getKeys(0)); + assertNull(response7.getOnSuccessMessage().getValue()); + assertEquals("key", response7.getOnSuccessMessage().getKeys()[0]); - Response response8 = Response.responseOnSuccess(defaultId, (Message) null); + // test onSuccess response creation with no success message + Response response8 = Response.responseOnSuccess(defaultId, null); assertEquals(defaultId, response8.getId()); assertNull(response8.getOnSuccessMessage()); - Response response9 = Response.responseOnSuccess(defaultId, ( SinkOuterClass.SinkResponse.Result.Message) null); + // + Response response9 = Response.responseOnSuccess(defaultId, null); assertNull(response9.getOnSuccessMessage()); } } diff --git a/src/test/java/io/numaproj/numaflow/sinker/ServerTest.java b/src/test/java/io/numaproj/numaflow/sinker/ServerTest.java index 8f866b8f..4f499bae 100644 --- a/src/test/java/io/numaproj/numaflow/sinker/ServerTest.java +++ b/src/test/java/io/numaproj/numaflow/sinker/ServerTest.java @@ -114,12 +114,10 @@ public void sinkerSuccess() { } // create user metadata to simulate it being passed from upstream - Map> userMetadataMap = Map.ofEntries( - entry(umdGroup, Map.ofEntries( + UserMetadata userMetadataObj = new UserMetadata(); + userMetadataObj.addKVs(umdGroup, Map.ofEntries( entry(umdKey, umdValue.getBytes()) - )) - ); - UserMetadata userMetadataObj = new UserMetadata(userMetadataMap); + )); MetadataOuterClass.Metadata metadata = userMetadataObj.toProto(); SinkOuterClass.SinkRequest.Request request = diff --git a/src/test/java/io/numaproj/numaflow/sourcer/ServerTest.java b/src/test/java/io/numaproj/numaflow/sourcer/ServerTest.java index b97f8675..6c3f05c2 100644 --- a/src/test/java/io/numaproj/numaflow/sourcer/ServerTest.java +++ b/src/test/java/io/numaproj/numaflow/sourcer/ServerTest.java @@ -294,9 +294,8 @@ public TestSourcer() { Instant eventTime = Instant.ofEpochMilli(1000L); // create user metadata - Map> userMetadataMap = new HashMap<>(); - userMetadataMap.put("src-group", Map.of("src-key", "src-value".getBytes())); - UserMetadata userMetadata = new UserMetadata(userMetadataMap); + UserMetadata userMetadata = new UserMetadata(); + userMetadata.addKVs("src-group", Map.of("src-key", "src-value".getBytes())); for (int i = 0; i < 10; i++) { messages.add(new Message( From 635ca0bd5276d5619e94b3b6dcfcfd39a6264f42 Mon Sep 17 00:00:00 2001 From: Vaibhav Tiwari Date: Wed, 28 Jan 2026 14:05:29 -0500 Subject: [PATCH 6/8] Address code review comments Signed-off-by: Vaibhav Tiwari --- .../numaflow/shared/UserMetadata.java | 20 ++++-------- .../io/numaproj/numaflow/sinker/Message.java | 6 ++-- .../io/numaproj/numaflow/sinker/Response.java | 9 +----- .../numaflow/shared/UserMetadataTest.java | 32 +++++++++++++++++++ 4 files changed, 42 insertions(+), 25 deletions(-) diff --git a/src/main/java/io/numaproj/numaflow/shared/UserMetadata.java b/src/main/java/io/numaproj/numaflow/shared/UserMetadata.java index 09780ed3..3a397dfb 100644 --- a/src/main/java/io/numaproj/numaflow/shared/UserMetadata.java +++ b/src/main/java/io/numaproj/numaflow/shared/UserMetadata.java @@ -1,8 +1,6 @@ package io.numaproj.numaflow.shared; import common.MetadataOuterClass; -import lombok.EqualsAndHashCode; -import lombok.Getter; import com.google.protobuf.ByteString; import java.util.ArrayList; @@ -185,18 +183,12 @@ public void addKVs(String group, Map kv) { if (group == null || kv == null) { return; } - - this.data.computeIfAbsent( - group, - k -> kv.entrySet().stream() - .filter(e -> e.getKey() != null && e.getValue() != null) - .collect( - Collectors.toMap( - Map.Entry::getKey, - e -> e.getValue().clone() - ) - ) - ); + Map groupMap = this.data.computeIfAbsent(group, k -> new HashMap<>()); + kv.forEach((key, value) -> { + if (key != null && value != null) { + groupMap.put(key, value.clone()); + } + }); } /** diff --git a/src/main/java/io/numaproj/numaflow/sinker/Message.java b/src/main/java/io/numaproj/numaflow/sinker/Message.java index f3590ded..f803b16c 100644 --- a/src/main/java/io/numaproj/numaflow/sinker/Message.java +++ b/src/main/java/io/numaproj/numaflow/sinker/Message.java @@ -37,9 +37,9 @@ public static Message fromDatum(Datum datum) { return Message.builder().build(); } return Message.builder() - .value(datum.getValue()) - .keys(datum.getKeys()) - .userMetadata(datum.getUserMetadata()) + .value(datum.getValue().clone()) + .keys(datum.getKeys().clone()) + .userMetadata(new UserMetadata(datum.getUserMetadata())) .build(); } diff --git a/src/main/java/io/numaproj/numaflow/sinker/Response.java b/src/main/java/io/numaproj/numaflow/sinker/Response.java index e2765c2a..96123005 100644 --- a/src/main/java/io/numaproj/numaflow/sinker/Response.java +++ b/src/main/java/io/numaproj/numaflow/sinker/Response.java @@ -1,16 +1,9 @@ package io.numaproj.numaflow.sinker; -import com.google.protobuf.ByteString; -import common.MetadataOuterClass; -import io.numaproj.numaflow.sink.v1.SinkOuterClass.SinkResponse; import lombok.AccessLevel; import lombok.AllArgsConstructor; import lombok.Getter; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.stream.Collectors; - /** * Response is used to send response from the user defined sinker. It contains the id of the * message, success status, an optional error message and a fallback status. Various static factory @@ -75,13 +68,13 @@ public static Response responseServe(String id, byte[] serveResponse) { /** * Static method to create response for onSuccess message using the Datum object. + * NOTE: response id is null if datum is null * * @param datum Datum object using which onSuccess message is created. Can be the original datum * @return Response object with onSuccess status and onSuccess message */ public static Response responseOnSuccess(Datum datum) { if (datum == null) { - // NOTE: response id is null if datum is null return new Response(null, false, null, false, false, null, true, null); } return responseOnSuccess(datum.getId(), Message.fromDatum(datum)); diff --git a/src/test/java/io/numaproj/numaflow/shared/UserMetadataTest.java b/src/test/java/io/numaproj/numaflow/shared/UserMetadataTest.java index 1452659f..7520973f 100644 --- a/src/test/java/io/numaproj/numaflow/shared/UserMetadataTest.java +++ b/src/test/java/io/numaproj/numaflow/shared/UserMetadataTest.java @@ -165,6 +165,38 @@ public void testAddKVs_filtersNullValues() { assertNull(metadata.getValue("g", "k2")); } + @Test + public void testAddKVs_toExistingGroup() { + UserMetadata metadata = new UserMetadata(); + + // First, add some initial key-value pairs to a group + Map initialKv = new HashMap<>(); + initialKv.put("k1", "v1".getBytes()); + metadata.addKVs("g", initialKv); + + // Verify initial state + assertEquals(1, metadata.getKeys("g").size()); + assertArrayEquals("v1".getBytes(), metadata.getValue("g", "k1")); + + // Now add more key-value pairs to the same existing group + Map additionalKv = new HashMap<>(); + additionalKv.put("k2", "v2".getBytes()); + additionalKv.put("k3", "v3".getBytes()); + metadata.addKVs("g", additionalKv); + + // Verify all keys are present in the group + List keys = metadata.getKeys("g"); + assertEquals(3, keys.size()); + assertTrue(keys.contains("k1")); + assertTrue(keys.contains("k2")); + assertTrue(keys.contains("k3")); + + // Verify all values + assertArrayEquals("v1".getBytes(), metadata.getValue("g", "k1")); + assertArrayEquals("v2".getBytes(), metadata.getValue("g", "k2")); + assertArrayEquals("v3".getBytes(), metadata.getValue("g", "k3")); + } + @Test public void testDeleteKeyAndDeleteGroupAndClear() { UserMetadata metadata = new UserMetadata(); From ccbdcd0a5624acdbcffdca2555ad2a388c0b4784 Mon Sep 17 00:00:00 2001 From: Vaibhav Tiwari Date: Thu, 29 Jan 2026 18:57:43 -0500 Subject: [PATCH 7/8] Address code review comments Signed-off-by: Vaibhav Tiwari --- .../examples/sink/onsuccess/OnSuccess.java | 13 +- .../io/numaproj/numaflow/sinker/Message.java | 33 ++-- .../io/numaproj/numaflow/sinker/Response.java | 14 -- .../numaflow/shared/SystemMetadataTest.java | 127 +++++++-------- .../numaflow/shared/UserMetadataTest.java | 153 +++++++++--------- 5 files changed, 161 insertions(+), 179 deletions(-) diff --git a/examples/src/main/java/io/numaproj/numaflow/examples/sink/onsuccess/OnSuccess.java b/examples/src/main/java/io/numaproj/numaflow/examples/sink/onsuccess/OnSuccess.java index 0d9b8026..ad92293d 100644 --- a/examples/src/main/java/io/numaproj/numaflow/examples/sink/onsuccess/OnSuccess.java +++ b/examples/src/main/java/io/numaproj/numaflow/examples/sink/onsuccess/OnSuccess.java @@ -46,14 +46,11 @@ public ResponseList processMessages(DatumIterator datumIterator) { log.info("Writing to onSuccess sink: {}", datum.getId()); // Build the onSuccess message using builder for changing values, keys or userMetadata responseListBuilder.addResponse(Response.responseOnSuccess(datum.getId(), - Message.builder() - .value(String.format("Successfully wrote message with ID: %s", - datum.getId()).getBytes()) - .userMetadata(datum.getUserMetadata()) - .keys(datum.getKeys()) - .build())); - // Or send the same values as datum using: - // responseListBuilder.addResponse(Response.responseOnSuccess(datum)); + Message.fromDatum(datum))); + // Or use on-success Message constructor: + // responseListBuilder.addResponse(Response.responseOnSuccess(datum.getId(), + // new Message(String.format("Successfully wrote message with ID: %s", + // datum.getId()).getBytes(), datum.getKeys(), datum.getUserMetadata())))); } else { log.info("Writing to fallback sink: {}", datum.getId()); responseListBuilder.addResponse(Response.responseFallback(datum.getId())); diff --git a/src/main/java/io/numaproj/numaflow/sinker/Message.java b/src/main/java/io/numaproj/numaflow/sinker/Message.java index f803b16c..12158819 100644 --- a/src/main/java/io/numaproj/numaflow/sinker/Message.java +++ b/src/main/java/io/numaproj/numaflow/sinker/Message.java @@ -6,6 +6,7 @@ import io.numaproj.numaflow.sink.v1.SinkOuterClass; import lombok.Builder; import lombok.Getter; +import lombok.NonNull; import java.util.ArrayList; import java.util.Arrays; @@ -15,9 +16,8 @@ * The message can be different from the original message that was sent to primary sink. */ @Getter -@Builder public class Message { - private final byte[] value; + private final byte [] value; private final String[] keys; /** * userMetadata is the user defined metadata that is added to the onSuccess message @@ -25,6 +25,24 @@ public class Message { */ private final UserMetadata userMetadata; + public Message(byte[] value) { + this.value = value; + this.keys = null; + this.userMetadata = null; + } + + public Message(byte [] value, String[] keys) { + this.value = value; + this.keys = keys; + this.userMetadata = null; + } + + public Message(byte [] value, String[] keys, UserMetadata userMetadata) { + this.value = value; + this.keys = keys; + this.userMetadata = userMetadata; + } + /** * Static method to create an onSuccess message from a sinker Datum object. * @@ -34,13 +52,10 @@ public class Message { */ public static Message fromDatum(Datum datum) { if (datum == null) { - return Message.builder().build(); + return new Message(null); } - return Message.builder() - .value(datum.getValue().clone()) - .keys(datum.getKeys().clone()) - .userMetadata(new UserMetadata(datum.getUserMetadata())) - .build(); + + return new Message(datum.getValue().clone(), datum.getKeys().clone(), new UserMetadata(datum.getUserMetadata())); } /** @@ -50,7 +65,7 @@ public static Message fromDatum(Datum datum) { * @param message The message object to convert into the relevant proto object * @return The converted proto object */ - public static SinkOuterClass.SinkResponse.Result.Message toProto(Message message) { + protected static SinkOuterClass.SinkResponse.Result.Message toProto(Message message) { if (message == null) { return SinkOuterClass.SinkResponse.Result.Message.getDefaultInstance(); } diff --git a/src/main/java/io/numaproj/numaflow/sinker/Response.java b/src/main/java/io/numaproj/numaflow/sinker/Response.java index 96123005..f7f57673 100644 --- a/src/main/java/io/numaproj/numaflow/sinker/Response.java +++ b/src/main/java/io/numaproj/numaflow/sinker/Response.java @@ -66,20 +66,6 @@ public static Response responseServe(String id, byte[] serveResponse) { return new Response(id, false, null, false, true, serveResponse, false, null); } - /** - * Static method to create response for onSuccess message using the Datum object. - * NOTE: response id is null if datum is null - * - * @param datum Datum object using which onSuccess message is created. Can be the original datum - * @return Response object with onSuccess status and onSuccess message - */ - public static Response responseOnSuccess(Datum datum) { - if (datum == null) { - return new Response(null, false, null, false, false, null, true, null); - } - return responseOnSuccess(datum.getId(), Message.fromDatum(datum)); - } - /** * Overloaded static method to create response for onSuccess message. Allows creation of onSuccess message * from OnSuccessMessage object. diff --git a/src/test/java/io/numaproj/numaflow/shared/SystemMetadataTest.java b/src/test/java/io/numaproj/numaflow/shared/SystemMetadataTest.java index 100b7c44..bffa52fd 100644 --- a/src/test/java/io/numaproj/numaflow/shared/SystemMetadataTest.java +++ b/src/test/java/io/numaproj/numaflow/shared/SystemMetadataTest.java @@ -18,7 +18,8 @@ public void testDefaultConstructor() { } @Test - public void testProtoConstructor_withValidMetadata() { + public void testProtoConstructor() { + // test with valid metadata MetadataOuterClass.KeyValueGroup kvGroup1 = MetadataOuterClass.KeyValueGroup.newBuilder() .putKeyValue("key1", ByteString.copyFromUtf8("value1")) .build(); @@ -32,71 +33,83 @@ public void testProtoConstructor_withValidMetadata() { .putSysMetadata("group2", kvGroup2) .build(); - SystemMetadata metadata = new SystemMetadata(protoMetadata); + SystemMetadata metadata1 = new SystemMetadata(protoMetadata); - assertEquals(2, metadata.getGroups().size()); - assertArrayEquals("value1".getBytes(), metadata.getValue("group1" ,"key1")); - assertArrayEquals("valueA".getBytes(), metadata.getValue("group2", "keyA")); - } + assertEquals(2, metadata1.getGroups().size()); + assertArrayEquals("value1".getBytes(), metadata1.getValue("group1" ,"key1")); + assertArrayEquals("valueA".getBytes(), metadata1.getValue("group2", "keyA")); - @Test - public void testProtoConstructor_withNullMetadata() { - SystemMetadata metadata = new SystemMetadata((MetadataOuterClass.Metadata) null); - assertNotNull(metadata.getGroups()); - assertTrue(metadata.getGroups().isEmpty()); - } + // test with null metadata + SystemMetadata metadata2 = new SystemMetadata(null); + assertNotNull(metadata2.getGroups()); + assertTrue(metadata2.getGroups().isEmpty()); - @Test - public void testProtoConstructor_withEmptyMetadata() { - MetadataOuterClass.Metadata protoMetadata = MetadataOuterClass.Metadata.newBuilder().build(); - SystemMetadata metadata = new SystemMetadata(protoMetadata); - assertNotNull(metadata.getGroups()); - assertTrue(metadata.getGroups().isEmpty()); + // test with empty metadata + MetadataOuterClass.Metadata protoMetadata2 = MetadataOuterClass.Metadata.newBuilder().build(); + SystemMetadata metadata3 = new SystemMetadata(protoMetadata2); + assertNotNull(metadata3.getGroups()); + assertTrue(metadata3.getGroups().isEmpty()); } @Test - public void testGetKeys_withMissingGroup() { - SystemMetadata metadata = new SystemMetadata(); - assertNotNull(metadata.getKeys("missing")); - assertTrue(metadata.getKeys("missing").isEmpty()); - } + public void testGetKeys() { + // test with missing group + SystemMetadata metadata1 = new SystemMetadata(); + assertNotNull(metadata1.getKeys("missing")); + assertTrue(metadata1.getKeys("missing").isEmpty()); - @Test - public void testGetKeys_withExistingGroup() { - MetadataOuterClass.KeyValueGroup kvGroup = MetadataOuterClass.KeyValueGroup.newBuilder() + // test with existing group + MetadataOuterClass.KeyValueGroup kvGroup1 = MetadataOuterClass.KeyValueGroup.newBuilder() .putKeyValue("key1", ByteString.copyFromUtf8("value1")) .putKeyValue("key2", ByteString.copyFromUtf8("value2")) .build(); - MetadataOuterClass.Metadata protoMetadata = MetadataOuterClass.Metadata.newBuilder() - .putSysMetadata("group1", kvGroup) + MetadataOuterClass.Metadata protoMetadata1 = MetadataOuterClass.Metadata.newBuilder() + .putSysMetadata("group1", kvGroup1) .build(); - SystemMetadata metadata = new SystemMetadata(protoMetadata); + SystemMetadata metadata2 = new SystemMetadata(protoMetadata1); - List keys = metadata.getKeys("group1"); - assertEquals(2, keys.size()); - assertTrue(keys.contains("key1")); - assertTrue(keys.contains("key2")); - } + List keys1 = metadata2.getKeys("group1"); + assertEquals(2, keys1.size()); + assertTrue(keys1.contains("key1")); + assertTrue(keys1.contains("key2")); - @Test - public void testGetValue_withMissingKey() { - MetadataOuterClass.KeyValueGroup kvGroup = MetadataOuterClass.KeyValueGroup.newBuilder() + // test if getKeys returns a copy + MetadataOuterClass.KeyValueGroup kvGroup2 = MetadataOuterClass.KeyValueGroup.newBuilder() .putKeyValue("key1", ByteString.copyFromUtf8("value1")) .build(); - MetadataOuterClass.Metadata protoMetadata = MetadataOuterClass.Metadata.newBuilder() - .putSysMetadata("group1", kvGroup) + MetadataOuterClass.Metadata protoMetadata2 = MetadataOuterClass.Metadata.newBuilder() + .putSysMetadata("group1", kvGroup2) .build(); - SystemMetadata metadata = new SystemMetadata(protoMetadata); + SystemMetadata metadata3 = new SystemMetadata(protoMetadata2); - assertNull(metadata.getValue("group1", "missingKey")); - } + List keys2 = metadata3.getKeys("group1"); + assertEquals(1, keys2.size()); + + keys2.clear(); + assertEquals(1, metadata3.getKeys("group1").size()); + assertTrue(metadata3.getKeys("group1").contains("key1")); + } @Test - public void testGetValue_returnsClone() { + public void testGetValue() { + // test with missing key + MetadataOuterClass.KeyValueGroup kvGroup2 = MetadataOuterClass.KeyValueGroup.newBuilder() + .putKeyValue("key1", ByteString.copyFromUtf8("value1")) + .build(); + + MetadataOuterClass.Metadata protoMetadata1 = MetadataOuterClass.Metadata.newBuilder() + .putSysMetadata("group1", kvGroup2) + .build(); + + SystemMetadata metadata1 = new SystemMetadata(protoMetadata1); + + assertNull(metadata1.getValue("group1", "missingKey")); + + // test if value is cloned MetadataOuterClass.KeyValueGroup kvGroup = MetadataOuterClass.KeyValueGroup.newBuilder() .putKeyValue("key1", ByteString.copyFromUtf8("value1")) .build(); @@ -105,18 +118,18 @@ public void testGetValue_returnsClone() { .putSysMetadata("group1", kvGroup) .build(); - SystemMetadata metadata = new SystemMetadata(protoMetadata); + SystemMetadata metadata2 = new SystemMetadata(protoMetadata); - byte[] got = metadata.getValue("group1", "key1"); + byte[] got = metadata2.getValue("group1", "key1"); assertArrayEquals("value1".getBytes(), got); got[0] = 'X'; - assertArrayEquals("value1".getBytes(), metadata.getValue("group1", "key1")); + assertArrayEquals("value1".getBytes(), metadata2.getValue("group1", "key1")); } @Test - public void testGetGroups_returnsCopy_notLiveView() { + public void testGetGroups() { MetadataOuterClass.KeyValueGroup kvGroup = MetadataOuterClass.KeyValueGroup.newBuilder() .putKeyValue("key1", ByteString.copyFromUtf8("value1")) .build(); @@ -136,24 +149,4 @@ public void testGetGroups_returnsCopy_notLiveView() { assertTrue(metadata.getGroups().contains("group1")); } - @Test - public void testGetKeys_returnsCopy_notLiveView() { - MetadataOuterClass.KeyValueGroup kvGroup = MetadataOuterClass.KeyValueGroup.newBuilder() - .putKeyValue("key1", ByteString.copyFromUtf8("value1")) - .build(); - - MetadataOuterClass.Metadata protoMetadata = MetadataOuterClass.Metadata.newBuilder() - .putSysMetadata("group1", kvGroup) - .build(); - - SystemMetadata metadata = new SystemMetadata(protoMetadata); - - List keys = metadata.getKeys("group1"); - assertEquals(1, keys.size()); - - keys.clear(); - - assertEquals(1, metadata.getKeys("group1").size()); - assertTrue(metadata.getKeys("group1").contains("key1")); - } } diff --git a/src/test/java/io/numaproj/numaflow/shared/UserMetadataTest.java b/src/test/java/io/numaproj/numaflow/shared/UserMetadataTest.java index 7520973f..526053bd 100644 --- a/src/test/java/io/numaproj/numaflow/shared/UserMetadataTest.java +++ b/src/test/java/io/numaproj/numaflow/shared/UserMetadataTest.java @@ -25,22 +25,18 @@ public void testDefaultConstructor() { } @Test - public void testProtoConstructor_withNullMetadata() { - UserMetadata metadata = new UserMetadata((MetadataOuterClass.Metadata) null); - assertNotNull(metadata.getGroups()); - assertTrue(metadata.getGroups().isEmpty()); - } - - @Test - public void testProtoConstructor_withEmptyMetadata() { - MetadataOuterClass.Metadata protoMetadata = MetadataOuterClass.Metadata.newBuilder().build(); - UserMetadata metadata = new UserMetadata(protoMetadata); - assertNotNull(metadata.getGroups()); - assertTrue(metadata.getGroups().isEmpty()); - } - - @Test - public void testProtoConstructor_withValidMetadata() { + public void testProtoConstructor() { + UserMetadata metadata1 = new UserMetadata((MetadataOuterClass.Metadata) null); + assertNotNull(metadata1.getGroups()); + assertTrue(metadata1.getGroups().isEmpty()); + + // test with empty metadata + MetadataOuterClass.Metadata protoMetadata1 = MetadataOuterClass.Metadata.newBuilder().build(); + UserMetadata metadata2 = new UserMetadata(protoMetadata1); + assertNotNull(metadata2.getGroups()); + assertTrue(metadata2.getGroups().isEmpty()); + + // test with valid metadata MetadataOuterClass.KeyValueGroup kvGroup1 = MetadataOuterClass.KeyValueGroup.newBuilder() .putKeyValue("key1", ByteString.copyFromUtf8("value1")) .build(); @@ -49,39 +45,38 @@ public void testProtoConstructor_withValidMetadata() { .putKeyValue("keyA", ByteString.copyFromUtf8("valueA")) .build(); - MetadataOuterClass.Metadata protoMetadata = MetadataOuterClass.Metadata.newBuilder() + MetadataOuterClass.Metadata protoMetadata2 = MetadataOuterClass.Metadata.newBuilder() .putUserMetadata("group1", kvGroup1) .putUserMetadata("group2", kvGroup2) .build(); - UserMetadata metadata = new UserMetadata(protoMetadata); + UserMetadata metadata3 = new UserMetadata(protoMetadata2); - assertEquals(2, metadata.getGroups().size()); - assertArrayEquals("value1".getBytes(), metadata.getValue("group1", "key1")); - assertArrayEquals("valueA".getBytes(), metadata.getValue("group2", "keyA")); + assertEquals(2, metadata3.getGroups().size()); + assertArrayEquals("value1".getBytes(), metadata3.getValue("group1", "key1")); + assertArrayEquals("valueA".getBytes(), metadata3.getValue("group2", "keyA")); } @Test - public void testCopyConstructor_deepCopy() { + public void testCopyConstructor() { + // Deep copy test UserMetadata original = new UserMetadata(); original.addKV("g", "k", "value".getBytes()); - UserMetadata copy = new UserMetadata(original); + UserMetadata copy1 = new UserMetadata(original); - assertArrayEquals("value".getBytes(), copy.getValue("g", "k")); + assertArrayEquals("value".getBytes(), copy1.getValue("g", "k")); - byte[] fromCopy = copy.getValue("g", "k"); + byte[] fromCopy = copy1.getValue("g", "k"); fromCopy[0] = 'X'; assertArrayEquals("value".getBytes(), original.getValue("g", "k")); - assertArrayEquals("value".getBytes(), copy.getValue("g", "k")); - } + assertArrayEquals("value".getBytes(), copy1.getValue("g", "k")); - @Test - public void testCopyConstructor_withNullInput() { - UserMetadata copy = new UserMetadata((UserMetadata) null); - assertNotNull(copy.getGroups()); - assertTrue(copy.getGroups().isEmpty()); + // test with null input + UserMetadata copy2 = new UserMetadata((UserMetadata) null); + assertNotNull(copy2.getGroups()); + assertTrue(copy2.getGroups().isEmpty()); } @Test @@ -101,30 +96,29 @@ public void testToProto_roundTrip() { } @Test - public void testAddKV_ignoresNulls() { - UserMetadata metadata = new UserMetadata(); + public void testAddKV() { + // test with nulls + UserMetadata metadata1 = new UserMetadata(); - metadata.addKV(null, "k", "v".getBytes()); - metadata.addKV("g", null, "v".getBytes()); - metadata.addKV("g", "k", null); + metadata1.addKV(null, "k", "v".getBytes()); + metadata1.addKV("g", null, "v".getBytes()); + metadata1.addKV("g", "k", null); - assertTrue(metadata.getGroups().isEmpty()); - } + assertTrue(metadata1.getGroups().isEmpty()); - @Test - public void testAddKV_defensiveCopy() { - UserMetadata metadata = new UserMetadata(); + // test defensive copy + UserMetadata metadata2 = new UserMetadata(); byte[] value = "value".getBytes(); - metadata.addKV("g", "k", value); + metadata2.addKV("g", "k", value); value[0] = 'X'; - assertArrayEquals("value".getBytes(), metadata.getValue("g", "k")); + assertArrayEquals("value".getBytes(), metadata2.getValue("g", "k")); } @Test - public void testGetValue_returnsClone() { + public void testGetValue() { UserMetadata metadata = new UserMetadata(); metadata.addKV("g", "k", "value".getBytes()); @@ -137,64 +131,61 @@ public void testGetValue_returnsClone() { } @Test - public void testAddKVs_ignoresNullGroupOrMap() { - UserMetadata metadata = new UserMetadata(); - Map kv = new HashMap<>(); - kv.put("k", "v".getBytes()); + public void testAddKVs() { + // test with nulls + UserMetadata metadata1 = new UserMetadata(); + Map kv1 = new HashMap<>(); + kv1.put("k", "v".getBytes()); - metadata.addKVs(null, kv); - metadata.addKVs("g", null); + metadata1.addKVs(null, kv1); + metadata1.addKVs("g", null); - assertTrue(metadata.getGroups().isEmpty()); - } + assertTrue(metadata1.getGroups().isEmpty()); - @Test - public void testAddKVs_filtersNullValues() { - UserMetadata metadata = new UserMetadata(); + // test filters null values + UserMetadata metadata2 = new UserMetadata(); - Map kv = new HashMap<>(); - kv.put("k1", "v1".getBytes()); - kv.put("k2", null); + Map kv2 = new HashMap<>(); + kv2.put("k1", "v1".getBytes()); + kv2.put("k2", null); - metadata.addKVs("g", kv); + metadata2.addKVs("g", kv2); - List keys = metadata.getKeys("g"); - assertEquals(1, keys.size()); - assertTrue(keys.contains("k1")); - assertArrayEquals("v1".getBytes(), metadata.getValue("g", "k1")); - assertNull(metadata.getValue("g", "k2")); - } + List keys1 = metadata2.getKeys("g"); + assertEquals(1, keys1.size()); + assertTrue(keys1.contains("k1")); + assertArrayEquals("v1".getBytes(), metadata2.getValue("g", "k1")); + assertNull(metadata2.getValue("g", "k2")); - @Test - public void testAddKVs_toExistingGroup() { - UserMetadata metadata = new UserMetadata(); + // test add to existing group + UserMetadata metadata3 = new UserMetadata(); // First, add some initial key-value pairs to a group Map initialKv = new HashMap<>(); initialKv.put("k1", "v1".getBytes()); - metadata.addKVs("g", initialKv); + metadata3.addKVs("g", initialKv); // Verify initial state - assertEquals(1, metadata.getKeys("g").size()); - assertArrayEquals("v1".getBytes(), metadata.getValue("g", "k1")); + assertEquals(1, metadata3.getKeys("g").size()); + assertArrayEquals("v1".getBytes(), metadata3.getValue("g", "k1")); // Now add more key-value pairs to the same existing group Map additionalKv = new HashMap<>(); additionalKv.put("k2", "v2".getBytes()); additionalKv.put("k3", "v3".getBytes()); - metadata.addKVs("g", additionalKv); + metadata3.addKVs("g", additionalKv); // Verify all keys are present in the group - List keys = metadata.getKeys("g"); - assertEquals(3, keys.size()); - assertTrue(keys.contains("k1")); - assertTrue(keys.contains("k2")); - assertTrue(keys.contains("k3")); + List keys2 = metadata3.getKeys("g"); + assertEquals(3, keys2.size()); + assertTrue(keys2.contains("k1")); + assertTrue(keys2.contains("k2")); + assertTrue(keys2.contains("k3")); // Verify all values - assertArrayEquals("v1".getBytes(), metadata.getValue("g", "k1")); - assertArrayEquals("v2".getBytes(), metadata.getValue("g", "k2")); - assertArrayEquals("v3".getBytes(), metadata.getValue("g", "k3")); + assertArrayEquals("v1".getBytes(), metadata3.getValue("g", "k1")); + assertArrayEquals("v2".getBytes(), metadata3.getValue("g", "k2")); + assertArrayEquals("v3".getBytes(), metadata3.getValue("g", "k3")); } @Test From b5b5297bbab2e7c5ab3c68842cd6f2d1e223efcf Mon Sep 17 00:00:00 2001 From: Vaibhav Tiwari Date: Sat, 31 Jan 2026 14:04:21 -0500 Subject: [PATCH 8/8] Update javadoc for on success message constructors Signed-off-by: Vaibhav Tiwari --- .../io/numaproj/numaflow/sinker/Message.java | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/main/java/io/numaproj/numaflow/sinker/Message.java b/src/main/java/io/numaproj/numaflow/sinker/Message.java index 12158819..0bc1d5ae 100644 --- a/src/main/java/io/numaproj/numaflow/sinker/Message.java +++ b/src/main/java/io/numaproj/numaflow/sinker/Message.java @@ -25,18 +25,36 @@ public class Message { */ private final UserMetadata userMetadata; + /** + * Constructor to create a Message object with only value. + * + * @param value The value of the message + */ public Message(byte[] value) { this.value = value; this.keys = null; this.userMetadata = null; } + /** + * Constructor to create a Message object with value and keys. + * + * @param value The value of the message + * @param keys The keys of the message + */ public Message(byte [] value, String[] keys) { this.value = value; this.keys = keys; this.userMetadata = null; } + /** + * Constructor to create a Message object with value, keys and userMetadata. + * + * @param value The value of the message + * @param keys The keysof the message + * @param userMetadata The user metadata of the message + */ public Message(byte [] value, String[] keys, UserMetadata userMetadata) { this.value = value; this.keys = keys;