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

Jump to bottom

Add protobuf MessageConverter #24087

Closed
wants to merge 1 commit into from
Closed

Add protobuf MessageConverter #24087

wants to merge 1 commit into from

Conversation

parviz-93
Copy link
Contributor

@parviz-93 parviz-93 commented Nov 26, 2019 •
edited by sbrannen

PR for #24022.

Should I add this converter to ...?

  • SimpAnnotationMethodMessageHandler
  • AbstractMessageBrokerConfiguration
  • MessageBrokerBeanDefinitionParser

I found that other converters are used in these classes.

@parviz-93 parviz-93 mentioned this pull request Nov 26, 2019
Closed
@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 requested changes Nov 27, 2019
View reviewed changes
Copy link
Contributor

@rstoyanchev rstoyanchev left a comment

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.

...essaging/src/main/java/org/springframework/messaging/converter/ProtobufMessageConverter.java Show resolved Hide resolved
...essaging/src/main/java/org/springframework/messaging/converter/ProtobufMessageConverter.java Show resolved Hide resolved
...essaging/src/main/java/org/springframework/messaging/converter/ProtobufMessageConverter.java Show resolved Hide resolved
...essaging/src/main/java/org/springframework/messaging/converter/ProtobufMessageConverter.java Show resolved Hide resolved
...ging/src/test/java/org/springframework/messaging/converter/ProtobufMessageConverterTest.java Show resolved Hide resolved
@parviz-93
Copy link
Contributor Author

parviz-93 commented Nov 27, 2019 •
edited

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

rstoyanchev
rstoyanchev reviewed Nov 27, 2019
View reviewed changes
...essaging/src/main/java/org/springframework/messaging/converter/ProtobufMessageConverter.java Outdated
methodCache.put(clazz, method);
}
return (Message.Builder) method.invoke(clazz);
} catch (Exception ex) {
Copy link
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
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
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
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
Copy link
Contributor

rstoyanchev commented Nov 28, 2019 •
edited

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
Polishing contribution
25f3465 
See gh-24087
@rstoyanchev
Copy link
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
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

This was quick. Thanks a lot

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

@rstoyanchev rstoyanchev rstoyanchev requested changes

Assignees

@rstoyanchev rstoyanchev

Labels
in: messaging Issues in messaging modules (jms, messaging) type: enhancement A general enhancement
Projects
None yet
Milestone
5.2.2
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@parviz-93 @rstoyanchev @onlybytes @spring-projects-issues