Skip to content
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

Protobuf does not check imported types #626

Closed
tvainika opened this issue May 25, 2023 · 4 comments · Fixed by #630
Closed

Protobuf does not check imported types #626

tvainika opened this issue May 25, 2023 · 4 comments · Fixed by #630

Comments

@tvainika
Copy link
Contributor

What happened?

Protobuf does not check all types and imports. E.g. following invalid schema can be registered.

syntax = "proto3";
package a1;
import "foobar.proto";
message UsingImportedTypes {
        string name = 1;
        google.type.PostalAddress address = 2;
}

Another case is where import is valid, but is not checked. Then compatibility check fails with internal errors due not checking types when registering another schema, and compatibility checker tries to check compatibility between the schemas.

What did you expect to happen?

Schema registry produces error message about missing type.

What else do we need to know?

Tested 3.4.6 & 3.5.0

@libretto
Copy link
Contributor

@tvainika
Could You show me what is invalid in Your code example?

@libretto
Copy link
Contributor

libretto commented May 25, 2023

I see no missing type there.
string - is protobuf built-in type
google.type.PostalAddress - is a karapace built-in type that we built in for compatibility with competitor products which also have this type built-in

@tvainika
Copy link
Contributor Author

syntax = "proto3";
package a1;
import "foobar.proto";
message UsingImportedTypes {
        string name = 1;
        google.type.PostalAddress address = 2;
}

does not import google/type/PostalAddress. A competitor product would respond here with

{
    "error_code": 42201,
    "message": "Invalid schema {subject=google-types,version=0,id=-1,schemaType=PROTOBUF,references=[],schema=syntax = \"proto3\";\npackage a1;\nimport \"foobar.proto\";\nmessage UsingImportedTypes {\n        string name = 1;\n        google.type.PostalAddress address = 2;\n}\n}, details: com.google.protobuf.Descriptors$DescriptorValidationException: a1.UsingImportedTypes.address: \"google.type.PostalAddress\" is not defined."
}

With correct import statement it is valid usng built-in known types.

syntax = "proto3";
package a1;
import "google/type/postal_address.proto";
message UsingImportedTypes {
        string name = 1;
        google.type.PostalAddress address = 2;
}

as this is valid and has an import statement.

Also my comment about compatiblity check seems to be mostly triggered with comment changes, but not always. Need to investigate it more in detail.

@tvainika
Copy link
Contributor Author

And here is ready made test case that triggers 500 internal server error with only comment updated

async def test_protobuf_customer_update(registry_async_client: Client) -> None:
    subject = create_subject_name_factory("test_protobuf_customer_update")()

    customer_proto = """\
syntax = "proto3";
package a1;
import "google/type/postal_address.proto";
// @consumer: the first comment
message Customer {
        string name = 1;
        google.type.PostalAddress address = 2;
}
"""

    body = {"schemaType": "PROTOBUF", "schema": customer_proto}
    res = await registry_async_client.post(f"subjects/{subject}/versions", json=body)

    assert res.status_code == 200

    customer_proto = """\
syntax = "proto3";
package a1;
import "google/type/postal_address.proto";
// @consumer: the comment was incorrect, updating it now
message Customer {
        string name = 1;
        google.type.PostalAddress address = 2;
}
"""

    body = {"schemaType": "PROTOBUF", "schema": customer_proto}
    res = await registry_async_client.post(f"subjects/{subject}/versions", json=body)

    assert res.status_code == 200

tvainika added a commit that referenced this issue May 26, 2023
Hard-coded known types do not have message element linked similar to
user defined messages.  Handle comparing of those fields without
internal errors from exceptions.  Fixes #626
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 a pull request may close this issue.

2 participants