schema_registry: Improve sanitization of Avro namespaces #12334
Conversation
Pass a context instead of just a memoryt allocator. Signed-off-by: Ben Pope <[email protected]>
| @@ -296,7 +302,21 @@ result<void> sanitize(json::Value::Object& o, sanitize_context& ctx) { | |||
| new_namespace = std::move(fullname); | |||
| } | |||
|
|
|||
| if (!new_namespace.empty()) { | |||
| if (!new_namespace.has_value()) { | |||
There was a problem hiding this comment.
nit: From what I can tell, the variable new_namespace represents the namespace taken from the name of a field. For example: name: existing.Simple implies new_namespace="existing"
It may be easier to read this code if we change the variable name for new_namespace to better reflect the above. For example: new_namespace -> namespace_from_field_name
This is a nit though and it could be apart of one of our tickets to refactor the Schema Registry.
There was a problem hiding this comment.
new_namespace is the value of the namespace attribute we expect to put onto the object, but that's not true until the fullname has been split, and if it's not a fullname, the existing namespace, if any, has been set. Maybe I could make it more functional.
There was a problem hiding this comment.
I'm adding a link to this comment in one of our epics for SR startup for tracking.
Regardless, this comment does not block the PR
If the name field is a fullname, then an existing namespace is ignored. Sanitize splits the fullname into name and namespace, so rewrite the namespace rather than returning an error. Reference: https://avro.apache.org/docs/1.11.1/specification/#names Signed-off-by: Ben Pope <[email protected]>
Check that simple namespace cases are left alone. Signed-off-by: Ben Pope <[email protected]>
This PR keeps track of the current namespace in a stack, starting with the implicitly null "empty" namespace. If the namespace changes, push it to the stack. If a namespace is redundant (the same as outer scope), remove it. Fixes redpanda-data#11912 Signed-off-by: Ben Pope <[email protected]>
Signed-off-by: Ben Pope <[email protected]>
BenPope
force-pushed
the
sr_avro_sanitize_ns
branch
from
40458dc
to
d73b2c8
Compare
BenPope
changed the title
|
/backport v23.2.x |
|
/backport v23.1.x |
|
/backport v22.3.x |
|
/backport v22.2.x |
|
Failed to run cherry-pick command. I executed the commands below: |
Improve sanitization of namespaces
Fixes #11912
Backports Required
Release Notes
Bug Fixes