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

feature: replace the old parsing strategy and move towards a tree based one #733

Conversation

eliax1996
Copy link
Contributor

@eliax1996 eliax1996 commented Oct 6, 2023

The scope of this pr is to replace the old parsing strategy and move towards a tree based type verification strategy.

From this commit it's possible to refer to a type using a partial path and not only a fully specified path. If we declare for e.g. the following schema:

syntax = "proto3";

package my.awesome.customer.plan.entity.v1beta1;

message CustomerPlan {
  string plan_name = 1;
}

When importing it it's sufficient to use a part of the full specified path (that in this case would be my.awesome.customer.plan.entity.v1beta1.CustomerPlan). As e.g. it would be enough to use entity.v1beta1.CustomerPlan or v1beta1.CustomerPlan, so the following declaration would be accepted:

syntax = "proto3";

package my.awesome.customer.plan.entity.v1beta1;
import "my/awesome/customer/plan/entity/v1beta1/entity.proto";

message CustomerPlanEvent {
  message Created {
    entity.v1beta1.CustomerPlan plan = 1;
  }
}

@eliax1996 eliax1996 force-pushed the eliax1996/switch-protobuf-type-validation-from-generating-all-valid-types-to-generate-tree-of-dependencies branch from a97af81 to bbfa298 Compare October 6, 2023 14:23
@eliax1996
Copy link
Contributor Author

This pr it's experimental, even if it's passing all the tests stuff can be still moved around and a final judgement should be given from @jjaakola-aiven before going forward. Please try as much valid (and invalid) schemas as possible and provide feedbacks if something it's fishy.
A sample test it's the test_partial_path_in_protobuf, feel free to start from here and craft new ones.

@eliax1996 eliax1996 force-pushed the eliax1996/switch-protobuf-type-validation-from-generating-all-valid-types-to-generate-tree-of-dependencies branch from bbfa298 to be81a6a Compare October 6, 2023 14:34
@eliax1996 eliax1996 marked this pull request as ready for review October 13, 2023 08:30
@eliax1996 eliax1996 requested review from a team as code owners October 13, 2023 08:30
Copy link
Contributor

@aiven-anton aiven-anton left a comment

Choose a reason for hiding this comment

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

I left some minor nits.

karapace/protobuf/proto_file_element.py Outdated Show resolved Hide resolved
karapace/protobuf/schema.py Outdated Show resolved Hide resolved
karapace/protobuf/schema.py Outdated Show resolved Hide resolved
karapace/protobuf/schema.py Show resolved Hide resolved
karapace/protobuf/schema.py Show resolved Hide resolved
@eliax1996 eliax1996 force-pushed the eliax1996/switch-protobuf-type-validation-from-generating-all-valid-types-to-generate-tree-of-dependencies branch 4 times, most recently from 3abacdb to fff366c Compare October 16, 2023 17:07
@eliax1996
Copy link
Contributor Author

Can I have another round of review @jjaakola-aiven and @aiven-anton?
I still have to get rid of the # pylint: disable=W0212 and split the huge test in 2/3 tests (because they are testing different aspects related but independent about the parser).

Except that wdyt? I've tried to transform the TypeTree into an immutable object but the descending visit would become much harder to read, I think that in that case the penalty of having a mutable object is ok if the parsing algorithm it's easier. We can also create an internal object called _TypeTree with a List in it and a type visible externally where we replace the List with a Sequence, would it be a good enough tradeoff to keep mutability inside and immutability outside @aiven-anton?

@eliax1996 eliax1996 force-pushed the eliax1996/switch-protobuf-type-validation-from-generating-all-valid-types-to-generate-tree-of-dependencies branch 2 times, most recently from a4c8b11 to f8563fa Compare October 16, 2023 17:30
…towards a tree based type verification strategy.

From this commit it's possible to refer to a type using a partial path and not only a fully specified path. If we declare for e.g. the following schema:
```protobuf
syntax = "proto3";

package my.awesome.customer.plan.entity.v1beta1;

message CustomerPlan {
  string plan_name = 1;
}
```

When importing it it's sufficient to use a part of the full specified path (that in this case would be `my.awesome.customer.plan.entity.v1beta1.CustomerPlan`). As e.g. it would be enough to use `entity.v1beta1.CustomerPlan` or `v1beta1.CustomerPlan`, so the following declaration would be accepted:
```protobuf
syntax = "proto3";

package my.awesome.customer.plan.entity.v1beta1;
import "my/awesome/customer/plan/entity/v1beta1/entity.proto";

message CustomerPlanEvent {
  message Created {
    entity.v1beta1.CustomerPlan plan = 1;
  }
}
```
@eliax1996 eliax1996 force-pushed the eliax1996/switch-protobuf-type-validation-from-generating-all-valid-types-to-generate-tree-of-dependencies branch from f8563fa to 04bff10 Compare October 17, 2023 08:30
@jjaakola-aiven jjaakola-aiven merged commit 118ba6b into main Oct 17, 2023
8 checks passed
@jjaakola-aiven jjaakola-aiven deleted the eliax1996/switch-protobuf-type-validation-from-generating-all-valid-types-to-generate-tree-of-dependencies branch October 17, 2023 11:46
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.

3 participants