tests,schema_reader: kafka message handling error tests

[nosahama]

About this change - What it does

We add tests to validate the possible error cases while parsing the message key:

• we test for invalid JSON for key
• missing keytype in the key data
• we test for invalid keytype
• we test for invalid config value
• we test for invalid subject delete value
• we test for invalid version number within schema value
• we test for generic invalid protobuf schema

Why this way

This is not added via the integration tests as we are verifying the side-effects which are then handled by other components and thus the unit tests are fine just to add.

The work requires the work currently done in #936 and thus was tested locally with a rebase on that branch, some linting and added tests might fail until we merge the other PR.

[davide-armand]

Should the base branch of this PR be #936 's branch , since it depends on it?

[nosahama]

Should the base branch of this PR be #936 's branch , since it depends on it?

Yes I totally get your point, I had already started off that work and thus the rebase, ideally when this is merged to main, the commits will be the ideal count.

[eliax1996]

Excellent! Other than that I think its fine!

[Claudenw]

Do we not log all messages associated with Errors? If not, should we? Also nit: "doesnt" should be "doesn't" or "does not" which I think is clearer. Claude

…

On Fri, Sep 6, 2024 at 10:59 AM Emmanuel Evbuomwan ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In karapace/schema_reader.py <#941 (comment)>: > @@ -487,7 +487,7 @@ def _handle_msg_config(self, key: dict, value: dict | None) -> None: def _handle_msg_delete_subject(self, key: dict, value: dict | None) -> None: # pylint: disable=unused-argument if value is None: LOG.warning("DELETE_SUBJECT record doesnt have a value, should have") - return + raise ValueError("DELETE_SUBJECT record doesnt have a value, should have") Yes I agree, i tried not to change most of these. — Reply to this email directly, view it on GitHub <#941 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AASTVHQKBPQOWZWRUKOHUS3ZVF4INAVCNFSM6AAAAABNKLES2GVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDEOBVG43DKMBUGM> . You are receiving this because your review was requested.Message ID: ***@***.***>

-- LinkedIn: http://www.linkedin.com/in/claudewarren

[nosahama]

Do we not log all messages associated with Errors? If not, should we? Also nit: "doesnt" should be "doesn't" or "does not" which I think is clearer. Claude
…
On Fri, Sep 6, 2024 at 10:59 AM Emmanuel Evbuomwan @.> wrote: @.* commented on this pull request. ------------------------------ In karapace/schema_reader.py <#941 (comment)>: > @@ -487,7 +487,7 @@ def _handle_msg_config(self, key: dict, value: dict | None) -> None: def _handle_msg_delete_subject(self, key: dict, value: dict | None) -> None: # pylint: disable=unused-argument if value is None: LOG.warning("DELETE_SUBJECT record doesnt have a value, should have") - return + raise ValueError("DELETE_SUBJECT record doesnt have a value, should have") Yes I agree, i tried not to change most of these. — Reply to this email directly, view it on GitHub <#941 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AASTVHQKBPQOWZWRUKOHUS3ZVF4INAVCNFSM6AAAAABNKLES2GVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDEOBVG43DKMBUGM . You are receiving this because your review was requested.Message ID: @.***>
-- LinkedIn: http://www.linkedin.com/in/claudewarren

Yes indeed, we should log all errors, we can check which ones we do not currently log and log those as a follow up task, as for the typo, I was trying my best not to change much of what already exists.

[eliax1996]

LGTM