Skip to content

Add protobuf MessageConverter#24087

Closed
parviz-93 wants to merge 1 commit into
spring-projects:masterfrom
parviz-93:#24022-protobuf
Closed

Add protobuf MessageConverter#24087
parviz-93 wants to merge 1 commit into
spring-projects:masterfrom
parviz-93:#24022-protobuf

Conversation

@parviz-93

@parviz-93 parviz-93 commented Nov 26, 2019

Copy link
Copy Markdown
Contributor

PR for #24022.

Should I add this converter to ...?

  • SimpAnnotationMethodMessageHandler
  • AbstractMessageBrokerConfiguration
  • MessageBrokerBeanDefinitionParser

I found that other converters are used in these classes.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Nov 26, 2019
@rstoyanchev rstoyanchev added in: messaging Issues in messaging modules (jms, messaging) type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Nov 27, 2019
@rstoyanchev rstoyanchev added this to the 5.2.2 milestone Nov 27, 2019
@rstoyanchev rstoyanchev self-assigned this Nov 27, 2019

@rstoyanchev rstoyanchev left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Protobuf 2 is more than 5 years old, so for this converter being added I think we should focus on Protobuf 3+ along with the official protobuf-java-util. In other words eliminate anything related to protobuf-java-format.

/cc @sdeleuze @jhoeller for comment.

@parviz-93

parviz-93 commented Nov 27, 2019

Copy link
Copy Markdown
Contributor Author

if we want eliminate anything related to protobuf-java-forma and support official protobuf-java-util, do we need ProtobufFormatSupport interface ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Correct the braces throughout, please :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sorry. it is my first contribution to spring. thanks for correcting me

@rstoyanchev

Copy link
Copy Markdown
Contributor

Indeed we could simplify a bit by removing ProtobufFormatSupport, but I would still keep ProtobufJavaUtilSupport as is so that it's easy to compare to ProtobufHttpMessageConverter which we have to maintain in parallel.

@parviz-93

Copy link
Copy Markdown
Contributor Author

I’m doing new commits so that you can see what I fixed, at the end I’ll collect in one commit.

@rstoyanchev

rstoyanchev commented Nov 28, 2019

Copy link
Copy Markdown
Contributor

This looks much better. Feel free to squash the commits, I'm only looking for the end result.

In terms of your other question about where to add the converter to, I'd say in AbstractMessageBrokerConfiguration and MessageBrokerBeanDefinitionParser (i.e. the Java and XML config).

@sbrannen sbrannen changed the title #24022 - added protobuf MessageConverter Add protobuf MessageConverter Nov 29, 2019
rstoyanchev added a commit that referenced this pull request Nov 29, 2019
@rstoyanchev

Copy link
Copy Markdown
Contributor

@parviz-93 thanks so much for your contribution and following through with the changes! This is now merged in. I ended up making a few further changes, adding a sub-class of the protobuf converter (just like in spring-web) because if it all came in one converter, the JSON format support wouldn't be optional. As a consequence I also decided to pull out the automatic registration in Java and XML config.

@parviz-93

Copy link
Copy Markdown
Contributor Author

Thank you very much for your help and advice.

@parviz-93 parviz-93 deleted the #24022-protobuf branch November 30, 2019 05:31
@onlybytes

Copy link
Copy Markdown

This was quick. Thanks a lot

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

Labels

in: messaging Issues in messaging modules (jms, messaging) type: enhancement A general enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants