Skip to content

When caching schemas, ensure the cache key matches the input #1922

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

Merged
merged 1 commit into from
Feb 24, 2025

Conversation

rayokota
Copy link
Member

@rayokota rayokota commented Feb 24, 2025

What

When caching schemas, ensure the cache key matches the input

Checklist

  • Contains customer facing changes? Including API/behavior changes
  • Did you add sufficient unit test and/or integration test coverage for this PR?
    • Tested with a local SR

References

JIRA:

Test & Review

Open questions / Follow-ups

@Copilot Copilot AI review requested due to automatic review settings February 24, 2025 19:26
@rayokota rayokota requested review from a team as code owners February 24, 2025 19:26
@confluent-cla-assistant
Copy link

🎉 All Contributor License Agreements have been signed. Ready to merge.
Please push an empty commit if you would like to re-run the checks to verify CLA status for all contributors.

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (4)

src/confluent_kafka/schema_registry/schema_registry_client.py:476

  • The new signature introduces an explicit 'schema' parameter which is used as a cache key, but there is no check to ensure it matches registered_schema.schema. Consider validating that both schemas are identical to prevent caching inconsistencies.
def set_registered_schema(self, schema: 'Schema', registered_schema: 'RegisteredSchema'):

src/confluent_kafka/schema_registry/schema_registry_client.py:788

  • Ensure that the 'schema' passed into set_schema here is intentionally different from registered_schema.schema, or else validate that they are equivalent, so that the cache key accurately reflects the intended input.
self._cache.set_schema(subject_name, registered_schema.schema_id, schema)

src/confluent_kafka/schema_registry/schema_registry_client.py:868

  • Verify that the explicit 'schema' argument passed here correctly represents the schema intended for caching, ensuring consistency with registered_schema.schema.
self._cache.set_registered_schema(schema, registered_schema)

src/confluent_kafka/schema_registry/schema_registry_client.py:1027

  • Confirm that using registered_schema.schema directly as the cache key aligns with the new caching logic and that no unintended discrepancies arise compared to other cache update calls.
self._cache.set_registered_schema(registered_schema.schema, registered_schema)

@sonarqube-confluent
Copy link

Passed

Analysis Details

0 Issues

  • Bug 0 Bugs
  • Vulnerability 0 Vulnerabilities
  • Code Smell 0 Code Smells

Coverage and Duplications

  • Coverage 100.00% Coverage (61.70% Estimated after merge)
  • Duplications No duplication information (0.90% Estimated after merge)

Project ID: confluent-kafka-python

View in SonarQube

@rayokota rayokota merged commit 57608ce into master Feb 24, 2025
1 of 2 checks passed
@rayokota rayokota deleted the fix-cache branch February 24, 2025 19:40
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