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

gRPC Client Library Migrator #11

Merged
merged 94 commits into from
Feb 8, 2022
Merged

gRPC Client Library Migrator #11

merged 94 commits into from
Feb 8, 2022

Conversation

Supereg
Copy link
Member

@Supereg Supereg commented Jan 4, 2022

gRPC Client Library Migrator

♻️ Current situation & Problem

The REST Migrator is the only supported migrator by the ApdoiniMigrator as of now.
We want to explore how automated client library migration fits other API types.

💡 Proposed solution

We implement a GRPCMigrator for generating and migration gRPC clients for ApodiniGRPC web services, to demonstrate the feasibility of automated machine-readable migration guide for RPC-based API types.

⚙️ Release Notes

  • This PR breaks the command line interface (adds new subcommand for rest and grpc for generate/migrate subcommands + renames -p option to -n).
  • This PR introduces document versions 2.1.0.
  • GRPCMigrator adds support for generating and migration client libraries for ApodiniGRPC-based web services. Use it with the migrator genreate/migrate grpc subcommands. You need to supply the APIDocument, the proto file (retrieved from the respective endpoint of the ApodiniGRPC reflection service), and optionally the MigrationGuide.

➕ Additional Information

This PR currently doesn't handle the installation of the protoc-gen-grpc-migrator protoc plugin. Currently, you need to add this binary to the PATH yourself. You may also use the according command line option to specify the path to the executable.

Related PRs

Testing

Testing was added. Primarily, end 2 end tests and additional tests which weren't perfectly covered by the QONEQTIC example.

Reviewer Nudging

Well, I assume this is rather a huge PR and a hell to review. You may just want to start at the entry point, the ApdoiniMigratorCLI. From that point on you may explore GRPCMethod (method generation and migration), GRPCMessage and GRPCEnum and their child types (fields/enum cases). Furthermore, MigrationContext.swift has some important processing you might want to check out, as it handles parameter combination, response type wrapping and everything which is required to ensure that ApodinIGRPC supplied identifiers (GRPCName, GRPCNumber, ...) are updated/encoded properly.

Code of Conduct & Contributing Guidelines

By submitting creating this pull request, you agree to follow our Code of Conduct and Contributing Guidelines:

@codecov
Copy link

codecov bot commented Jan 4, 2022

Codecov Report

Merging #11 (6743c17) into develop (8e9b051) will decrease coverage by 3.89%.
The diff coverage is 79.34%.

❗ Current head 6743c17 differs from pull request most recent head 83e00ce. Consider uploading reports for the commit 83e00ce to get more accurate results

Impacted file tree graph

@@             Coverage Diff             @@
##           develop      #11      +/-   ##
===========================================
- Coverage    89.97%   86.09%   -3.88%     
===========================================
  Files          135      199      +64     
  Lines         5284     9641    +4357     
===========================================
+ Hits          4754     8299    +3545     
- Misses         530     1342     +812     
Impacted Files Coverage Δ
...grator/LibraryStructure/Components/Directory.swift 93.75% <ø> (ø)
...niMigrator/LibraryStructure/Components/Empty.swift 0.00% <ø> (ø)
...or/LibraryStructure/Components/GeneratedFile.swift 93.34% <ø> (ø)
...tor/LibraryStructure/Components/ResourceFile.swift 78.00% <ø> (ø)
.../LibraryStructure/Components/Root/ReadMeFile.swift 100.00% <ø> (ø)
...braryStructure/Components/Root/RootDirectory.swift 90.91% <ø> (ø)
...tor/LibraryStructure/Components/Root/Sources.swift 100.00% <ø> (ø)
...yStructure/Components/Root/StubLinuxMainFile.swift 100.00% <ø> (ø)
...ents/Root/SwiftPackageFile/PackageDependency.swift 80.00% <ø> (ø)
...ponents/Root/SwiftPackageFile/PackageProduct.swift 100.00% <ø> (ø)
... and 199 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8e9b051...83e00ce. Read the comment docs.

@Supereg Supereg changed the title [WIP] gRPC Client Library Migrator gRPC Client Library Migrator Feb 8, 2022
@Supereg Supereg marked this pull request as ready for review February 8, 2022 01:49
@Supereg
Copy link
Member Author

Supereg commented Feb 8, 2022

@PSchmiedmayer

To reflect on the state of the PR. It may be considered finished apart from the following points.

  • Its blocked by Add homebrew support and 'installprotoc' input .github#10 (which should also bump the code coverage by a few percent)
  • It is somewhat impacted by [Bug]: ProtobufferCoding doesn't add explicit presence information to optional message properties. Apodini#424. The described issue will result in wrongfully generate proto files which has some impact on the generated output (nothing sever, only really matters in combination with certain change types). It mainly affects the ApodiniMigratorExample. The proto file included in this PR was manually corrected.
  • Usability wasn't really considered yet from my side. Meaning, once currently must manually link the protoc-gen-grpc-migrator executable before the grpc migrator is usable. I think the simplest resolution would be to dynamically determine the location of the executable (like we do in the tests; if that is possible in non test targets) and pass that to the protoc compiler. EDIT as described, the migrator will search for the binary in the local dev environment as a usability workaround.
  • Testing networking functionality is currently blocked. It seems like grpc-swift sends a windowUpdate which ApodiniGRPC doesn't support. I have talked to @lukaskollmer to work towards resolving this, but both of us haven't really work on it afaik. The error on the ApodiniGRPC side looks like the following:
2022-02-06T20:45:56+0100 notice org.apodini.application : Configuring NIO channel pipeline. TLS: true, HTTP/2: true
2022-02-06T20:45:56+0100 notice [GRPCRequestDecoder] : init()
2022-02-06T20:45:56+0100 error [GRPCRequestDecoder] : Received unexpected frame: windowUpdate(windowSizeIncrement: 8323073). Closing stream in response.
2022-02-06T20:45:56+0100 error [GRPCRequestDecoder] : Received unexpected frame on closed channel: data(NIOHTTP2.HTTP2Frame.FramePayload.Data(data: IOData { ByteBuffer { readerIndex: 0, writerIndex: 0, readableBytes: 0, capacity: 0, storageCapacity: 0, slice: _ByteBufferSlice { 0..<0 }, storage: 0x00006000015e0390 (0 bytes) } }, endStream: true, _paddingBytes: nil))

@Supereg
Copy link
Member Author

Supereg commented Feb 8, 2022

Testing networking functionality is currently blocked. It seems like grpc-swift sends a windowUpdate which ApodiniGRPC doesn't support. I have talked to @lukaskollmer to work towards resolving this, but both of us haven't really work on it afaik. The error on the ApodiniGRPC side looks like the following:

With the PR on the Apodini repo we will introduce a workaround by ignoring those frames. @lukaskollmer might investigate this in the future. Apart from that, networking functionality is tested and fully operational.

@Supereg Supereg merged commit 46dddf5 into develop Feb 8, 2022
@Supereg Supereg deleted the feature/grpc-migrator branch February 8, 2022 21:53
@Supereg Supereg mentioned this pull request Feb 8, 2022
1 task
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.

2 participants