Added support for proto encoding in set

[RamonQuerol]

Hi,
Previously it was impossible to send data in proto encoding with set because json.dumps() would fail when given bytes (See #126). I've changed the construct_update_message function so that it expects bytes when the encoding is "proto" or "bytes" (And updated the docstring of set to inform the user about this behavior). I'm aware this changes a bit the way set works, so I'm open to discussion if you don't like this approach.

On a side note, I also denested a bit the function with some guard clauses. Since this is more of a stylistic thing I made it its own commit so that we could undo it if you don't like that part :).

[akarneliuk]

Hey @RamonQuerol , thanks for submission?
Can you please share any testing before/after you added proposed change. You are most welcome to update tests: https://github.com/akarneliuk/pygnmi/tree/master/tests

[RamonQuerol]

Note: I want to apologize beforehand for the length of the message, while I think is beneficial for the conversation, I understand that this will probably consume quite a lot of your time, so take as long as you need.

Hi @akarneliuk,
Sorry for not sharing more information on my first post about how the change affects the functionality of the client (The before and after). Since I first posted this pull request I have come to realize that maybe this is not the best way to approach this issue (mainly due to backwards compatibility), so before applying any changes or adding any tests I would like to give you quick* rundown about the issue I am trying to solve, the reasoning behind the solution I proposed, the problems that this solution might arise and, finally, a proposal of a less intrusive solution. But first things first, what's the issue:

For context, in the place where I work we are exploring the idea of starting to use protobuf as the encoding for the connection between the clients and our personal gnmi server. So I checked the specification and saw that this was possible, either using the bytes_val (Vendor specific arbitrary sequence of bytes) or the experimental proto_bytes (Protobuf encoded bytes). Then I just needed to add the protobuf encoded data in these fields.

As you already know, protobuf encoding is dependant on the specific protobuf schema you are using, and therefore, we can not expect pygnmi to adapt to every single protobuf model out there, it only make sense that if I were to use this encoding i should encode the message first and then send the byte data to the pygnmi client. So i did just that:

# qkd_pb2 == python objects obtained from our model
# sdqkd_node == root of our model
data = ParseDict(self._update_data["sdqkd_node"], qkd_pb2.SdkQkdNode())

# data.SerializeToString() results in bytes
gnmi_client.set(update=[(self._data_path, data.SerializeToString())]) 

And there is the problem, if I try to send these bytes using pygnmi an exception will be raised due to a json.dumps() not being able to parse the byte sequence. Specifically the json.dumps (line 1510 of client.py) in construct_update_message():

def construct_update_message(user_list: list, encoding: str) -> list:
"""This is a helper method to construct the Update() GNMI message"""
result = []

if isinstance(user_list, list):
	for ue in user_list:
		if isinstance(ue, tuple):
			u_path = gnmi_path_generator(ue[0])
			u_val = json.dumps(ue[1]).encode("utf-8") # <= THIS LINE
			u_ascii_val = str(ue[1]).encode("utf-8")
			encoding = encoding.lower()
			if encoding == "json":
				result.append(Update(path=u_path, val=TypedValue(json_val=u_val)))
			elif encoding == "bytes":

My proposed solution to this problem (the one that you can currently see in the commit) was something similar to what was made in #153, separate the output for bytes_val and proto_bytes from the one that parses the json. This made sense because as I said earlier these fields where made for non-JSON encoded bytes, therefore, if you are using them you don't expect your data to be able to be parsed as a JSON:

# Current state of my code 
		u_path = gnmi_path_generator(ue[0])
		encoding = encoding.lower() # Normalize to lower case
		if encoding == "json":
			val=TypedValue(json_val=_encode_update_value(ue[1], "json"))
		elif encoding == "bytes":
			val=TypedValue(bytes_val=_encode_update_value(ue[1], "bytes"))
		elif encoding == "proto":
			val=TypedValue(proto_bytes=_encode_update_value(ue[1], "bytes"))
		elif encoding == "ascii":
			val=TypedValue(ascii_val=_encode_update_value(ue[1], "ascii"))
		elif encoding == "json_ietf":
			val=TypedValue(json_ietf_val=_encode_update_value(ue[1], "json"))
		else:
			raise ValueError(f"Unsupported encoding: '{encoding}'")
		
		result.append(Update(path=u_path, val=val))
	
	return result

def _encode_update_value(update_value: dict | str | bytes, encoding: str) -> bytes:
	"""This is a helper function to encode the diferent formats of the update value into bytes"""
	if encoding == "json":
		return json.dumps(update_value).encode("utf-8")
	
	if encoding == "ascii":
		return str(update_value).encode("utf-8")
	
	return update_value

Note: If we keep this code I have to make a change in which before the last line (return update_value) it checks if it is of type bytes.

Applying this solution would mean that everytime you use bytes/proto encoding you have encode first and then make the set() call. Although this works as the specification intends to, after thinking about it for a while, I myself am not sure if it is the best course of action to solve the problem, since it would create some backward compatibility issues for anyone who was using these encodings expecting the json.dumps() to happen. An alternative I think it may be better would be:

	u_path = gnmi_path_generator(ue[0])
	
	if isinstance(ue[1], bytes):
		u_val = ue[1]
	else:
		u_val = json.dumps(ue[1]).encode("utf-8")
	
	u_ascii_val = str(ue[1]).encode("utf-8")
	encoding = encoding.lower() # Normalize to lower case
	if encoding == "json":
		val=TypedValue(json_val=u_val)
	elif encoding == "bytes":
		val=TypedValue(bytes_val=u_val)
	elif encoding == "proto":
		val=TypedValue(proto_bytes=u_val)
	elif encoding == "ascii":
		val=TypedValue(ascii_val=u_ascii_val)
	elif encoding == "json_ietf":
		val=TypedValue(json_ietf_val=u_val)
	else:
		raise ValueError(f"Unsupported encoding: '{encoding}'")
	
	result.append(Update(path=u_path, val=val))

return result

This solves the issue of set() not accepting pre-encoded messages without changing its behavior for anything that worked before. It even comes with the added benefit that now set() has a safeguard in case users pre-encode json data.

So yeah, I'm a really sorry for the length of this response (and for not directly answering your question), I thought this explanation may get you to understand better the cases I am referring to and the posible solutions we could apply. I wait for you opinion before doing anything and when you decide which approach is best, I'll start adding the changes and/or tests you see fit.

Thank you for your time.

References:

Bytes in gnmi: https://github.com/openconfig/reference/blob/master/rpc/gnmi/gnmi-specification.md#232-bytes

Protobuf being able to use byte_val field: https://github.com/openconfig/reference/blob/master/rpc/gnmi/gnmi-specification.md#223-node-values

proto_bytes typedvalue specification: https://github.com/openconfig/reference/blob/master/rpc/gnmi/protobuf-vals.md#encoding-in-typedvalue