refactor(#1640): support multiple packages (breaking changes)

[zry656565]

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: https://github.com/nestjs/nest/blob/master/CONTRIBUTING.md
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: #1640

What is the new behavior?

const grpcServerOptions: GrpcOptions = {
  transport: Transport.GRPC,
  options: {
    url: 'localhost:5577',
    packages: ['product', 'order'],      // <--- this line
    protoPath: join(__dirname, './proto/payment.proto'),
    loader: {
      includeDirs: [
        join(__dirname, 'proto'),
      ],
    }
  },
} 

Does this PR introduce a breaking change?

[x] Yes
[ ] No

Other information

[coveralls]

Pull Request Test Coverage Report for Build 1890

• 25 of 25 (100.0%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.01%) to 93.709%

---

Totals
Change from base Build 1886:	0.01%
Covered Lines:	3004
Relevant Lines:	3149

---

💛 - Coveralls

[zry656565]

Maybe I need to update sample/04-grpc also?
And another choice not to include breaking change is that keep package and add a new packages property. That is,

interface GrpcOptions {
  // ...
  options: {
    package?: string
    packages?: string[]
  }
}

if one define a option like:

export const grpcServerOptions: GrpcOptions = {
  transport: Transport.GRPC,
  options: {
    url: `localhost:${config.payment.grpcPort}`,
    package: 'payment',
    packages: ['product', 'order'],
    protoPath: join(__dirname, './proto/payment.proto'),
    loader: {
      includeDirs: [join(__dirname, 'proto')],
    },
  },
}

then the actual used packages will be the merge of these two properties:

['payment', 'product', 'order']

[zry656565]

The tests randomly failed in node v8 or node v11. I have no idea about it.
And it seems that there is no way for me to retry it unless I update the commit message and force push this commit again.

[anton-alation]

One question towards the multi-package support, usually you start most of your interfaces with root namespace like: myservice and then you adding something like myservice.products and myservice.orders, that allows protobuf to compile all required messages from one root which allows you to have some common or shared messages to be under the root: myservice.commons as of example.

While compilation on rootless packaging probably may fail for some of the platforms supported by protobuf.

Did you test that kind of a pattern for example on Python, Java and Go generation with GRPC enabled, and did it work and compile correctly?

[zry656565]

@anton-alation I just checked with my workmate, he said multi-package is fully supported in go gRPC. Not sure about other languages, I will figure it out if I have time.

[zry656565]

@anton-alation I agree with you that in most cases, we need only a root package for a gRPC service, but as client side, multi-package is needed. for example, if a Nest http server is an interface for several different gRPC microservices, and there're multiple packages served by those gRPC endpoints. We have to load more than one package which cannot be covered by current code base.

[anton-alation]

@anton-alation I agree with you that in most cases, we need only a root package for a gRPC service, but as client side, multi-package is needed. for example, if a Nest http server is an interface for several different gRPC microservices, and there're multiple packages served by those gRPC endpoints. We have to load more than one package which cannot be covered by current code base.

By description you provided I can assume something is not right with the code examples above then:

{
    ...
    packages: ['product', 'order'],      // <--- this line
    protoPath: join(__dirname, './proto/payment.proto'),
    ...
}

Code example saying you loading all packages from the same file, while you describing multiple roots to be loaded.

My assumption is that you want to have something like that:

const grpcProductServerOptions: GrpcOptions = {
  transport: Transport.GRPC,
  options: {
    url: 'localhost:5577',
    package: "product",
    protoPath: join(__dirname, './proto/product_service.proto'),
  },
} 
const grpcOrdersServerOptions: GrpcOptions = {
  transport: Transport.GRPC,
  options: {
    url: 'localhost:5578',
    package: "orders",
    protoPath: join(__dirname, './proto/order_service.proto'),
  },
} 
/*
   And controllers like
*/
@Controller()
export class ProductService {

}
@Controller()
export class OrderService {

}

And then join those under the Nest HTTP Client proxy like:

@Client({
  transport: Transport.GRPC,
  options: {
    package: "product",
    protoPath: join(__dirname, './proto/product_service.proto'),
  },
})
clientProducts: ClientGrpc;

@Client({
  transport: Transport.GRPC,
  options: {
    package: "orders",
    protoPath: join(__dirname, './proto/order_service.proto'),
  },
})
clientOrders: ClientGrpc;

onModuleInit() {
  this.productService = this.clientProducts.getService<ProductService>('ProductService');
  this.orderService = this.clientOrders.getService<OrderService>('OrderService');
}

(c) https://docs.nestjs.com/microservices/grpc

And that may work without additional adjustments of the codebase.

But please let me know if that would not be a reasonable or workable solution.

[zry656565]

The Client part looks fairly good to me. 👍
But I think that it would be better if we could serve several service under one single port. for example:

const grpcProductServerOptions: GrpcOptions = {
  transport: Transport.GRPC,
  options: {
    url: 'localhost:5577',
    package: {
      product: join(__dirname, './proto/product_service.proto'),
      order: join(__dirname, './proto/order_service.proto'),
    },
  },
} 

Certainly, it's a better-to-have feature, and current solution provided by you, is okay to me.

[aaskandrew]

The Client part looks fairly good to me.
But I think that it would be better if we could serve several service under one single port. for example:

const grpcProductServerOptions: GrpcOptions = {
  transport: Transport.GRPC,
  options: {
    url: 'localhost:5577',
    package: {
      product: join(__dirname, './proto/product_service.proto'),
      order: join(__dirname, './proto/order_service.proto'),
    },
  },
} 

Certainly, it's a better-to-have feature, and current solution provided by you, is okay to me.

"package" is an grpc namespace. Namespace mutation inside the GrpcOptions in not well. May be it is sollution #1774 ?

[zry656565]

@aaskandrew Not exactly, but thanks for informing me this.
I guess multi-package is still not supported in the PR you mentioned.

[AlexDaSoul]

This is not work for client

[zry656565]

hi, @AlexDaSoul . For client side, I accepted @anton-alation 's advise, so don't worry about it. I will revert those changes on client when I have time.

[aaskandrew]

PR #1774 is about ability to load more then one proto file. 'grpc' package allows this functionality. But this PR about load multiple namespaces in the nest GrpcClient. That can lead to conflicts in grpc services names with multi level namespaces. More over, problem of this PR resolves with multi level namespaces.

file: order.proto
package common.order;
...

file: product.proto
package common.product;
...

file: some myGrpcConfig.ts:
myGrpcConfig = {
...
package: 'common',
protoPath: ['order.proto', 'product.proto'], <======= PR #1774
...
}

Problem is that construction

@GrpcMethod('order.OrderService')
@GrpcMethod('product.ProductService')

is working but

@client(myGrpcConfig) private readonly client: ClientGrpc;
private service: product.ProductService;

onModuleInit() {
this.service = this.client.getService('product.ProductService'); <=== not working
}

This problem for feature PR.

[anton-alation]

This problem for feature PR.

It's really not obvious to figure out the problem from the message where the code isn't formatted into the blocks.

[aaskandrew]

sorry

this.client.service('product.ProductService'); <== this is a current problem for me, it is not working

It works only so:

In the GrpcConfig object:
package: 'common.product',

In the code:
this.client.getService('ProductService');

[aaskandrew]

My current production grpc config code. I prefer to remove <string><unknown> and get ability to load multiply files for GrpcClient. PR #1774 would do it.

const tradeOptions = {
    grpcService: <GrpcOptions>{
        transport: Transport.GRPC,
        options: {
            url: env.GRPC_TRADE_SERVICE || '0.0.0.0:9016',
            package: 'sdex.webtrade',
            protoPath: <string>(
                (<unknown>[
                    'candles.proto',
                    'health-webtrade.proto',
                    'marketdata.proto',
                    'markets.proto',
                    'order-book.proto',
                    'orders.proto',
                    'trading.proto',
                    'version-webtrade.proto'
                ])
            ),
            loader: {
                includeDirs: [getProtoPath('common'), getProtoPath('webtrade')]
            }
        }
    },

    grpcSdexapiCandles: <GrpcOptions>{
        transport: Transport.GRPC,
        options: {
            url: env.GRPC_LEDGER_SERVICE,
            package: 'sdex.sdexapi.candles',
            protoPath: getProtoPath('sdexapi') + '/candles.proto'
        }
    },

    grpcSdexapiOrderBook: <GrpcOptions>{
        transport: Transport.GRPC,
        options: {
            url: env.GRPC_LEDGER_SERVICE,
            package: 'sdex.sdexapi.orderbook',
            protoPath: getProtoPath('sdexapi') + '/order-book.proto'
        }
    }
};

[anton-alation]

sorry

this.client.service('product.ProductService'); <== this is a current problem for me, it is not working

It works only so:

In the GrpcConfig object:
package: 'common.product',

In the code:
this.client.getService('ProductService');

About code formatting: please use 3 backticks ` for format your code injections like that -> code goes here, refer Cheatsheet Code section

About client malfunction: so you saying GrpcClient cannot select nested namespaces defined in Protobuf? If that so, please confirm that assumption and I will be looking over GrpcClient behavior on that agenda, mainly just because it was me adding nested namespaces into Nest GrpcMethod :)

[aaskandrew]

About client malfunction: so you saying GrpcClient cannot select nested namespaces defined in Protobuf? If that so, please confirm that assumption and I will be looking over GrpcClient behavior on that agenda, mainly just because it was me adding nested namespaces into Nest GrpcMethod :)

• I confirm. I did not have time to investigate the problem, and in principle I see nothing wrong with importing one level.
• I added the code for config above, is this enough?

[anton-alation]

My current production grpc config code. I prefer to remove <string><unknown> and get ability to load multiply files for GrpcClient. PR #1774 would do it.

I feel that it's just about defining your .proto files in another way, for example, you can just have one proto-file as a root file for all of those:

webtrade.proto

syntax = "proto3";
import public "candles.proto";
import public "health-webtrade.proto";
import public "marketdata.proto";
import public "markets.proto";
import public "order-book.proto";
import public "orders.proto";
import public "trading.proto";
import public "version-webtrade.proto";

And have then:

grpcService: <GrpcOptions>{
        transport: Transport.GRPC,
        options: {
            url: env.GRPC_TRADE_SERVICE || '0.0.0.0:9016',
            package: 'sdex.webtrade',
            protoPath: "webtrade.proto",
            loader: {
                includeDirs: [getProtoPath('common'), getProtoPath('webtrade')]
            }
        }
    }

And that (maybe some adjustments need) would resolve all of the files and services

[aaskandrew]

no, so:

import public "../common/health-webtrade.proto";
import public "../common/version-webtrade.proto

and now 'grpc-web' is not working on the frontend :(

[aaskandrew]

Keep different services in the different files more more better for maintenance. Health and Version services I need insert into every nest microservice.

[anton-alation]

• I confirm. I did not have time to investigate the problem, and in principle I see nothing wrong with importing one level.

Sure let me look on @GrpcClient namespace resolution capabilities to match @GrpcMethod resolution capabilities then.

Config files are enough, still seems like problem resolution went too far away into the direction of having some very customized approach rather going through a simplified one.

Please sync if you would be having any problems loading your files with a simplified approach. If you'll be trying that one of course.

[AlexDaSoul]

This is not a simplified approach, but the only one. When for use only 2 proto need adds 3th proto

[anton-alation]

no, so:

import public "../common/health-webtrade.proto";
import public "../common/version-webtrade.proto

and now 'grpc-web' is not working on the frontend :(

You can't use ../ or ./ in protobuf, it's restricted approach and not supported for static codegen. What you need ot use is a namespace resolution inside of your protobuf.

If you want to discuss more on this: please form an abstracted version of few of your files and I will be trying to help you with correct protobuf approach on the shared messages between namespaces.

However if you thinking that you want to stay on the dynmaic codegen (which is doing some things out of common protoc rules) — don't expect any other platform to compile your proto files in a standard way.

[aaskandrew]

Please sync if you would be having any problems loading your files with a simplified approach. If you'll be trying that one of course.

It works, and it was first version. Keep multiply proto files preferable for me.

[zry656565]

Second use case:
I use two different npm package with different set of proto files in each npm package. Each npm package maintains by independent from me owners. Why I must to create my own "barrel" file for this packages instead of simply include those into nets GrpcConfig as separate files?

Protobuf files are all about business logic. Is there any reason that we would import a proto file from others' npm packages?

[AlexDaSoul]

Protobuf files are all about business logic. Is there any reason that we would import a proto file from others' npm packages?

For example when multiple owners of the packages and packages locations in different repositories

[kamilmysliwiec]

This is the longest discussion in Nest PRs so far. Love it :)

Next, if grpc package allows multiply proto packages (protoc works well), why to restrict it in the nest? Problems with another languages is problems of that langs and must leeds to pull requests in that langs.

The thing is that gRPC is a "language-agnostic" thing and thus we should try to stay as close as possible to other implementations.

[AlexDaSoul]

The thing is that gRPC is a "language-agnostic" thing and thus we should try to stay as close as possible to other implementations.

Sure. So need to have the alternative to choose

[anton-alation]

Second use case:
I use two different npm package with different set of proto files in each npm package. Each npm package maintains by independent from me owners. Why I must to create my own "barrel" file for this packages instead of simply include those into nets GrpcConfig as separate files?

Are we still in a canvas of microservices? :) It seems you trying to describe a macro-service with a lot of logic residing inside of one executor. For that one to succeed you certainly need also to define all of your parts to be well scalable within all of the interfaces you introducing, and that probably a bit tougher architectural conception to go with, rather than tiny-to-small logic segregation which allows components to stay maintainable at a reasonable point.

Architectural discussions do carry even more controversy than interface discussions, so hopefully we'll not get into that messy swamp :)

For example when multiple owners of the packages and packages locations in different repositories

Same thing here. Remember what are you trying to do here within this framework: reasonable microservices which easy to maintain, deploy and evolve.

You can have all of the parts acting on the different ports independently and that may save your project in the future when you hit the traffic and will be in the situation of horizontal scaling. What you calling a bug now, may turn into a feature which would help resolve that situation in future.

P.S. I need to repeat that I am not saying that not allowing to load multiple proto-files should stay like that, but certainly solution which we have now supported more developer culture and accuracy on interfaces rather than a solution with more freedom. Balance is a hard thing to achieve, and certainly, there should be a proposal which covers rest of the cases with most accuracy on interfaces in mind.

[anton-alation]

Next, if grpc package allows multiply proto packages (protoc works well), why to restrict it in the nest? Problems with another languages is problems of that langs and must leeds to pull requests in that langs.

1. Protoc allows, but it compiles them for you to take them and establish them on different ports. Nest automates port per root namespace — so Nest acts exactly as protoc.

2. The second statement breaking the fundamental principle of Engineers following standards. You practically saying that standards aren't for you and let break the world apart because you don't like how it structured now.

It's very hard to have good unified interfaces for us all to work with, and it's even harder to make every platform to follow some new good ones. The reason that we'll here is that we all liked conception of Proto + GRPC and that is a success of one of the thousands of iterations of network interfaces through the last 30 years of software engineering controversy.

[aaskandrew]

protobufjs documentation:
http://dcode.io/protobuf.js/Root.html#load
filename | string|Array<string> | Names of one or multiple files to load

protoc documentation:
https://developers.google.com/protocol-buffers/docs/proto3
"You must provide one or more .proto files as input. Multiple .proto files can be specified at once. Although the files are named relative to the current directory, each file must reside in one of the IMPORT_PATHs so that the compiler can determine its canonical name."

Why they don't require to create '"barrel" file? Is it less "developer culture and accuracy on interfaces"?

About standards.
https://developers.google.com/protocol-buffers/docs/reference/cpp/#google.protobuf.compiler
"You want to parse .proto files at runtime." - Is it standard to create protobuf dynamically from set of proto files?

[aaskandrew]

https://developers.google.com/protocol-buffers/docs/proto#packages
"You can add an optional package specifier to a .proto file to prevent name clashes between protocol message types." - so, empty root namespace '' is standard namespace and must be supported.

[anton-alation]

protobufjs documentation:
http://dcode.io/protobuf.js/Root.html#load
filename | string|Array<string> | Names of one or multiple files to load

Totally! It certainly will parse proto files and stack them to correct trees!!! :)

Caveat: Nest here trying to help you to automate bootstrapping of the GRPC Server for some root namespace, and certainly not trying to help you to load some abstract protobuf with no meaning for Nest framework itself.

You certainly need to separate two things from each other: Nest bootstrap for service and ability to load protobuf, and that's why namespace is important here: we loading services related to one defined, rather than we loading services for anything possible. Somewhere should be that fixation point which will make development a bit more sane rather than too abstract.

I probably will be repeating myself but we need to take into account that Nest currently matches one proto-namespace for some port. So if you have 2 namespaces like one[.orders] and two[.products] — only one of them will be loaded for a single port. And that may raise confusion when someone defines those proto for the first time.

The point here is that you should really provide the only service.proto files for the GRPC loader, not the rest of the files, and compiler will do the rest of the job, but do you actually need to know such little things while you just can have very valid root file. Sad thing that Protoc cannot really load dependency trees, and I can name it as a flaw for the tool, while protobuf.js doing a great job actually :)

I hope we shouldn't get into the discussion of why package roots should be the same for the library, company, product etc. logic. com.company.product.module... ;)

[anton-alation]

https://developers.google.com/protocol-buffers/docs/proto#packages
"You can add an optional package specifier to a .proto file to prevent name clashes between protocol message types." - so, empty root namespace '' is standard namespace and must be supported.

And it's supported, but obviously, rootless proto definitions are bulky and have their own scope-naming limitations, while we all want those to be closer to real-world OOP structured objects.

Nest GRPC for empty root will be just straight definitions of Service methods without any prefixes.

[aaskandrew]

I probably will be repeating myself but we need to take into account that Nest currently matches one proto-namespace for some port. So if you have 2 namespaces like one[.orders] and two[.products] — only one of them would be loaded for a single port. And that may raise confusion when someone defines those proto for the first time.

Sounds very reasonable, but why that restriction? I think better to drop exception for this case. Cause "barrel" is not protects from it.

a.proto
package one.orders;

b.proto
package two.products;

barrel.proto
import "a.proto";
import "b.proto";

But all of I wanted is to load from different files the same namespace without "barrel".
rootns.serviceA from file A and rootns.serviceB from file B

I hope we shouldn't get into the discussion of why package roots should be the same for the library, company, product etc. logic. com.company.product.module... ;)

I am fully agree with it. In addition I can say that usually microservices sits under the proxy with routing (envoy) and common namespace actually needed.

[anton-alation]

And let me add again: I am totally for support of loading all related service files into Grpc-loader, even let it be in whatever namespace or without, but there should be certain instructions of what someone should load not less not more, to reduce confusion and increase productivity.

Also please take into account that protobuf.js allows developers to use ../ and ./ symbols while protoc doesn't support those (and it's the biggest issue so far), so it breaks multi-platforming when developer going the path he thinks the best. Instead of relative imports Protoc encourages namespaces to be used for the connectivity between different message dependencies, as like it in Java, Go or CPP.

[aaskandrew]

Yes, "use ../ and ./" prohibited by google proto docs

but there should be certain instructions of what...

I think that programmer with ability to understand namespaces must know that hi is do.

[aaskandrew]

only one of them would be loaded for a single port.

Hmm... It is true for server, but what about client?

:) this PR was stared from breaking nest binding rules.
"What is the new behavior?"

const grpcServerOptions: GrpcOptions = {
  transport: Transport.GRPC,
  options: {
    url: 'localhost:5577',
    packages: ['product', 'order'],      // <--- this line
    protoPath: join(__dirname, './proto/payment.proto'),
    loader: {
      includeDirs: [
        join(__dirname, 'proto'),
      ],
    }
  },
} 

[anton-alation]

I think that programmer with ability to understand namespaces must know that hi is do.

We cannot expect that, but we can expect deep frustration when some things would not work according to expectations. So single namespace to load is the best way so far to put the developer into some reasonable boundaries of possible expectations.

Hmm... It is true for server, but what about client?

Clients are more flexible on the selection of services they want to connect to. One expectation is that proto will match. Personally we load those for mocha just directly with grpc-loader no Nest involved :)

[aaskandrew]

But I am discussing exactly nest. Nest @GrpcMethod and ClientGrpc.

to put the developer into some reasonable boundaries of possible expectations.

With all due respect, mentoring is not the best way. In the summary, I don't want to have 'barrel' file. It isn't my paradigm. But if no way for it mean so be it.

Respectfully, I finish this discussion.

[AlexDaSoul]

I am very sorry that we could not come to an understanding. We may have to use patches or forks of the microservice package to get the features we need.

[anton-alation]

But I am discussing exactly nest. Nest @GrpcMethod and ClientGrpc.

You can just point to single proto with services you need for particular Controller and you'll be fine :) One file will load the whole dependency tree.

We may have to use patches or forks of the microservice package to get the features we need.

I think we need to just allow multiple file-load and include a bunch of tests for the situations of:

• multi-root one namespace
• multi-root no namespace
  Along with updated documentation on optimal Proto structure for Nest GRPC service, that probably only service files required to be loaded and restricting ../ and ./ imports by somehow.

As well if you loading multiple files then you need to understand how you'll be dealing with the situation of load-dir (which is usually serving as a root for namespace), and relative file-paths per each file.

[aaskandrew]

Returning to the discussion, one more use case. I have pool of grpc nest's microservices and 'envoy' proxy in front of them. Each upstream configured with 'grpc_health_check' that has only two parameters:
{ "service_name": "...", "authority": "..." }

Envoy ask about health the same port as main microcervice port, but with standard for grpc health URL 'grpc.health.v1'. So I can't to distribute packages on different ports and really need in different packages on the same port: ['appgrpcroot', 'grpc']

Envoy health_check
https://www.envoyproxy.io/docs/envoy/latest/api-v2/api/v2/core/health_check.proto.html?highlight=grpc_health_check#envoy-api-msg-core-healthcheck-grpchealthcheck

GRPC health proto
https://github.com/grpc/grpc/blob/master/src/proto/grpc/health/v1/health.proto#L20

[Jeff-Tian]

usually you start most of your interfaces with root namespace like: myservice and then you adding something like myservice.products and myservice.orders, that allows protobuf to compile all required messages from one root which allows you to have some common or shared messages to be under the root: myservice.commons as of example.

Add 1 scenario to rootless namespace practice:

Our protobuf's root namespace is actually myservice, and it goes well until when trying to add a standard health check feature to our nest js grpc service.

The standard health check proto file's package is grpc.health.v1, and only with that package name the standard grpc_health_probe in kubernetes will work.

So we might need to have 2 roots: myservice and grpc. Before this PR get merged, do we have a workaround for now?

Thanks in advance!

[Jeff-Tian]

So we might need to have 2 roots: myservice and grpc. Before this PR get merged, do we have a workaround for now?

Found the workaround for now: The ClientOptions can contain a strategy filed which can be instantiated to a server. So we can take advantage of this to replace the default nestjs grpc server with our customized grpc server which can handle the multiple root namespaces.

An example: https://github.com/Jeff-Tian/grpc-health/blob/6715ff175b0882a467cf41e6f7b9fca3a5bc7f95/src/health/grpc-client.options.ts#L23 where the ServerGrpc is the customized one and its implementation is here: https://github.com/Jeff-Tian/grpc-health/blob/master/src/extend/grpc/server.ts

The ServerGrpc implementation is basically copied from the un-merged PR with the improvements to handle multiple protoPaths options.

I created a pacakge: grpc-health that applied the above logic and improved the hero sample app to show it works: https://github.com/Jeff-Tian/nestjs-hero-grpc-sample-with-health-check, the main change to the original sample is here: Jeff-Tian/nestjs-hero-grpc-sample-with-health-check@b76249c

[kamilmysliwiec]

Partially added as part of this PR #3418

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.