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

fix: enable self-referencing schemas #716

Merged

Conversation

eliax1996
Copy link
Contributor

@eliax1996 eliax1996 commented Sep 15, 2023

Before this PR the protobuf schema was able to refer only to fully qualified names or names defined in an higher leaf of the parsing tree.
This commit enable a schema to refer the same scope level if at the same leaf level there is defined exactly the field required after.

In an example:

enum FancyEnum {
  ...
}
message FancyMessage1 {
   FancyEnum field ...
}

FancyMessage1 is allowed to call FancyEnum because his scope is /FancyMessage1 while the scope of FancyEnum it's / (the name itself doesn't count in the declarations, but take part only in field assignment inside the declarations, we could say that field has the scope set to /FancyMessage1 but the type FancyEnum has also the scope /, only when referring to a type you enter in it's scope during the parsing procedure).

This was a valid declaration in the previous status.

Instead, in this example:

message FancyMessage1 {
   enum FancyEnum {
     ...
   }
   FancyEnum field ...
}

In the previous state was an invalid schema.

With this pr, since FancyEnum field ... it's declared inside the scope /FancyMessage1 but also the requested type FancyEnum it's declared in the same scope, /FancyMessage1 it's considered valid

Fixes #713

@eliax1996 eliax1996 requested review from a team as code owners September 15, 2023 14:58
@eliax1996 eliax1996 force-pushed the eliax1996/karapace-support-embedded-message-parsing branch from 9740f53 to 81bdb71 Compare September 15, 2023 15:03
@tvainika
Copy link
Contributor

This code still fails with slightly modified schema like

syntax = "proto3";

package fancy.company.in.party.v1;
message AnotherMessage {
  enum BamFancyEnum {
    // Hei! This is a comment!
    MY_AWESOME_FIELD = 0;
  }
  message WowANestedMessage {
    message DeeplyNestedMsg {
      BamFancyEnum im_tricky_im_referring_to_the_previous_enum = 1;
    }
  }
}

which I believe is as valid as your test case.

@eliax1996 eliax1996 force-pushed the eliax1996/karapace-support-embedded-message-parsing branch 2 times, most recently from 462a93e to be5c0db Compare September 18, 2023 09:51
@juha-aiven
Copy link
Contributor

In description

...the avro schema was able to refer ...

You mean of course protobuf schema.

@eliax1996 eliax1996 force-pushed the eliax1996/karapace-support-embedded-message-parsing branch 5 times, most recently from 475deef to 2cea6ad Compare September 18, 2023 11:43
@eliax1996 eliax1996 force-pushed the eliax1996/karapace-support-embedded-message-parsing branch 2 times, most recently from c1f36ab to 826f16d Compare September 18, 2023 13:55
@eliax1996 eliax1996 force-pushed the eliax1996/karapace-support-embedded-message-parsing branch from 826f16d to d8212b8 Compare September 19, 2023 07:11
@jjaakola-aiven jjaakola-aiven merged commit 60fb7c3 into main Sep 19, 2023
8 checks passed
@jjaakola-aiven jjaakola-aiven deleted the eliax1996/karapace-support-embedded-message-parsing branch September 19, 2023 08:50
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.

Nested enum support in Protobuf message
4 participants