Fix/luaproto encode hooks

[git-torrent]

Summary

Transcoding between GRPC and JSON requires some types to be handled exceptionally.

Hooks provided by lua-protobuf are insufficient as they:

1. cannot hook native types (required for bytes)
2. are not executed recursively (required for Any)

Further, gateway must read custom JSON names from proto definitions. Conversion must also allow to keep camel_case names, enum names (instead of numbers) and to emit also JSON with default values.

Full changelog

• added file plugins/grpc-gateway/protojson.lua handling proto->JSON formatting
• updated rockspecs to include this new file

In tools/grpc.lua:

• refactored to be more readable
• add_path and parse_file methods are now exposed instead of for_each_method
• extra hook on every field of every message included (to extract json_name option value)
• path( "spec/fixtures/grpc" ) excluded from internally added paths as they are only intended for testing and might contain files conflicting with user code. Mixing testing code with production code is bad practice anyway.

in plugins/grpc-gateway/deco.lua

• removed code from ancient times where include_imports directive was false.
• adapted to new tools/grpc.lua
• decoding/encoding of messages is now done by using protojson.lua

in plugins/grpc-web

• adapted to new tools/grpc.lua
• no functional changes have been made
• schema extended to allow also additional files (useful for google.protobuf.Any conversions)
• schema allows to add also additional import paths (useful for libraries)

Also fixed bug when shorter binding path matched longer one (e.g. /test matched also /test/something/else).

Issue reference

Fix #7469

KAG-2262(internal ticket)

Todo: more test coverage

[CLAassistant]

All committers have signed the CLA.

[StarlightIbuki]

We appreciate your contribution. This is a large PR and we need time to review it.

[git-torrent]

We appreciate your contribution. This is a large PR and we need time to review it.

Thank you.

All the tests are working except spec/02-integration/07-sdk/03-cluster_spec.lua which fails for both, cassandra and postgres. I hope it is not caused by my changes (plugin is tested daily in our environment). I have no idea how to fix these.

[StarlightIbuki]

All the tests are working except spec/02-integration/07-sdk/03-cluster_spec.lua which fails for both, cassandra and postgres. I hope it is not caused by my changes (plugin is tested daily in our environment). I have no idea how to fix these.

I'm trying to investigate the issue.

[StarlightIbuki]

All the tests are working except spec/02-integration/07-sdk/03-cluster_spec.lua which fails for both, cassandra and postgres. I hope it is not caused by my changes (plugin is tested daily in our environment). I have no idea how to fix these.

I'm trying to investigate the issue.

The failing test runs slow in my env and passed. Guess we need a longer timeout.

[StarlightIbuki]

Please rebase the PR. We are dropping the wRPC protocol, which should bring more freedom to how we support gRPC.

[StarlightIbuki]

Sorry. This PR takes really long to review and new changes seem to collide with it. Could you solve conflicts?

[git-torrent]

There are two major issues that stand in our way:

1. my lack of time (every conflict resolution takes me lot of time as I need to recall what to do and where, my daytime job has nothing to do with Lua)
2. my inability to make unit-tests better than they are right now in reasonable time

I can resolve conflicts this week (maybe today if lucky), but the unit-tests require substantially more time to learn and master.

[StarlightIbuki]

@git-torrent As this is a very large PR and hard to maintain, and we see no hope for it to be merged soon, I'm thinking about taking over this PR myself if you don't mind. Of course, you will be given credit. What do you think? My first step would be to break this PR into 3 or more PRs so it's easier to review.

[git-torrent]

@git-torrent As this is a very large PR and hard to maintain, and we see no hope for it to be merged soon, I'm thinking about taking over this PR myself if you don't mind. Of course, you will be given credit. What do you think? My first step would be to break this PR into 3 or more PRs so it's easier to review.

I appreciate your offer. With this PR having more badges (labels) than old military officer, I think your care would help.

Just one thing please: Names of the variables/functions are optimized for me from the future. Of course, feel free to optimize the code, but until merged, I will be still in charge of keeping our (company) fleet of Kongs up to date, which require this gRPC<->REST operating. Keeping the variable names (when possible) would make this upgrade&patch process easier.

If I can be of any further help, please write me.

Thank you :)

[hbagdi]

Closing this due to lack of activity. Please re-open if needed.

[git-torrent]

@hbagdi > Closing this due to lack of activity. Please re-open if needed.

How can you close XXL PR where lack of inactivity is clearly an effect of being XXL? You have got bunch of functionality for free. I do not expect to be paid, I expect Kong to pay little interest in community's work. This way I would use Traefik next time :(

I kindly ask to reopen this PR. Thank you.

[StarlightIbuki]

@git-torrent Sorry for the confusion. The closing is done in a process without looking at what the PR is doing and has not much intention on it. I'm reopening this PR but sadly I'm occupied by other issues and cannot find time to work on this recently. Please understand that we need to prioritize our work for different urgency. I'll convert this into a draft PR so it won't show up in the list and get closed mistakenly.

[git-torrent]

@git-torrent Sorry for the confusion. The closing is done in a process without looking at what the PR is doing and has not much intention on it. I'm reopening this PR but sadly I'm occupied by other issues and cannot find time to work on this recently. Please understand that we need to prioritize our work for different urgency. I'll convert this into a draft PR so it won't show up in the list and get closed mistakenly.

@StarlightIbuki I am afraid I need your help with unit tests. I have to rebase this branch from time to time to prepare patch for our deployments. It is useful to have unit-tests done, but I cannot do that. Can you please point me to some exemplary solution of what you wanted? I tried to look at existing test-cases, but mine are still failing. Thank you

[StarlightIbuki]

@git-torrent Hi. Glad to know you're still interested in maintaining the RP. When I said taking over, I mean I will help to rework the PR, write proper tests for it and etc. It's fine to not fix the test or rebase to keep maintaining the PR. The good news is that I have planned this PR's work. I will hand on it after the 3.4 release if no unexpected urgent work gets in the way. If you still want to write the unit test, please tell me the purpose of the test (which part is tested? What do we want to test?) and the problem you're facing.

[git-torrent]

@git-torrent Hi. Glad to know you're still interested in maintaining the RP. When I said taking over, I mean I will help to rework the PR, write proper tests for it and etc. It's fine to not fix the test or rebase to keep maintaining the PR. The good news is that I have planned this PR's work. I will hand on it after the 3.4 release if no unexpected urgent work gets in the way. If you still want to write the unit test, please tell me the purpose of the test (which part is tested? What do we want to test?) and the problem you're facing.

Good news :) Thank you for helping me, I'll try my best.

I need to set up 8 different Kongs for every combination of three parameters. Combinations are currently visible from names of test files in 28-grpc-gateway folder. Then I would like to run series of tests, basically the same test-cases in 8 different contexts.

I does not have to be 8 files, I just found them more maintainable than one huge. If one huge file is preferred, do I need to use another description block to start/stop new kong?

[StarlightIbuki]

@git-torrent Hi. I'm gradually starting the rework of this PR. First thing is to make a full list of the improvement this PR wants to do. Please correct me/add to the list if I made a mistake or miss something.

1. Wrap the pb library so we can isolate the hack to it;
2. Better handling of pb messages:
3. Make the empty table/array distinctable. This is already done: #10790;
4. More careful handling of numbers, say, int64, NaN, etc, and boolean/byte types;
5. Name conversions (camel/snake case);
6. Support of .google.protobuf.*
7. Allow different configs to coexist for different instances of isolated pb library, including decode_default_message use_proto_names, local enum_as_name, emit_defaults
8. Make decoding/encoding behavior configurable for the grpc-gateway plugin;
9. Endpoint sort;
10. Fix path searching;
11. additional_protos;
12. PUT method;
13. json_name attribute support

[git-torrent]

@git-torrent Hi. I'm gradually starting the rework of this PR. First thing is to make a full list of the improvement this PR wants to do. Please correct me/add to the list if I made a mistake or miss something.

1. Wrap the pb library so we can isolate the hack to it;
2. Better handling of pb messages:
3. Make the empty table/array distinctable. This is already done: fix(*): gRPC and cJSON's behavior to empty array #10790;
4. More careful handling of numbers, say, int64, NaN, etc, and boolean/byte types;
5. Name conversions (camel/snake case);
6. Support of .google.protobuf.*
7. Allow different configs to coexist for different instances of isolated pb library, including decode_default_message use_proto_names, local enum_as_name, emit_defaults
8. Make decoding/encoding behavior configurable for the grpc-gateway plugin;
9. Endpoint sort;
10. Fix path searching;
11. additional_protos;
12. PUT method;

Thank you. I am not sure if I can map your steps to this PR completely. I miss a support for json_name option in proto files to handle overrides.

Also, I am not sure if you want to preserve proto<->json transcoding as a standalone code. It is a standalone library in C++, C# and Go, so I guess it is better for maintenance. For pb.(encode_)hook to work properly, it must be able to handle scalar types (bytes in particular) and also be recursive (to decode Any type).

I would also like to prepare unit-tests to be sure none of the steps breaks existing (PR) code. I still struggle to run tests in different configurations.

What do you think?

[StarlightIbuki]

@git-torrent I Need to mention that maybe not all the items in the list would be merged... We're going through an evaluation process.

I miss a support for json_name option in proto files to handle overrides.

Edited.

Also, I am not sure if you want to preserve proto<->json transcoding as a standalone code. It is a standalone library in C++, C# and Go, so I guess it is better for maintenance.

Sure. Better maintenance is also a target of this task.

For pb.(encode_)hook to work properly, it must be able to handle scalar types (bytes in particular) and also be recursive (to decode Any type).

Scalar types handling is included as a part of #4. I'm not very sure what the "recursive mean". Could you elaborate?

I would also like to prepare unit-tests to be sure none of the steps breaks existing (PR) code. I still struggle to run tests in different configurations.

No need to worry. I will add tests (by referencing what you've written) for each of those changes. And I'm inviting you to review PRs, so you can also check if the design does what you expect and won't break your usage.

[git-torrent]

I'm not very sure what the "recursive mean". Could you elaborate?

Message serialized into google.protobuf.Any can contain another google.protobuf.Any attribute and so on. To transcode them the hook must treat Any as new message instance and start over. Moreover, the hook must know the @type in advance. This is handled by additional_protos or some other kind of DescriptorPools from C++ or python protobuf implementations.

And I'm inviting you to review PRs, ...

Gladly. My goal is to have PROTO-JSON transcoding in Lua on par with Google's native implementations. Thank you! :)

[Water-Melon]

Message serialized into google.protobuf.Any can contain another google.protobuf.Any attribute and so on. To transcode them the hook must treat Any as new message instance and start over.

Yeah, I tried this proto that given below in Lua and Python.

message Phone {                                                                                                                                
   optional string name        = 1;                                                                                                            
   optional int64  phonenumber = 2;                                                                                                            
   optional Person item        = 3;                                                                                                            
}                                                                                                                                              
message Person {                                                                                                                               
   optional string name     = 1;                                                                                                               
   optional int32  age      = 2;                                                                                                               
   optional string address  = 3;                                                                                                               
   repeated Phone  contacts = 4;                                                                                                               
}

In python, the reference of a Person object can not be assigned to Phone's item. But in Lua, you can assigned the Person object (such as named person) to Phone's item, and it makes stack overflow when I was using lua-protobuf to encode message.

So I think may be we shouldn't to build a ref cycle. Type can refer to itself, but data cannot. Maybe we can try to imagine how we can implement the encode function in this situation. Encoding one message object A, and it contains another message object B, and B also need the A, encoder will never obtain the complete message data. So the stack overflowed

[git-torrent]

Hello, how is this going?

[StarlightIbuki]

@git-torrent I underestimated the amount of work and am sorry for failing to keep the promise. 3.5 is filled with other features and for personal reasons, I was off for nearly 2 weeks. This improvement cannot be fit in a gap and has to be planned. I'm trying to make this a part of 3.6.

[git-torrent]

@git-torrent I underestimated the amount of work and am sorry for failing to keep the promise. 3.5 is filled with other features and for personal reasons, I was off for nearly 2 weeks. This improvement cannot be fit in a gap and has to be planned. I'm trying to make this a part of 3.6.

Can I be of any help? I would still gladly contribute to unit-tests (if I know how to do them :) )

[StarlightIbuki]

@git-torrent I underestimated the amount of work and am sorry for failing to keep the promise. 3.5 is filled with other features and for personal reasons, I was off for nearly 2 weeks. This improvement cannot be fit in a gap and has to be planned. I'm trying to make this a part of 3.6.

Can I be of any help? I would still gladly contribute to unit-tests (if I know how to do them :) )

We want to rework the PR and before the implementation is written we don't know how to unit test them. The best help we could expect from you should be a list of the improvements that we want in detail like the list we made before. To be honest, I don't fully understand the PR's intention, and the list I made is not very in-depth.

[git-torrent]

We want to rework the PR and before the implementation is written we don't know how to unit test them. The best help we could expect from you should be a list of the improvements that we want in detail like the list we made before. To be honest, I don't fully understand the PR's intention, and the list I made is not very in-depth.

Google provides automated GRPC-REST translation in their own API gateway solution. Kong provides its own, but lacks many of the translation features required by Google. The goal of this PR is to bring google-compliant translation into Kong.

For this purpose Google offers several libraries (go, c++, C#, python), but none for lua. protojson.lua in PR is this missing library for Lua (personally crafted :) ). Maybe it might easier to replace it by a dll (made of official Google C++ code) loaded into Lua by LuaBridge/LuaBind.

The rest is to use protojson library in grpc-gateway plugin.

Does this bring more light into this PR?

[StarlightIbuki]

@git-torrent 3.5 is delayed and we have planned so many things for 3.6. I see little hope that we can fit this improvement into the foreseeable future... In fact, we have many other possible improvements in the queue. Sorry again for this message. I reviewed this fix and have some personal views. IMO, your effort is worth a dedicated repository. It may be easier to do as a separate functionality instead of a part of Kong. It could be a set of functions or a module (to_JSON, from_JSON?) of lua-protobuf, or an extension to it (not necessarily an official part of the lib). That way it should also be easier to unit test (and you don't need to follow the Kong's conversion to write the tests). Then from Kong, the PR should be much smaller and easier to review.

[git-torrent]

@git-torrent 3.5 is delayed and we have planned so many things for 3.6. I see little hope that we can fit this improvement into the foreseeable future... In fact, we have many other possible improvements in the queue. Sorry again for this message. I reviewed this fix and have some personal views. IMO, your effort is worth a dedicated repository. It may be easier to do as a separate functionality instead of a part of Kong. It could be a set of functions or a module (to_JSON, from_JSON?) of lua-protobuf, or an extension to it (not necessarily an official part of the lib). That way it should also be easier to unit test (and you don't need to follow the Kong's conversion to write the tests). Then from Kong, the PR should be much smaller and easier to review.

Oh, that could be. Is there any guide to write modules or extension to lua-protobuf? Or some example on the web (google did not help me much :) ). I am not the C guy, so I have to learn that first.

[StarlightIbuki]

@git-torrent 3.5 is delayed and we have planned so many things for 3.6. I see little hope that we can fit this improvement into the foreseeable future... In fact, we have many other possible improvements in the queue. Sorry again for this message. I reviewed this fix and have some personal views. IMO, your effort is worth a dedicated repository. It may be easier to do as a separate functionality instead of a part of Kong. It could be a set of functions or a module (to_JSON, from_JSON?) of lua-protobuf, or an extension to it (not necessarily an official part of the lib). That way it should also be easier to unit test (and you don't need to follow the Kong's conversion to write the tests). Then from Kong, the PR should be much smaller and easier to review.

Oh, that could be. Is there any guide to write modules or extension to lua-protobuf? Or some example on the web (google did not help me much :) ). I am not the C guy, so I have to learn that first.

By module, I mean "Lua module". There's no such mechanism of lua-protobuf to support plugins. To begin with, I guess you could request some features from the library maintainer, like custom handlers for build-in types or even direct fixes to those build-in types' behavior (with an option). You could write your module in Lua to handle the proto file importing, and if the features you requested do not make it into the library, you could wrap over the original library, or make some hooks/monkey patches on it.

[StarlightIbuki]

And in my very personal POV, there's no need to write such a library in C given LuaJIT emits quite efficient binary and cooperates with C types well. If you still want to PR to lua-protobuf directly, you could start with Lua C APIs: https://www.lua.org/manual/5.1/manual.html#lua_Alloc