proto3 standard compatibility improvements #143

tim2CF · 2021-03-22T15:43:41Z

Added DotProtoMessageOption constructor to support options inside messages, provided example, included to tests, example
Added globalIdentifier parser to support leading dots on field types, provided example, included to tests, example
Fixed SwaggerObject conditional compilation bug here
Fixed shell.nix (cabal might not like wget installed on host machine) here
Added CircleCI support
Added proposal related to protoc plugins here
Need help with this

Gabriella439

Looks great! Just one suggestion

Gabriella439 · 2021-03-22T17:34:30Z

tools/canonicalize-proto-file/Main.hs

@@ -126,6 +126,8 @@ instance CanonicalRank DotProtoMessagePart
    DotProtoMessageReserved _fs -> Left (Right ())
      -- We use '()' here because 'Canonicalize [DotProtoMessagePart]'
      -- collapses all of the 'DotProtoMessageReserved's into just one.
+    DotProtoMessageOption _ -> Left (Right ())


So I'm new to this section of the code, too, but I think I know what we need to do here. Basically, this module is formatting a .proto file with everything in a specified order. The way that it works is that it maps each node in the syntax tree to some type that has an Ord instance and then uses that Ord instance to sort them.

This part of the code would probably be clearer and easier to fix if you first make a small refactor to replace the weird nested Either type with a new separate datatype, like this:

data PartRank = PartRankDefinition (Int, DotProtoIdentifier) | PartRankReserved | PartRankField (Maybe FieldNumber) instance CanonicalRank DotProtoMessagePart where canonicalRank = \case DotProtoMessageField f -> PartRankField (canonicalRank f) DotProtoMessageOneOf _ fs -> PartRankField (canonicalRank f) DotProtoMessageDefinition d -> PartRankDefinition (canonicalRank d) DotProtoMessageReserved _ -> PartRankReserved

… and then once you refactor it like that then it's easier to add in a new constructor for message options, like this:

data PartRank = PartRankDefinition (Int, DotProtoIdentifier) | PartRankReserved | PartRankField (Maybe FieldNumber) | PartRankOption DotProtoOption instance CanonicalRank DotProtoMessagePart where canonicalRank = \case DotProtoMessageField f -> PartRankField (canonicalRank f) DotProtoMessageOneOf _ fs -> PartRankField (canonicalRank f) DotProtoMessageDefinition d -> PartRankDefinition (canonicalRank d) DotProtoMessageReserved _ -> PartRankReserved DotProtoMessageOption o -> PartRankOption o

I haven't type-checked that suggestion, so there might be a few mistakes or other things that would need to be fixed, but I think that's the general idea of how to go about this.

@Gabriel439 thanks for advise, it worked perfectly! Fixed in 699277b

I also changed order of PartRank constructors to provide more logical ordering of protobuf AST.

evanrelf

I don't understand the "Added CircleCI support" part of this pull request, since we don't use CircleCI. What's your intention adding .circleci/config.yml?

21it · 2021-03-22T17:56:58Z

I don't understand the "Added CircleCI support" part of this pull request, since we don't use CircleCI. What's your intention adding .circleci/config.yml?

We are using CircleCI, in our fork, you can see it there https://github.com/coingaming/proto3-suite/commits/master
In general, CirecleCI is quite popular and working CircleCI config might be useful for people who fork this library, and want CI to be working from the box.

But if you don't want it - I can remove it for sure.

evanrelf · 2021-03-22T18:06:19Z

@tkachuk-labs I think it would be best if you removed it from the pull request to avoid confusion. I don't think it's common for open source projects to have multiple CI configs unless they're actually using those CI services.

21it · 2021-03-22T18:09:40Z

@evanrelf sure, no problem!

…ation of DotProtoMessagePart, removed CircleCI support

tim2CF · 2021-03-23T13:11:10Z

@evanrelf done in 699277b

Gabriella439

Looks great! Just one last thing

Gabriella439 · 2021-03-23T15:26:42Z

tools/canonicalize-proto-file/Main.hs

+    DotProtoMessageOption x -> PartRankOption x
+    DotProtoMessageDefinition d -> PartRankDefinition (canonicalRank d)
+    DotProtoMessageReserved _fs -> PartRankReserved
+      -- We don't use '_fs' here because 'Canonicalize [DotProtoMessagePart]'


This comment needs to be relocated above the DotProtoMessageOneOf case after your change

But it was like this before. Comment is related to DotProtoMessageReserved _fs AST node. So it depends on style of codebase - should I put comment after related to comment line, or before.

proto3-suite/tools/canonicalize-proto-file/Main.hs

Lines 126 to 128 in c40e73e

DotProtoMessageReserved _fs -> Left (Right ())

-- We use '()' here because 'Canonicalize [DotProtoMessagePart]'

-- collapses all of the 'DotProtoMessageReserved's into just one.

…linux generator binary matching local swagger2 version

tim2CF · 2021-03-24T15:27:18Z

I did added swagger2isAbove_2_4_0 optional flag here. It's needed for being able to get suitable version of proto3-suite.proto3-suite-linux executable, which can properly generate hs files from proto files according version of swagger2 used in application project. By default, before it always was generating hs files for swagger2 version > 2.4.0, and this caused compilation errors in generated code. Another possible solution is to move CPP conditional compilation to generated code itself, but it seems harder to do.

     * Couldn't match expected type `HsJSONPB.SwaggerType                                 
                                       'swagger2-2.3.1.1:Data.Swagger.Internal.           
 SwaggerKindSchema                                                                        
 '                                                                                        
                   with actual type `Hs.Maybe                                             
                                       (HsJSONPB.SwaggerType                              
                                          'swagger2-2.3.1.1:Data.Swagger.Internal.        
 SwaggerKindSch                                                                           
 ema)'                                                                                    
     * In the `_paramSchemaType' field of a record                                        
       In the `_schemaParamSchema' field of a record                                      
       In the `_namedSchemaSchema' field of a record                                      
     |                                                                                    
 134 |                                                                  Hs.Just HsJSONPB. 
 Swagger                                                                                  
 Object},                                                                                 
     |                                                                                    
 ^^^^^^^^^^^^^^^^^^^^^^^^                                                                 
 ^^^^^^

…e codegen bug

Gabriella439 · 2021-03-31T19:08:57Z

@tim2CF: I added you as a collaborator so that you can merge this whenever you think this is ready

tim2CF · 2021-04-01T07:02:25Z

Thanks, Gabriel!

Gabriella439 · 2021-04-01T17:06:40Z

You're welcome! 🙂

evanrelf · 2021-06-22T19:36:45Z

@tim2CF I made some non-trivial changes to the Nix code in another PR (#159) which conflict with the changes you made here. Would you mind if I pushed to your branch to fix up the Nix stuff?

After that, unless anything else is left to be done, I think this'll be ready to merge!

tim2CF · 2021-06-22T20:21:52Z

@tim2CF I made some non-trivial changes to the Nix code in another PR (#159) which conflict with the changes you made here. Would you mind if I pushed to your branch to fix up the Nix stuff?

After that, unless anything else is left to be done, I think this'll be ready to merge!

Sure @evanrelf ! I've got busy with the other stuff, so if you will help me to fix conflicts and finally merge, this will be super cool!

evanrelf · 2021-06-22T21:22:53Z

Hmm... @tim2CF, can you see if this option is enabled in the pull request sidebar?

I can't seem to push to coingaming/proto3-suite's master branch:

ERROR: Permission to coingaming/proto3-suite.git denied to evanrelf.
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

Or maybe I've misunderstood that GitHub option?

tim2CF · 2021-06-25T07:59:22Z

@evanrelf seems like organization github accounts don't have this feature, but I did invited you as a contributor directly from our fork repo. Please try it

evanrelf · 2021-06-25T15:53:03Z

I invited you as a contributor directly from our fork repo

Thanks, that worked. This branch is now updated with the latest changes from master.

@tim2CF Since Gabriel gave you the commit bit, feel free to merge whenever you're ready!

EDIT: And feel free to remove me from coingaming/proto3-suite afterwards if you want.

tim2CF · 2021-06-28T08:18:02Z

Cool, thanks for the help @evanrelf !

21it added 6 commits September 10, 2020 18:38

expose Proto3.Suite.DotProto.Internal

27fe240

merge upstream

ff46c4b

cabal fix

b191343

conditional compilation according Swagger version

19d83b6

globalIdentifier support, circleci support

293d7b7

DotProtoMessageOption

8f3b0ca

Gabriella439 reviewed Mar 22, 2021

View reviewed changes

evanrelf reviewed Mar 22, 2021

View reviewed changes

21it added 2 commits March 23, 2021 15:02

data PartRank for easier ordering, refactored CanonicalRank implement…

699277b

…ation of DotProtoMessagePart, removed CircleCI support

Merge remote-tracking branch 'upstream/master'

89047af

Gabriella439 approved these changes Mar 23, 2021

View reviewed changes

21it added 2 commits March 24, 2021 15:51

optional swagger2isAbove_2_4_0 flag for getting correct proto3-suite-…

e8a49d4

…linux generator binary matching local swagger2 version

better swagger2 overrides

175cb46

21it added 2 commits March 26, 2021 11:53

WIP debug namespace

2efc0f4

qualifiedMessageTypeName function to fix deep nested message namespac…

efe1518

…e codegen bug

tim2CF mentioned this pull request Mar 26, 2021

Broken code generated for nested messages #112

Open

Evan Relf added 2 commits June 22, 2021 13:57

Merge remote-tracking branch 'upstream/master'

24c2880

Ignore result-* files

07be52a

tim2CF merged commit 9a89cc0 into awakesecurity:master Jun 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

proto3 standard compatibility improvements #143

proto3 standard compatibility improvements #143

tim2CF commented Mar 22, 2021 •

edited

Loading

Gabriella439 left a comment

Gabriella439 Mar 22, 2021

tim2CF Mar 23, 2021

evanrelf left a comment

21it commented Mar 22, 2021

evanrelf commented Mar 22, 2021

21it commented Mar 22, 2021

tim2CF commented Mar 23, 2021

Gabriella439 left a comment

Gabriella439 Mar 23, 2021

tim2CF Mar 24, 2021 •

edited

Loading

tim2CF commented Mar 24, 2021 •

edited

Loading

Gabriella439 commented Mar 31, 2021

tim2CF commented Apr 1, 2021

Gabriella439 commented Apr 1, 2021

evanrelf commented Jun 22, 2021

tim2CF commented Jun 22, 2021

evanrelf commented Jun 22, 2021

tim2CF commented Jun 25, 2021

evanrelf commented Jun 25, 2021 •

edited

Loading

tim2CF commented Jun 28, 2021

	DotProtoMessageReserved _fs -> Left (Right ())
	-- We use '()' here because 'Canonicalize [DotProtoMessagePart]'
	-- collapses all of the 'DotProtoMessageReserved's into just one.

proto3 standard compatibility improvements #143

proto3 standard compatibility improvements #143

Conversation

tim2CF commented Mar 22, 2021 • edited Loading

Gabriella439 left a comment

Choose a reason for hiding this comment

Gabriella439 Mar 22, 2021

Choose a reason for hiding this comment

tim2CF Mar 23, 2021

Choose a reason for hiding this comment

evanrelf left a comment

Choose a reason for hiding this comment

21it commented Mar 22, 2021

evanrelf commented Mar 22, 2021

21it commented Mar 22, 2021

tim2CF commented Mar 23, 2021

Gabriella439 left a comment

Choose a reason for hiding this comment

Gabriella439 Mar 23, 2021

Choose a reason for hiding this comment

tim2CF Mar 24, 2021 • edited Loading

Choose a reason for hiding this comment

tim2CF commented Mar 24, 2021 • edited Loading

Gabriella439 commented Mar 31, 2021

tim2CF commented Apr 1, 2021

Gabriella439 commented Apr 1, 2021

evanrelf commented Jun 22, 2021

tim2CF commented Jun 22, 2021

evanrelf commented Jun 22, 2021

tim2CF commented Jun 25, 2021

evanrelf commented Jun 25, 2021 • edited Loading

tim2CF commented Jun 28, 2021

tim2CF commented Mar 22, 2021 •

edited

Loading

tim2CF Mar 24, 2021 •

edited

Loading

tim2CF commented Mar 24, 2021 •

edited

Loading

evanrelf commented Jun 25, 2021 •

edited

Loading