Implement Protobuf support

[amrutha-shanbhag]

About this change - What it does

• Validating, storing, and versioning Protobuf schemas
• Protobuf schema evolution and compatibility
• Protobuf messages serialization/deserialization

References: #67

Why this way

Design document outlining design decisions: https://github.com/instaclustr/karapace/pull/11/files?short_path=45bec23#diff-45bec23eda89f362ca9cda50a6be0477f6058d4e6b9cc6c5198cdc5f0488df77

[libretto]

@hackaugusto please see my comments... there are a few issues not finished yet.

[amrutha-shanbhag]

Hi @hackaugusto ,

@libretto has addressed most of your feedback and left some comments of his own. Please let us know if there is any more feedback.

Thanks

[hackaugusto]

I added some comments, but those can be implemented as follow up PRs. Thank you for you contribution.

[hackaugusto]

These are the things that come up to mind:

• Hashable objects should be implemented using the __hash__ method ref
• Hashing and equality should take into account all the attributes of an object, here only self.string is being used. It should also include is_map, is_scalar, key_type, and value_type. Without these attributes the result of the equality is not reliable
• Hashable objects must have a constant hash result, that means they must be immutable. This implementation is on a mutable object.

[hackaugusto]

could you please remove the TODOs that are implemented and create issues for the ones that need further work?

[hackaugusto]

Mutable objects are not allowed as default values (note the error bellow is raised by the standard library but not by the attr implementation):

>>> from dataclasses import dataclass
>>> 
>>> @dataclass
... class A:
...     f: list[str] = []
... 
Traceback (most recent call last):
  File "<stdin>", line 2, in <module>
  File "/usr/lib64/python3.9/dataclasses.py", line 1021, in dataclass
    return wrap(cls)
  File "/usr/lib64/python3.9/dataclasses.py", line 1013, in wrap
    return _process_class(cls, init, repr, eq, order, unsafe_hash, frozen)
  File "/usr/lib64/python3.9/dataclasses.py", line 863, in _process_class
    cls_fields = [_get_field(cls, name, type)
  File "/usr/lib64/python3.9/dataclasses.py", line 863, in <listcomp>
    cls_fields = [_get_field(cls, name, type)
  File "/usr/lib64/python3.9/dataclasses.py", line 747, in _get_field
    raise ValueError(f'mutable default {type(f.default)} for field '
ValueError: mutable default <class 'list'> for field f is not allowed: use default_factory

this must be implement with a field_factory:

Suggested change

	options: List[OptionElement] = []
	options: List[OptionElement] = field(default_factory=list)

[hackaugusto]

I know you didn't introduce this variable in your PR, but this is dead code, should be removed.

[hackaugusto]

So there may be None in writer_schema or reader_schema.

This exception is only raised here:

https://github.com/aiven/karapace/pull/296/files#diff-382cb7b49ec14a61d73c973d5e7e44e6395d89e9bf0eed062635b69797e52837R75-R77

Neither of the arguments can be None in that function (assuming the type annotations used there are correct)

And that's why we use str() call to avoid passing None to pretty_print_json

This may make the type correct, but it is not valid JSON as per the snippet on my previous comment.

So the type here shouldn't be Optional

[hackaugusto]

Please remove the type ignore, the type of other should be object (this is part of the language, since any two objects can be compared)

Suggested change

	def __eq__(self, other: 'ProtoFileElement') -> bool: # type: ignore
	def __eq__(self, other: object) -> bool:

[hackaugusto]

This needs to check the type of other:

Suggested change

	a = self.to_schema()
	b = other.to_schema()
	return a == b
	if not isinstance(other, ProtoFileElement):
	return False
	return self.to_schema() == other.to_schema()

[hackaugusto]

At least my pylint still accepts it without any messages

This is an anti pattern for tools like pylint. pylint is a static analyzer, it can't run the code for analysis, so code like this results in false negatives. Here is an example that pylint doesn't caught because the @static_init annotation is missing and it can't know that:

class R:
    @classmethod
    def static_init(cls) -> None:
        cls.A = "a"


print(R().A)

So i prefer to add this change to the next stage of a development cycle.

Fair enough 👍

[hackaugusto]

Is this TODO necessary?

[hackaugusto]

which exceptions? maybe remove the todo?

[amrutha-shanbhag]

I added some comments, but those can be implemented as follow up PRs. Thank you for you contribution.

thank you @hackaugusto :)