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

Prefix/suffix/lowercase crashes on oneof #75

Closed
tsloughter opened this issue Aug 13, 2016 · 6 comments
Closed

Prefix/suffix/lowercase crashes on oneof #75

tsloughter opened this issue Aug 13, 2016 · 6 comments

Comments

@tsloughter
Copy link
Contributor

If the options include a msg prefix, suffix or msg_name_to_lower the parser crashes in prefix_suffix_fields because it expects every list element to be a gpb_field but instead finds a gpb_oneof:

https://github.com/tomas-abrahamsson/gpb/blob/master/src/gpb_parse.yrl#L1093-L1101

@tsloughter
Copy link
Contributor Author

Hm, the issue is a little more complicated than I thought.

If I have it simply skip the oneof in this function I still have an issue with both oneof and map if they use the types of other messages and a formatter like lowercase:

message A { ...}

message B { 
  oneof x {
    A f1 = 1;
  }

  map<string, A> y = 2;
}

Both of these will fail because it can't find A in a dict it looks it up in is_msgsize_known_at_generationtime.

Not sure yet where in the parser to properly format those types.

@tomas-abrahamsson
Copy link
Owner

For oneof, I think the following change would do it:

--- a/src/gpb_parse.yrl
+++ b/src/gpb_parse.yrl
@@ -1095,6 +1095,9 @@ prefix_suffix_fields(Prefix, Suffix, ToLower, Fields) ->
       fun(#?gpb_field{type={msg,MsgName}}=F) ->
               NewMsgName = prefix_suffix_name(Prefix, Suffix, ToLower, MsgName),
               F#?gpb_field{type={msg,NewMsgName}};
+         (#gpb_oneof{fields=Fs}=F) ->
+              Fs2 = prefix_suffix_fields(Prefix, Suffix, ToLower, Fs),
+              F#gpb_oneof{fields=Fs2};
          (#?gpb_field{}=F) ->
               F
       end,

@tomas-abrahamsson
Copy link
Owner

Will take a look at map<_,_>

@tomas-abrahamsson
Copy link
Owner

Good thing you thought about maps too. Perhaps something along these lines:

--- a/src/gpb_parse.yrl
+++ b/src/gpb_parse.yrl
@@ -1095,6 +1095,9 @@ prefix_suffix_fields(Prefix, Suffix, ToLower, Fields) ->
       fun(#?gpb_field{type={msg,MsgName}}=F) ->
               NewMsgName = prefix_suffix_name(Prefix, Suffix, ToLower, MsgName),
               F#?gpb_field{type={msg,NewMsgName}};
+         (#?gpb_field{type={map,KeyType,{msg,MsgName}}}=F) ->
+              NewMsgName = prefix_suffix_name(Prefix, Suffix, ToLower, MsgName),
+              F#?gpb_field{type={map,KeyType,{msg,NewMsgName}}};
          (#gpb_oneof{fields=Fs}=F) ->
               Fs2 = prefix_suffix_fields(Prefix, Suffix, ToLower, Fs),
               F#gpb_oneof{fields=Fs2};

@tsloughter
Copy link
Contributor Author

Cool, yup, those did it, thanks!

@tomas-abrahamsson
Copy link
Owner

I merged your fix in #76 and just pushed it as 3.25.0. (Closing)

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

No branches or pull requests

2 participants