Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: payload util methods #11

Closed
wants to merge 2 commits into from
Closed

feat: payload util methods #11

wants to merge 2 commits into from

Conversation

Anush008
Copy link
Member

@Anush008 Anush008 commented Dec 14, 2023

This PR intends to payload util methods and unit tests for easy interop between java maps. Supports nested maps and arrays.

Example usage:

Map<String, Object> map = new HashMap<>();
map.put("name", "John Doe");
map.put("age", 42);
map.put("married", true);

Map<String, Value> payload = PayloadUtil.toPayload(map);

Reciprocal

Map<String, Object> mapAgain = PayloadUtil.toMap(payload);

@Anush008 Anush008 requested a review from russcam December 14, 2023 04:16
Copy link
Collaborator

@russcam russcam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little weary of adding utility methods that don't control invariants in public packages. For example,

toPayload(Map<String, Object> inputMap) { ... }

Allows any Object entry value. Consequently,

  1. It silently ignores types that aren't supported by Value (it could throw instead)
  2. It throws for Maps that have entry keys other than String

With the methods in ValueFactory, it's a little more convenient to build payloads e.g.

Map<String, Value> map = Map.of(
  "foo", value("bar"),
  "baz", value(1),
  "quux", list(List.of(value(true), value(false)))
);

Do you feel strongly about supporting conversion to/from Map<String,Object> in addition? If you do, what do you think about a specialized PayloadMap type with overloaded methods for the supported types?

* @return The converted payload map.
*/
public static Map<String, Value> toPayload(Map<String, Object> inputMap) {
Map<String, Value> map = new HashMap<>();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

initialize with the expected size (avoid allocation on resizing)

Suggested change
Map<String, Value> map = new HashMap<>();
Map<String, Value> map = new HashMap<>(inputMap.size());

* @return The converted map.
*/
public static Map<String, Object> toMap(Map<String, Value> payload) {
Map<String, Object> hashMap = new HashMap<>();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

initialize with the expected size (avoid allocation on resizing)

Suggested change
Map<String, Object> hashMap = new HashMap<>();
Map<String, Object> hashMap = new HashMap<>(payload.size());

* @param valueBuilder The Value.Builder to set the value for.
* @param value The object value to set.
*/
static void setValue(Value.Builder valueBuilder, Object value) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYT about making this function pure?

Suggested change
static void setValue(Value.Builder valueBuilder, Object value) {
static Value toValue(Object value) {

* @return The converted array of objects.
*/
static Object listValueToList(ListValue listValue) {
return listValue.getValuesList().stream().map(PayloadUtil::valueToObject).toArray();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would a List<T> be more appropriate? It would be congruent with the inverse operation, List<T> -> ListValue

Suggested change
return listValue.getValuesList().stream().map(PayloadUtil::valueToObject).toArray();
return listValue.getValuesList().stream().map(PayloadUtil::valueToObject).collect(Collectors.toList());

valueBuilder.setDoubleValue((Double) value);
} else if (value instanceof Boolean) {
valueBuilder.setBoolValue((Boolean) value);
} else if (value instanceof Map) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will throw an exception for any key that isn't a String

@Anush008
Copy link
Member Author

I'm a little weary of adding utility methods that don't control invariants in public packages. For example,

toPayload(Map<String, Object> inputMap) { ... }

Allows any Object entry value. Consequently,

  1. It silently ignores types that aren't supported by Value (it could throw instead)
  2. It throws for Maps that have entry keys other than String

With the methods in ValueFactory, it's a little more convenient to build payloads e.g.

Map<String, Value> map = Map.of(
  "foo", value("bar"),
  "baz", value(1),
  "quux", list(List.of(value(true), value(false)))
);

Do you feel strongly about supporting conversion to/from Map<String,Object> in addition? If you do, what do you think about a specialized PayloadMap type with overloaded methods for the supported types?

Great points. I too now don't see any clear benefit of using this utility with exceptions over the Map.of() usage you presented.

@Anush008
Copy link
Member Author

Closing with reference to #11 (comment).

@Anush008 Anush008 closed this Dec 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants