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

Provide an RSocketMessageHandlerCustomizer to allow customizing of the RSocketMessageHandler #20303

Closed
4 tasks done
joshlong opened this issue Feb 24, 2020 · 17 comments
Closed
4 tasks done
Assignees
Labels
status: superseded An issue that has been superseded by another type: enhancement A general enhancement

Comments

@joshlong
Copy link
Member

joshlong commented Feb 24, 2020

Hi, this is a first-timers-only issue. This means we've worked to make it more legible to folks who either haven't contributed to our codebase before, or even folks who haven't contributed to open source before.

If that's you, we're interested in helping you take the first step and can answer questions and help you out as you do. Note that we're especially interested in contributions from people from groups underrepresented in free and open source software!

If you have contributed before, consider leaving this one for someone new, and looking through our general ideal-for-contribution issues. Thanks!

Background

Spring Boot provides a number of callback interfaces that can be used to customize the beans that it auto-configures. For example, a TomcatConnectorCustomizer can be used to customize a Tomcat Connector.

Problem

At the moment, the only way to add customizations to a RSocketMessageHandler is to provide your own RSocketMessageHandler bean which completely overrides the auto-configured RSocketMessageHandler bean. This can be bit tedious and we should allow providing customizers instead which can be applied to the bean created by Spring Boot.

It would look something like:

@Bean
public RSocketMessageHandlerCustomizer messageHandlerCustomizer() {
	return (handler) -> handler.setRouteMatcher(...);
}

Solution

Add a RSocketMessageHandlerCustomizer type. Beans of RSocketMessageHandlerCustomizer type should be applied automatically to RSocketMessageHandler, this can be done here.

The ObjectProvider interface can be used for injecting a dependency. Here is an example that can be used to inject customizers when there can be 0..n of them.

Tests are here.

Steps to Fix

  • Claim this issue with a comment below and ask any clarifying questions you need
  • Set up a repository locally following the Contributing Guidelines
  • Try to fix the issue following the steps above
  • Commit your changes and start a pull request.
@joshlong
Copy link
Member Author

This is following up on #18356 (comment)

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Feb 24, 2020
@bclozel bclozel changed the title Provide an RSocketMessageHandlerChdtomizer to allow customizing of the RSocketMessageHandler, please Provide an RSocketMessageHandlerCustomizer to allow customizing of the RSocketMessageHandler Feb 24, 2020
@bclozel bclozel added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Feb 24, 2020
@bclozel bclozel added this to the 2.3.x milestone Feb 24, 2020
@aartiguptaa
Copy link
Contributor

I can pick this up !

@snicoll
Copy link
Member

snicoll commented Mar 26, 2020

@aartiguptaa thank you very much, it's all yours. Let us know if you have any question.

@philwebb philwebb added the status: first-timers-only An issue that can only be worked on by brand new contributors label Apr 2, 2020
@cicioflaviu
Copy link
Contributor

If something comes up and @aartiguptaa will not be able to complete the task, I'm available to take over.

@mbhave
Copy link
Contributor

mbhave commented Apr 6, 2020

@aartiguptaa how's it going? Please let us know if you need any help tackling this issue.

@logicatmidod
Copy link

Hi

looks like this one's gone. please let me know if there is any first timer issue which I can contribute to?

Thanks

@snicoll
Copy link
Member

snicoll commented Apr 11, 2020

@logicatmidod we can't let you know when there are a first timer issue available. If you're trying to contribute, I've answered a similar question this morning, please read this comment. We prefer no to use the issue tracker for questions so for any follow-up questions on the project, please join the community on Gitter.

@aartiguptaa
Copy link
Contributor

Apologies for the delay folks, I am working on this now, and should have more updates this weekend.

@corneliouzbett
Copy link

@joshlong @snicoll could you refer me to a good first-time issue to work on. I want to understand the codebase and contribute more In future

@snicoll
Copy link
Member

snicoll commented Apr 17, 2020

@aartiguptaa How are things? Do you need help with anything?

@corneliouzbett I've answered that question already here but it was hidden so I just changed that.

@aartiguptaa
Copy link
Contributor

@snicoll , @mbhave , which dependency should the Object Provider interface be used to inject?

@snicoll
Copy link
Member

snicoll commented Apr 17, 2020

I am not sure I got the question. ObjectProvider is part of Spring Framework. There is an example here

@aartiguptaa
Copy link
Contributor

aartiguptaa commented Apr 17, 2020

The instructions say "The ObjectProvider interface can be used for injecting a dependency. Here is an example that can be used to inject customizers when there can be 0..n of them."

which dependency should ObjectProvider inject ?

Is it the message handler bean ? or a customizer bean?

@Bean
@ConditionalOnMissingBean(RSocketMessageHandler.class)
public <WhatType?>  RSocketMessageHandler(ObjectProvider<RSocketMessageHandlerCustomizer> customizers) {
		(what should this inject?)
}

@scottfrederick
Copy link
Contributor

@aartiguptaa The existing method that looks like this:

	@Bean
	@ConditionalOnMissingBean
	public RSocketMessageHandler messageHandler(RSocketStrategies rSocketStrategies) {
		RSocketMessageHandler messageHandler = new RSocketMessageHandler();
		messageHandler.setRSocketStrategies(rSocketStrategies);
		return messageHandler;
	}

would be changed to take the new optional customizer dependency using ObjectProvider, looking something like this:

	public RSocketMessageHandler messageHandler(RSocketStrategies rSocketStrategies, ObjectProvider<RSocketMessageHandlerCustomizer> customizers) {
            ...
	}

Inside of that method, each RSocketMessageHandlerCustomizer can be applied to the messageHandler object as in this example.

@aartiguptaa
Copy link
Contributor

aartiguptaa commented Apr 20, 2020

Trying to push a PR, on a new branch, do I need special permissions to add a new branch?
git push --set-upstream origin RSocketMessageHandlerCustomizer
Username for 'https://github.com': aartiguptaa
Password for 'https://aartiguptaa@github.com':
remote: Permission to spring-projects/spring-boot.git denied to aartiguptaa.
fatal: unable to access 'https://github.com/spring-projects/spring-boot.git/': The requested URL returned error: 403

@scottfrederick
Copy link
Contributor

scottfrederick commented Apr 20, 2020

@aartiguptaa You'll need to create your own fork of the spring-boot repository and push your changes there.

After creating a fork, you should be able to

$ git remote origin set-url https://github.com/aartiguptaa/spring-boot

to point the origin remote to your fork instead of spring-projects/spring-boot. Then you can

$ git push --set-upstream origin RSocketMessageHandlerCustomizer

to push your new branch to your own fork.

Once the branch is pushed to your fork, see the github docs for creating a pull request.

@wilkinsona
Copy link
Member

Closing in favour of #21081.

@wilkinsona wilkinsona added status: superseded An issue that has been superseded by another and removed status: first-timers-only An issue that can only be worked on by brand new contributors labels Apr 29, 2020
@wilkinsona wilkinsona removed this from the 2.3.x milestone Apr 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: superseded An issue that has been superseded by another type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests