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

fix(microservices): allow falsy values in options #10549

Merged

Conversation

Papooch
Copy link
Contributor

@Papooch Papooch commented Nov 9, 2022

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • 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?

Falsy values passed to ClientsModule options would be overwritten by defaults.
Noticeably, the postfixId passesd to ClientKafka.

This happens because of faulty logic in the getOptionsProp method on ClientProxy. I have changed it to align with the behavior of the corresponding method in Server.

What is the new behavior?

When passing empty string to postfixId to options of ClientKafka, the -client postfix no longer gets appended. This probably fixes a bunch of other defaults as well.

Does this PR introduce a breaking change?

  • Yes
  • No, unless someone relied on the wrong behavior.

Other information

I have also uncommented the skipped Kafka integration tests, but they got stuck on the error below (after the annoying rebalancing errors) and I had to cancel them.
image
I did not investigate further, because I think this is for another issue, but it doesn't feel right skipping all Kafka integration tests.

@coveralls
Copy link

Pull Request Test Coverage Report for Build e19c1717-da0c-4c59-8080-910576c30eaf

  • 5 of 5 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 93.404%

Totals Coverage Status
Change from base Build c2dbdd8a-105c-4233-97cc-e76da419e49f: 0.0%
Covered Lines: 6202
Relevant Lines: 6640

💛 - Coveralls

@kamilmysliwiec
Copy link
Member

This is partially a duplicate of #10448 (this is a tiny, questionable but still a breaking change)

@Papooch
Copy link
Contributor Author

Papooch commented Nov 10, 2022

Would you still consider it a breaking change if it actually fixes faulty behavior? I remember this happening to us in the past when our app relied on a bug in binding middleware using a wildcard (*) and it broke after updating from 8.0.10 to 8.0.11.

Should I close this PR in favor of the other one? I can see that code in the other one is different, but the functionality is almost the same. However I think it would be better if the implementation were the same as in the Server class (also this implementation allows explicitly passing null and undefined without them being overriden).

@kamilmysliwiec
Copy link
Member

Would you still consider it a breaking change if it actually fixes faulty behavior

This usually depends on how long the faulty behavior has been the default

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants