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 RSocketRequester API for requests without payload #23649

Closed
sdeleuze opened this issue Sep 17, 2019 · 2 comments
Closed

Fix RSocketRequester API for requests without payload #23649

sdeleuze opened this issue Sep 17, 2019 · 2 comments
Assignees
Labels
in: messaging Issues in messaging modules (jms, messaging) type: enhancement A general enhancement
Milestone

Comments

@sdeleuze
Copy link
Contributor

Current RSocketRequester makes it mandatory to use RequestSpec#data methods in order to reach ResponseSpec methods that allow to perform the exchange. This RequestSpec / ResponseSpec split seems unnecessary since all ResponseSpec methods can be invoked directly for request without payload, which seems to be a perfectly valid use case.

This leads to code like req.route("find.radar.{iata}", iata).data(Mono.empty()).retrieveMono(AirportLocation.class)), and the issue is even more impacting in Coroutines world since data() does not accept null.

After discussion with @bclozel, our proposal is to merge RequestSpec and ResponseSpec in order to avoid this issue. Another hint that they should probably be merged is that ResponseSpec methods have side effects on the request itself.

About the name, maybe we could just use RequestSpec, move ResponseSpec methods in it and remove ResponseSpec.

@sdeleuze sdeleuze added in: messaging Issues in messaging modules (jms, messaging) type: enhancement A general enhancement labels Sep 17, 2019
@sdeleuze sdeleuze added this to the 5.2 GA milestone Sep 17, 2019
@sdeleuze sdeleuze self-assigned this Sep 17, 2019
@bclozel
Copy link
Member

bclozel commented Sep 17, 2019

I think this change aligns nicely with our infrastructure and RSocket in general, because:

  1. unlike its counterpart in WebClient, here the ResponseSpec is not only about how to consume the response, but also about the type of request and RSocket communication pattern we'll use. ResponseSpec has side-effects on RequestSpec.
  2. as @sdeleuze said, none of the RequestSpec methods are mandatory.

The alternative would be to have a noData() in the RequestSpec, but its only goal would be to express the absence of a data() call.

sdeleuze added a commit to sdeleuze/spring-framework that referenced this issue Sep 18, 2019
This commit makes it possible to send requests without
without requiring to call data(Mono.empty).

It clarifies the semantics by renaming ResponseSpec
to InteractionSpec in order to leverage more directly RSocket
vocabulary and make things more consistent now that RequestSpec
extends it.

It also improves the Consumer variant of metadata in order to prevent
calling data in the callback (only metadata is now accessible).

Closes spring-projectsgh-23649
sdeleuze added a commit to sdeleuze/spring-framework that referenced this issue Sep 18, 2019
This commit makes it possible to send requests without
without requiring to call data(Mono.empty).

It clarifies the semantics by renaming ResponseSpec
to InteractionSpec in order to leverage more directly RSocket
vocabulary and make things more consistent now that RequestSpec
extends it.

It also improves the Consumer variant of metadata in order to prevent
calling data in the callback (only metadata is now accessible).

Closes spring-projectsgh-23649
sdeleuze added a commit to sdeleuze/spring-framework that referenced this issue Sep 18, 2019
This commit makes it possible to send requests without
without requiring to call data(Mono.empty()).

It clarifies the semantics by renaming ResponseSpec
to InteractionSpec in order to leverage more directly RSocket
vocabulary and make things more consistent now that RequestSpec
extends it.

It also improves the Consumer variant of metadata in order to prevent
calling data in the callback (only metadata is now accessible).

Closes spring-projectsgh-23649
@sdeleuze
Copy link
Contributor Author

sdeleuze commented Sep 18, 2019

I have worked on that, and here is my proposal: sdeleuze@rsocketrequester-refactoring.

This commit makes it possible to send requests without requiring to call data(Mono.empty()).

It clarifies the semantics by renaming ResponseSpec to InteractionSpec in order to leverage more directly RSocket vocabulary and make things more consistent now that RequestSpec
extends it.

It also improves the Consumer variant of metadata in order to prevent calling data in the callback (only metadata is now accessible).

Any thoughts @bclozel @rstoyanchev?

sdeleuze added a commit to sdeleuze/spring-framework that referenced this issue Sep 18, 2019
This commit makes it possible to send requests without
requiring to call data(Mono.empty()).

It clarifies the semantics by renaming ResponseSpec
to InteractionSpec in order to leverage more directly RSocket
vocabulary and make things more consistent now that RequestSpec
extends it.

It also improves the Consumer variant of metadata in order to prevent
calling data in the callback (only metadata is now accessible).

Closes spring-projectsgh-23649
sdeleuze added a commit to sdeleuze/spring-framework that referenced this issue Sep 18, 2019
This commit makes it possible to send requests without
requiring to call data(Mono.empty()). It introduces a
dedicated MetadataSpec interface and merge ResponseSpec
into RequestSpec for more flexibility.

Closes spring-projectsgh-23649
sdeleuze added a commit to sdeleuze/spring-framework that referenced this issue Sep 18, 2019
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

No branches or pull requests

2 participants