criccomini / proto-schema-parser

A Pure Python Protobuf Parser
MIT License
38 stars 20 forks source link

Generator turns integer and enum option values into strings #33

Closed johan-groenenboom closed 1 week ago

johan-groenenboom commented 3 weeks ago

When parsing:

syntax = "proto3";
package test.v1.proto_1;
import "test/options/v1/option_extensions.proto";
service TestService {
  option (test.options.v1.service_lifecycle) = SERVICE_LIFECYCLE_UNPUBLISHED;
  option (test.options.v1.major_version) = 1;
  option (test.options.v1.minor_version) = 2;
  option (test.options.v1.patch_version) = 3;
  rpc TestMethod (TestRequest) returns (TestResponse);
}
message TestRequest {}   
message TestResponse {}

And putting the output of Parser().parse back into Generator().generate(), the result is as shown below, with all the option values turned into strings:

syntax = "proto3";
package test.v1.proto_1;
import "test/options/v1/option_extensions.proto";
service TestService {
  option (test.options.v1.service_lifecycle) = "SERVICE_LIFECYCLE_UNPUBLISHED";
  option (test.options.v1.major_version) = "1";
  option (test.options.v1.minor_version) = "2";
  option (test.options.v1.patch_version) = "3";
  rpc TestMethod (TestRequest) returns (TestResponse);
}
message TestRequest {
}
message TestResponse {
}
criccomini commented 2 weeks ago

Yes, the parser doesn't know what the data types are for options, so it treats everything as a string:

    def visitOptionDecl(self, ctx: ProtobufParser.OptionDeclContext):
        name = self._getText(ctx.optionName())
        value = self._getText(ctx.optionValue())
        return ast.Option(name=name, value=value)
criccomini commented 2 weeks ago

Let me see if I can add bool, int, and float support.

criccomini commented 2 weeks ago

Fixed and released in 1.3.8:

https://pypi.org/project/proto-schema-parser/1.3.8/

johan-groenenboom commented 2 weeks ago

Hi Chris, thanks for the fast response. Awesome! - Johan

johan-groenenboom commented 2 weeks ago

I did some testing on the new version, I still got the same result:

PROTO_TEXT = '''
syntax = "proto3";
package test.v1.proto_1;
service TestService {
    option (test.options.v1.lifecycle) = DEPRECATED;
    option (test.options.v1.major_version) = 1;
}
'''
ast = Parser().parse(PROTO_TEXT)
gen = Generator().generate(ast)
print(gen)

Gives this output:

syntax = "proto3";
package test.v1.proto_1;
service TestService {
  option (test.options.v1.lifecycle) = "DEPRECATED";
  option (test.options.v1.major_version) = "1";
}

So the option values are still output as strings. I guess the criteria (for the parser) for a value being a string is simply that it has quotes, otherwise it's something else.

criccomini commented 2 weeks ago

Ah! I think I might be defaulting all options to string on output. Lemme check

criccomini commented 2 weeks ago

Fixing here: https://github.com/criccomini/proto-schema-parser/pull/36

johan-groenenboom commented 2 weeks ago

I left some comments

criccomini commented 2 weeks ago

K, I think it should work now. Lemme publish a release.

johan-groenenboom commented 2 weeks ago

The new 1.3.9 release doesn't load in my Python 3.8.1 environment - I get a TypeError: File "C:\Python38\lib\site-packages\proto_schema_parser\parser.py", line 41, in ASTConstructor def visitOptionValue(self, ctx: ProtobufParser.OptionValueContext | Any): TypeError: unsupported operand type(s) for |: 'type' and '_SpecialForm'

If I remove the Any, similar errors pop up in several other places, such as generator.py line 206 and parser.py line 267

(this broke our build so I had to revert to proto-schema-parser ==1.3.8)

I'm not familiar with using pdm test - so not sure if I missed something - but running pdm test on master gave me this: λ pdm test ================================================================================================================================== test session starts ================================================================================================================================== platform win32 -- Python 3.8.1, pytest-8.2.2, pluggy-1.5.0 rootdir: D:\SB\WorkBench\proto-schema-parser configfile: pyproject.toml plugins: anyio-4.5.0 collected 43 items

tests\test_generator.py ..................F [ 44%] tests\test_parser.py ...........F....F..F...F

criccomini commented 1 week ago

Gah, my bad. I did Python 3.10 syntax. Fixing.

criccomini commented 1 week ago

Just published 3.10 @johan-groenenboom.

https://pypi.org/project/proto-schema-parser/1.3.10/

LMK how it goes!

johan-groenenboom commented 1 week ago

Hi Chris, thanks! The use cases I submitted for this issue are now handled transparently by a parser->generator cycle. All good there. I do have another use case that still fails, but I will open a separate issue for that.