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.

Sign up for GitHub

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

Jump to bottom

Microservices: Order of pattern fields #2428

Merged

kamilmysliwiec merged 19 commits into nestjs:master from Geass1000:pattern-to-route

Jun 28, 2019

Contributor

Geass1000 commented Jun 21, 2019

PR Checklist

Please check if your PR fulfills the following requirements:

[ x ] The commit message follows our guidelines: https://github.com/nestjs/nest/blob/master/CONTRIBUTING.md
[ x ] 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?

[ x ] 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?

When we send/emit request according to some object pattern, pattern doesn’t sort pattern’s fields . Pattern:

{ controller: `main`, service: `users`, use: `addUser` }

doesn’t equal:

{ service: `users`, controller: `main`, use: `addUser` }

Thus, if we try to catch a pattern using MessagePattern or EventPattern, nest will not call a required handler.

What is the new behavior?

I added simple middle-logic, that parses a patterns, sorts fields and creates a new pattern (route).

Does this PR introduce a breaking change?

[ ] Yes
[ x ] No

Other information

Geass1000 added 13 commits

June 20, 2019 18:16


          feature(microservices): add transformPatternToRoute utility

216645f


          feature(microservices): add index reexport file

df9f6ec


          feature(microservices): add logic to transform patterns to routes

43b85aa


          feature(microservices): use recursion for nested pattern object

9e45797


          feature(microservices): use braces to divide route by scope

1bebe98


          test(microservices): add tests for transformPatternToRoute utility …

58214a1

…method


          test(microservices): regroup test suites


          feature(microservices): use MsvcUtil as a class property

0bc3684


          feature(microservices): use MsvcUtil in addHandler instead of `ge…

16ee6cc

…tRouteFromPattern`


          test(microservices): add tests for new pattern-route logic


          feature(microservices): use MsvcUtil as a class property

0fa72b6


          test(microservices): add tests for new pattern-route logic

4349b14


          style(microservices): change indentation to space (2)

5ddd8cb

coveralls commented Jun 21, 2019 •

edited

Loading

Pull Request Test Coverage Report for Build 3326

26 of 26 (100.0%) changed or added relevant lines in 4 files are covered.
4 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.007%) to 94.961%

Files with Coverage Reduction	New Missed Lines	%
packages/microservices/server/server.ts	1	93.1%
packages/microservices/client/client-grpc.ts	3	94.17%

Totals
Change from base Build 3263:	0.007%
Covered Lines:	3486
Relevant Lines:	3671

💛 - Coveralls

kamilmysliwiec reviewed

View reviewed changes

packages/microservices/client/client-proxy.ts

@@ @@ -18,11 +18,14 @@ import { @@
                 WritePacket,
               } from '../interfaces';
+              import * as Utils from '../utils';

Member

kamilmysliwiec Jun 23, 2019

Why did you use import * as instead of importing a particular class?

Contributor Author

Geass1000 Jun 24, 2019

I don't see any reason why I can't use it :) It provides an isolated scope (utils/interfaces/enums etc) and allows me to make import section more simple.

kamilmysliwiec reviewed

View reviewed changes

packages/microservices/client/client-proxy.ts

               export abstract class ClientProxy {
                 public abstract connect(): Promise<any>;
                 public abstract close(): any;
                 protected routingMap = new Map<string, Function>();
+                protected readonly msvcUtil = Utils.MsvcUtil;

Member

kamilmysliwiec Jun 23, 2019

The variable name isn't very descriptive (+using msvc seems to be redundant here)

Contributor Author

Geass1000 Jun 24, 2019

In my opinion, if other programmer sees this variable it will understand what is it. What name do you suggest?

kamilmysliwiec reviewed

View reviewed changes

packages/microservices/server/server.ts

@@ @@ -18,27 +18,31 @@ import { @@
               } from '../interfaces';
               import { NO_EVENT_HANDLER } from './../constants';
+              import * as Utils from '../utils';

Member

kamilmysliwiec Jun 23, 2019

Same as above

Contributor Author

Geass1000 Jun 24, 2019

I don't see any reason why I can't use it :) It provides an isolated scope (utils/interfaces/enums etc) and allows me to make import section more simple.

kamilmysliwiec reviewed

View reviewed changes

packages/microservices/server/server.ts

               export abstract class Server {
                 protected readonly messageHandlers = new Map<string, MessageHandler>();
                 protected readonly logger = new Logger(Server.name);
+                protected readonly msvcUtil = Utils.MsvcUtil;

Member

kamilmysliwiec Jun 23, 2019

Same as above

Contributor Author

Geass1000 Jun 24, 2019

In my opinion, if other programmer sees this variable it will understand what is it. What name do you suggest?

kamilmysliwiec reviewed

View reviewed changes

packages/microservices/server/server.ts Outdated

+                 * @returns string
+                 */
+                private getRouteFromPattern(pattern: string): string {
+                  let validPattern: any;

Member

kamilmysliwiec Jun 23, 2019

any -> Record<string, unknown> | string

Contributor Author

Geass1000 Jun 24, 2019

Changed.

kamilmysliwiec reviewed

View reviewed changes

packages/microservices/server/server.ts

+                  let validPattern: any;
+                  try {
+                    // Gets the pattern in JSON format

Member

kamilmysliwiec Jun 23, 2019

This comment seems to be needless

Contributor Author

Geass1000 Jun 24, 2019

When I write code, I adhere to PPP (Pseudocode Programming Process). If you want, I will remove this comment.

kamilmysliwiec reviewed

View reviewed changes

packages/microservices/server/server.ts

+                    validPattern = pattern;
+                  }
+                  // Transform the Pattern to Route

Member

kamilmysliwiec Jun 23, 2019

This comment seems to be needless

Contributor Author

Geass1000 Jun 24, 2019

When I write code, I adhere to PPP (Pseudocode Programming Process). If you want, I will remove this comment.

kamilmysliwiec reviewed

View reviewed changes

packages/microservices/utils/msvc.util.ts

		@@ -0,0 +1,39 @@
		import { isString, isObject } from '@nestjs/common/utils/shared.utils';

		export class MsvcUtil {

Member

kamilmysliwiec Jun 23, 2019

The filename + class name aren't very descriptive (+using msvc prefix seems to be redundant here)

Contributor Author

Geass1000 Jun 24, 2019

What name do you suggest?

kamilmysliwiec reviewed

View reviewed changes

packages/microservices/utils/msvc.util.ts Outdated

+                 * @param  {any} pattern - client pattern
+                 * @returns string
+                 */
+                public static transformPatternToRoute(pattern: any): string {

Member

kamilmysliwiec Jun 23, 2019 •

edited

Loading

Why static? Why pattern: any?

Contributor Author

Geass1000 Jun 24, 2019

I use classes for utils and static method allows me to use utils as singleton. Changed.

kamilmysliwiec reviewed

View reviewed changes

packages/microservices/utils/msvc.util.ts Outdated

+                 * @returns string
+                 */
+                public static transformPatternToRoute(pattern: any): string {
+                  // Returns the pattern according to the 1st

Member

kamilmysliwiec Jun 23, 2019

This comment seems to be redundant (it's easy to infer from the context)

Contributor Author

Geass1000 Jun 24, 2019

When I write code, I adhere to PPP (Pseudocode Programming Process). If you want, I will remove this comment.

kamilmysliwiec reviewed

View reviewed changes

packages/microservices/utils/msvc.util.ts

+                    return pattern;
+                  }
+                  // Throws the error if the pattern has an incorrect type

Member

kamilmysliwiec Jun 23, 2019

This comment seems to be redundant (it's easy to infer from the context)

Contributor Author

Geass1000 Jun 24, 2019

When I write code, I adhere to PPP (Pseudocode Programming Process). If you want, I will remove this comment.

kamilmysliwiec reviewed

View reviewed changes

packages/microservices/utils/msvc.util.ts

+                  const sortedPatternParams = sortedKeys.map((key) =>
+                    `${key}:${MsvcUtil.transformPatternToRoute(pattern[key])}`);
+                  // Creates and returns the Route

Member

kamilmysliwiec Jun 23, 2019

This comment seems to be redundant (it's easy to infer from the context)

Contributor Author

Geass1000 Jun 24, 2019

When I write code, I adhere to PPP (Pseudocode Programming Process). If you want, I will remove this comment.

Member

kamilmysliwiec commented Jun 23, 2019

Great idea! Thanks for contribution :)

I left a few comments, could you look at them?

kamilmysliwiec added scope: microservices status: wip ⚡️ type: enhancement 🐺 labels

Geass1000 added 6 commits

June 24, 2019 10:44


          feature(microservices): add interface for pattern

49bda5c


          refactor(microservices): use MsPattern interface instead of any

ff6c46e


          feature(microservices): add handling of fields with number type

2faca66


          test(microservices): rework tests for number values

ed18eac


          refactor(microservices): use equalTest function instead of repeated…

5763e2f

… tests


          refactor(microservices): use MsPattern interface instead of any type

52caa02

Member

kamilmysliwiec commented Jun 28, 2019

I have nothing against your coding style, just want to ensure that newly added code is consistent with the project. I fixed gRPC issues (broken integration tests) + adjusted code style a little bit.

Thanks for your contribution! I'll release it soon

kamilmysliwiec merged commit 52caa02 into nestjs:master

JonathanMeyer2600 commented Jul 10, 2019

Hello @kamilmysliwiec,

we have around 15 microservices in production. Upgrading to version > 6.4.0 changes the command name because of this feature. Before this the command name was {"cmd":"someCommand"}, now it is {cmd:someCommand}. This is making communication between services < 6.4.0 and >= 6.4.0 incompatible. We can't and don't want to update all services at once but one by one. This change is making it impossible. Any idea? Can we introduce the quotes again?

Thanks in advance,
Jonathan

Member

kamilmysliwiec commented Jul 10, 2019

Hi @JonathanMeyer2600,
Apologize for the breaking change! The issue has been already fixed in the 6.5.2 release. Could you let me know if it works for you now?

JonathanMeyer2600 commented Jul 10, 2019

Hi @kamilmysliwiec
it's working! Thanks for the quick fix!

Contributor Author

Geass1000 commented Jul 10, 2019 •

edited

Loading

Hello @kamilmysliwiec

When I was writing this code, I didn’t think about messages from other sources. I didn’t use JSON.stringify to encode the message and it made pattern (route) a bit smaller. But now code does the same thing as JSON.stringify and I see no reason not to use it. If you want, you can use something like that:

  const sortedPattern  = {};
  sortedKeys.map(key => {
    sortedPattern[key] = transformPatternToRoute(pattern[key]);
  });

  return JSON.stringify(sortedPattern);

I apologize about any inconvenience. :(

Member

kamilmysliwiec commented Jul 10, 2019

No worries @Geass1000! :) I'll look at this soon, just wanted to fix the issue as quick as possible

kamilmysliwiec mentioned this pull request

Cannot connect dockerised microservices after v.6.0.5 #2532

Closed

lock bot commented Oct 8, 2019

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.

lock bot locked as resolved and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

scope: microservices status: wip ⚡️ type: enhancement 🐺