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

Examples: multiple ports #2834

Merged
merged 11 commits into from
Mar 15, 2021
Merged

Conversation

barchetta
Copy link
Member

@barchetta barchetta commented Mar 5, 2021

Fixes #2815

Also adds int port(String name) to ServerCdiExtension

@barchetta barchetta added this to the 2.3.0 milestone Mar 5, 2021
@barchetta barchetta self-assigned this Mar 5, 2021
@tjquinno
Copy link
Member

tjquinno commented Mar 5, 2021

I came at this by considering how a user might convert our classic MP greeting app to use multiple routing names.

First choice: add @RoutingName to the PUT method for changing the greeting

Ideally, there would be the obvious change to config and then one line of change in the code: add @RoutingName("private") to the PUT method on the existing resource. The public port would be available to outside requests but not the private one so only internal users could change the greeting.

This will not even compile because the @RoutingName annotation is defined to apply only to types. Ideally, it would work on methods as well.

Second choice: add @RoutingName to the resource classes

The next best would be to create two resources, one for the public endpoints with @Path("/greet") and the second annotated with @RoutingName("private") @Path("/greet/greeting") for the private ones. That builds but does not work as desired; the app endpoints (/greet and /greet/greeting) seem available on both the default routing and the private one so it looks as if the @RoutingName on resources is ignored.

Last choice: split the existing resource and create two Application classes (as you've had to do)

So, are we really telling developers that to use this feature they need take the quick-start example (or their own app), create two Application classes so they can specify the @RoutingName("private") on one and then write their respective getClasses methods, and also divide the resource class into two different resource classes so each application can report its own resource?

Ugh.

Even with the first and second choices not possible, it would be good in our example to at least preserve the external API of the greeting app. I tried binding the "public" resource to the path /greet and the "private" one to /greet/greeting and that seems to work. Trying to PUT to the public port returns a 405 Method Not Allowed which makes sense.

@barchetta
Copy link
Member Author

@tjquinno I experimented with those first two options as well with the same results.

@tjquinno
Copy link
Member

tjquinno commented Mar 5, 2021

@barchetta It might be worth seeing if the @RoutingName processing could be changed so one of the first two approaches would work.

Copy link
Member

@tjquinno tjquinno left a comment

Choose a reason for hiding this comment

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

Maybe add some additional negative-expected probes? Otherwise LGTM.

@barchetta barchetta requested a review from tjquinno March 9, 2021 20:13
Copy link
Member

@tomas-langer tomas-langer left a comment

Choose a reason for hiding this comment

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

LGTM, please see the comment

@barchetta barchetta merged commit 9df5639 into helidon-io:master Mar 15, 2021
paulparkinson pushed a commit that referenced this pull request Mar 29, 2021
* Add multiple port examples for WebServer and MicroProfile
@barchetta barchetta deleted the multiport-example-2815 branch April 15, 2021 18:27
aseovic pushed a commit to aseovic/helidon that referenced this pull request Apr 26, 2021
* Add multiple port examples for WebServer and MicroProfile
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Examples: multiple ports
4 participants