fix: align transitive compatibility checks #953

davide-armand · 2024-09-18T10:00:54Z

About this change - What it does

Make so that the transitive compatibility checks that are done in the
schema creation endpoint are also done in the schema validation
endpoint.

In the creation endpoint (/subjects/<subject-key>/versions), if the
compatibility mode is transient then the new schema is checked against
all schemas.

In the validation endpoint (/compatibility/subjects/<subject-key>/versions/<schema-version>):

Before this fix, only the latest schema is checked against (even in
case of transitive mode)
After this fix, in case of transitive mode then all schema are
checked against.
Note that in this case the version provided in the POST request
(<schema-version>) is ignored.

Also includes some preparation refactorings and some additional tests in separate commits.

Why this way

The refactorings are needed to avoid code duplication.

An alternative solution would be to allow querying the validation endpoint with no version (/compatibility/subjects/<subject-key>), and apply the fix only in that case.

website/source/quickstart.rst

github-actions · 2024-09-19T13:48:27Z

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
src/karapace
schema_registry.py
schema_registry_apis.py
src/karapace/compatibility
__init__.py
Project Total

_{This report was generated by python-coverage-comment-action}

eliax1996

I think the code its nice, the feature its well tested.
I'm still not fully convinced by the use of mocks, I think they can couple the way of a certain result its computed.
I would much more like a test where you give the input and the real implementation does the logic and you check the outcome, its a type of testing that makes the refactor much less painful since you don't need to change the assertions not being coupled by the names of the functions and the order in which they are called

karapace/schema_registry.py

karapace/schema_registry_apis.py

tests/unit/test_schema_registry.py

karapace/schema_registry.py

karapace/schema_registry_apis.py

tests/unit/test_schema_registry.py

davide-armand · 2024-10-10T07:38:17Z

Rebased and fixed conflicts

src/karapace/compatibility/schema_compatibility.py

src/karapace/schema_registry.py

src/karapace/schema_registry_apis.py

tests/integration/test_dependencies_compatibility_protobuf.py

tests/integration/test_schema.py

tests/integration/test_schema_compatibility.py

eliax1996

Almost there, except for a couple of changes great great job!
I like much more those tests, I can read them and understand them really well, thanks for the hard work 🚀

This in preparation for the fix for [EC-289]. - Move schema compatibility related code from __init__.py to its own module (schema_compatibility.py) - Refactor logic in: - Schema registering endpoint (`/subjects/<subject-key>/versions`) - Schema compatibility endpoint (`/compatibility/subjects/<subject-key>/versions/latest`)

Make so that the transitive compatibility checks that are done in the schema creation endpoint are also done in the schema validation endpoint. In the creation endpoint (`/subjects/<subject-key>/versions`), if the compatibility mode is transient then the new schema is checked against all schemas. In the validation endpoint (`/compatibility/subjects/<subject-key>/versions/<schema-version>`): - Before this fix, only the latest schema is checked against (even in case of transitive mode) - After this fix, in case of transitive mode then all schema are checked against. Note that in this case the version provided in the POST request (`<schema-version>`) is ignored.

davide-armand · 2024-10-10T14:03:12Z

@eliax1996 I should I have address all comments, thanks for your detailed review!

davide-armand force-pushed the davide-armand/EC-289-fix-transitive-validation branch 5 times, most recently from c32cfaf to 9f1c54b Compare September 18, 2024 13:13

davide-armand commented Sep 18, 2024

View reviewed changes

website/source/quickstart.rst Show resolved Hide resolved

davide-armand marked this pull request as ready for review September 18, 2024 13:15

davide-armand requested review from a team as code owners September 18, 2024 13:15

davide-armand force-pushed the davide-armand/EC-289-fix-transitive-validation branch from 9f1c54b to 3cce148 Compare September 19, 2024 13:05

eliax1996 reviewed Sep 23, 2024

View reviewed changes

davide-armand force-pushed the davide-armand/EC-289-fix-transitive-validation branch from 3cce148 to d4b7cc0 Compare September 23, 2024 13:52

jjaakola-aiven reviewed Sep 24, 2024

View reviewed changes

karapace/schema_registry.py Outdated Show resolved Hide resolved

karapace/schema_registry_apis.py Outdated Show resolved Hide resolved

tests/unit/test_schema_registry.py Outdated Show resolved Hide resolved

davide-armand force-pushed the davide-armand/EC-289-fix-transitive-validation branch 6 times, most recently from bec59e5 to d98176e Compare October 3, 2024 10:23

davide-armand force-pushed the davide-armand/EC-289-fix-transitive-validation branch 2 times, most recently from bd499aa to 2a7a250 Compare October 10, 2024 07:36