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

normalization: add normalization of the options #848

Merged
merged 4 commits into from
Apr 12, 2024

Conversation

eliax1996
Copy link
Contributor

@eliax1996 eliax1996 commented Apr 8, 2024

The options could be of different types:

  1. string
  2. boolean
  3. number
  4. enum
  5. map
  6. list
  7. option

Here I've added an example for each one of them:

  1. STRING:
option example_option = "hello world";
  1. BOOLEAN:
option example_option = true;
  1. NUMBER:
option example_option = 42;
  1. ENUM:
enum ExampleEnum {
  OPTION_ONE = 1;
  OPTION_TWO = 2;
}

option example_option = OPTION_ONE;
  1. MAP:
message ExampleMessage {
  map<string, int32> example_map = 1;
}

option (map_option) = {
  key: "key1"
  value: 42
};
  1. LIST:
message ExampleMessage {
  repeated string example_list = 1;
}

option (list_option) = [
  "item1",
  "item2",
  "item3"
];
  1. OPTION:
option (custom_option) = {
  string_option: "hello world",
  enum_option: OPTION_ONE,
  boolean_option: true
};

The goal of this pr its to start the process of normalization of the schemas initially just for the options in the protobuf types.

About this change - What it does

It changes how the protobuf schema gets normalized before being saved

Why this way

Seems that sorting alphabetically the options its a good way to provide an invariant over the order of the options.

For the reviewers: I'm not completely familiar with protobuf nor the normalizations, please read each line of documentation (of the code and of the pr) and if anything doesn't sound right or you found something that seems wrong please discuss about it!
I've added an example for each type of option just to make sure we are not loosing any important corner case. To me sounds reasonable to order those in any case. Additional normalizations could be added in another pr

@eliax1996 eliax1996 requested review from a team as code owners April 8, 2024 15:52
1. string
2. boolean
3. number
4. enum
5. map
6. list
7. option

Here I've added an example for each one of them:

1. STRING:

```
option example_option = "hello world";
```

2. BOOLEAN:

```
option example_option = true;
```

3. NUMBER:

```
option example_option = 42;
```

4. ENUM:

```
enum ExampleEnum {
  OPTION_ONE = 1;
  OPTION_TWO = 2;
}

option example_option = OPTION_ONE;
```

5. MAP:

```
message ExampleMessage {
  map<string, int32> example_map = 1;
}

option (map_option) = {
  key: "key1"
  value: 42
};
```

6. LIST:

```
message ExampleMessage {
  repeated string example_list = 1;
}

option (list_option) = [
  "item1",
  "item2",
  "item3"
];
```

7. OPTION:

```
option (custom_option) = {
  string_option: "hello world",
  enum_option: OPTION_ONE,
  boolean_option: true
};
```

The goal of this pr its to start the process of normalization of the schemas initially just for the options in the protobuf types.

# About this change - What it does

It changes how the protobuf schema gets normalized before being saved

# Why this way

Seems that sorting alphabetically the options its a good way to provide an invariant over the order of the options.

For the reviewers: I'm not completely familiar with protobuf nor the normalizations, please read each line of documentation (of the code and of the pr) and if anything doesn't sound right or you found something that seems wrong please discuss about it!
I've added an example for each type of option just to make sure we are not loosing any important corner case. To me sounds reasonable to order those in any case. Additional normalizations could be added in another pr
@eliax1996 eliax1996 force-pushed the eliax1996/create-protobuf-schema-normalisation branch from eda6cae to 478feb5 Compare April 8, 2024 16:01
@tvainika
Copy link
Contributor

tvainika commented Apr 9, 2024

Support for normalize flag is also needed in POST /subjects/<subject> which returns ID of schema if already registered.

Copy link
Contributor

@jjaakola-aiven jjaakola-aiven left a comment

Choose a reason for hiding this comment

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

Options are allowed on message and field level, I'd add the support for these also.

@eliax1996 eliax1996 force-pushed the eliax1996/create-protobuf-schema-normalisation branch from 7557a8e to 57372e7 Compare April 9, 2024 16:10
Copy link
Contributor Author

@eliax1996 eliax1996 left a comment

Choose a reason for hiding this comment

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

ready to be reviewed again :)

karapace/protobuf/proto_normalizations.py Show resolved Hide resolved
karapace/protobuf/proto_normalizations.py Show resolved Hide resolved
Comment on lines +15 to +20
# this would be a good case for using a property based test with a well-formed message generator
# (hypothesis could work if the objects representing the language
# do always correspond to a valid proto schema, that for a correct parser should always be true)
# we should create a set of random valid DSLs and check that the normalization is giving back a sorted options list
#
# nb: we could use the protoc compiler as an oracle to check that the files are always valid protobuf schemas
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moment:
immagine

karapace/protobuf/proto_normalizations.py Outdated Show resolved Hide resolved
karapace/protobuf/proto_normalizations.py Outdated Show resolved Hide resolved
karapace/protobuf/proto_normalizations.py Outdated Show resolved Hide resolved
karapace/protobuf/proto_normalizations.py Outdated Show resolved Hide resolved
@eliax1996 eliax1996 force-pushed the eliax1996/create-protobuf-schema-normalisation branch 6 times, most recently from 335fc9b to c56edd1 Compare April 10, 2024 14:39
@eliax1996 eliax1996 force-pushed the eliax1996/create-protobuf-schema-normalisation branch from c56edd1 to 0b2d929 Compare April 10, 2024 14:43
karapace/protobuf/proto_file_element.py Outdated Show resolved Hide resolved
karapace/protobuf/proto_normalizations.py Outdated Show resolved Hide resolved
karapace/protobuf/proto_normalizations.py Outdated Show resolved Hide resolved
@eliax1996 eliax1996 force-pushed the eliax1996/create-protobuf-schema-normalisation branch 3 times, most recently from 313fd6d to aa907ce Compare April 11, 2024 10:42
@eliax1996 eliax1996 requested a review from aiven-anton April 11, 2024 10:49
@eliax1996 eliax1996 force-pushed the eliax1996/create-protobuf-schema-normalisation branch from aa907ce to dde1f84 Compare April 11, 2024 11:26
@eliax1996 eliax1996 requested a review from aiven-anton April 11, 2024 11:27
aiven-anton
aiven-anton previously approved these changes Apr 12, 2024
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
jjaakola-aiven
jjaakola-aiven previously approved these changes Apr 12, 2024
@eliax1996 eliax1996 dismissed stale reviews from jjaakola-aiven and aiven-anton via ea6e82f April 12, 2024 10:58
@eliax1996 eliax1996 force-pushed the eliax1996/create-protobuf-schema-normalisation branch from da6bc5e to 7293fc7 Compare April 12, 2024 11:01
@aiven-anton aiven-anton enabled auto-merge April 12, 2024 11:11
@aiven-anton aiven-anton merged commit aafc087 into main Apr 12, 2024
8 checks passed
@aiven-anton aiven-anton deleted the eliax1996/create-protobuf-schema-normalisation branch April 12, 2024 11:19
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.

4 participants