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

feat(cli): add avro support #1735

Merged
merged 3 commits into from
Aug 23, 2024
Merged

feat(cli): add avro support #1735

merged 3 commits into from
Aug 23, 2024

Conversation

LAST7
Copy link
Collaborator

@LAST7 LAST7 commented Aug 19, 2024

PR Checklist

If you have any questions, you can refer to the Contributing Guide

What is the current behavior?

Support only protobuf as schema-based encoding. (cli)

Issue Number

None

What is the new behavior?

Support avro encoding now. (cli)

Does this PR introduce a breaking change?

  • Yes
  • No

Specific Instructions

Other information

I wonder what tests should be written to test the function and stability.

@LAST7 LAST7 marked this pull request as ready for review August 19, 2024 09:39
@Red-Asuka Red-Asuka added feature This pr is a feature CLI MQTTX CLI labels Aug 19, 2024
@ysfscream ysfscream added this to the v1.11.0 milestone Aug 19, 2024
cli/src/lib/pub.ts Outdated Show resolved Hide resolved
cli/src/lib/pub.ts Outdated Show resolved Hide resolved
cli/src/lib/sub.ts Outdated Show resolved Hide resolved
cli/src/lib/sub.ts Outdated Show resolved Hide resolved
cli/src/index.ts Outdated Show resolved Hide resolved
* @param {FormatType} [format] - The format to convert the message to.
* @returns {Buffer | string} - The processed message.
*/
const processPublishMessage = (
message: string | Buffer,
protobufPath?: string,
protobufMessageName?: string,
schemaOptions: SchemaOptions,
format?: FormatType,
): Buffer | string => {
Copy link
Member

@Red-Asuka Red-Asuka Aug 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that schemaOptions should be an optional parameter, and that schemaOptions and format should be combined into a single options object.

The code could be refactored like this:

const processReceivedMessage = (
  payload: Buffer,
  options?: {
    format?: "base64" | "json" | "hex" | "cbor" | "binary"
    schema?: 'protobuf' | 'avro'
    protobufPath?: string
    protobufMessageName?: string
    avscPath?: string
  },
): string | Buffer => {

The rationale is that these parameters are all related to processing the payload and are optional. By merging them into a single options object, we eliminate concerns about argument order and make the code more intuitive and easier to maintain.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your effort on reviewing my code. But I have a different opinion on the design of this parameter.

  1. format and schemaOptions serve different purposes within the processing pipeline.(They are used in different functions)
  2. merging them into one parameter would bypass the restriction set in type SchemaOptions, which I believe is kind of unsafe.

But I do understand your suggestion on simplifying the signature of this function, maybe we can come to a middle ground which retains safety while simplifies the signature.

add JSON validation
fix typo & doc comment
@ysfscream ysfscream self-requested a review August 23, 2024 02:53
cli/src/utils/parse.ts Outdated Show resolved Hide resolved
@ysfscream
Copy link
Member

@LAST7 Thanks for your contribution and everything LGTM.

@Red-Asuka Red-Asuka merged commit 1665514 into emqx:main Aug 23, 2024
4 checks passed
@Red-Asuka
Copy link
Member

@LAST7 Thanks for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLI MQTTX CLI feature This pr is a feature
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants