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

[WFLY-19618] Helloworld REST Quickstart #953

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

theashiot
Copy link

@emmartins emmartins marked this pull request as ready for review September 18, 2024 09:07
@emmartins emmartins self-requested a review as a code owner September 18, 2024 09:07
@emmartins emmartins added the hold do not merge label Sep 18, 2024
Copy link
Contributor

@emmartins emmartins left a comment

Choose a reason for hiding this comment

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

Let's change a bit the Quickstart, to become the helloworld with only one change, we replace the servlet class with the classes for the REST app and the REST endpoint, in short considering this PR we do the following changes:

  • remove the cdi bean HelloService
  • we sync the test class with helloworld quickstart (you may improve both, but we should have both in sync)
  • we change the HelloWorld REST endpoint to interface with the user exactly as helloworld:
/*
 * Copyright The WildFly Authors
 * SPDX-License-Identifier: Apache-2.0
 */
package org.jboss.as.quickstarts.rshelloworld;

import jakarta.inject.Inject;
import jakarta.ws.rs.GET;
import jakarta.ws.rs.Path;
import jakarta.ws.rs.Produces;

/**
 * A simple REST service which is able to say "Hello World!"" at /HelloWorld
 *
 */
@Path("/")
public class HelloWorld {
    @GET
    @Path("/HelloWorld")
	@Produces(MediaType.TEXT_PLAIN)
    public String getHelloWorldJSON() {
        return "<h1>Hello World!</h1>";
    }
} 
  • update the README-source according to changes above

@theashiot theashiot force-pushed the WFLY-19618 branch 2 times, most recently from 75d903a to 927f699 Compare September 24, 2024 11:20
@theashiot
Copy link
Author

Thanks, @emmartins for the review! I've made the required changes.

Do you want me to create a separate WFLY issue to update the HelloWorld quickstart test? I think it'd be good to test whether the response contains "Hello World!", WDYT?

best,
ashwin

Copy link
Member

@jamezp jamezp left a comment

Choose a reason for hiding this comment

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

There should probably be a CI workflow for this as well.

helloworld-rs/pom.xml Outdated Show resolved Hide resolved
Comment on lines 83 to 93
<pluginRepository>
<id>redhat-ga-maven-repository</id>
<name>Red Hat GA Maven Repository</name>
<url>https://maven.repository.redhat.com/ga/</url>
<releases>
<enabled>true</enabled>
</releases>
<snapshots>
<enabled>true</enabled>
</snapshots>
</pluginRepository>
Copy link
Member

Choose a reason for hiding this comment

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

There should be no need for this repository.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Contributor

Choose a reason for hiding this comment

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

This change should extend to the guide itself, by default users should only add the jboss.org Repo, but we should give optional instructions wrt adding also GA repo, since some apps may require it (e.g. microprofile-jwt quickstart).

Copy link
Author

Choose a reason for hiding this comment

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

removed

helloworld-rs/pom.xml Outdated Show resolved Hide resolved
Comment on lines 39 to 67
@Test
public void testHTTPEndpointIsAvailable() throws IOException, InterruptedException, URISyntaxException {
String serverHost = this.getServerHost();
final HttpRequest request = HttpRequest.newBuilder()
.uri(new URI(serverHost))
.GET()
.build();
final HttpClient client = HttpClient.newBuilder()
.followRedirects(HttpClient.Redirect.ALWAYS)
.connectTimeout(Duration.ofMinutes(1))
.build();
final HttpResponse<String> response = client.send(request, HttpResponse.BodyHandlers.ofString());
assertEquals(200, response.statusCode());
}

@Test
public void testMessage() throws IOException, InterruptedException, URISyntaxException {
String serverHost = this.getServerHost();
final HttpRequest request = HttpRequest.newBuilder()
.uri(new URI(serverHost+"/rest/HelloWorld"))
.GET()
.build();
final HttpClient client = HttpClient.newBuilder()
.followRedirects(HttpClient.Redirect.ALWAYS)
.connectTimeout(Duration.ofMinutes(1))
.build();
final HttpResponse<String> response = client.send(request, HttpResponse.BodyHandlers.ofString());
assertTrue(response.body().contains("Hello World!"));
}
Copy link
Member

Choose a reason for hiding this comment

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

It seems a little strange to me to have two different tests here. Both should likely be asserting on the status code.

Also, is there a reason not to use the REST Client given this is a REST quickstart?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Author

Choose a reason for hiding this comment

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

@GET
@Path("/HelloWorld")
@Produces(MediaType.TEXT_PLAIN)
public String getHelloWorldJSON() {
Copy link
Member

Choose a reason for hiding this comment

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

The method name should be changed as this isn't a JSON response.

Also should we add a @PathParam("name") and great with the name instead of world? e.g.

@GET
@Path("/hello/{name}")
@Produces(MediaType.TEXT_PLAIN)
public String hello(@PathParam("name") final String name) {
    return String.format("Hello %s!", name);
}

Not needed, but just shows a simple path parameter too.

Copy link
Contributor

@emmartins emmartins Oct 3, 2024

Choose a reason for hiding this comment

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

I agree with renaming the method, but not with introducing the parameter since this would complicate the interaction (should be the simplest as possible) and documentation

Copy link
Author

Choose a reason for hiding this comment

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

updated the method name.

@theashiot
Copy link
Author

Thanks, @jamezp for the review! I've started implementing the changes. I'll let you know when the PR is ready for review again.

cheers,
ashwin

@theashiot theashiot force-pushed the WFLY-19618 branch 2 times, most recently from 4b38e52 to 3d8268f Compare October 7, 2024 11:46
@theashiot
Copy link
Author

Thanks a lot @jamezp and @emmartins for the review! I've made the changes. Can you please have another look?

best,
ashwin

helloworld-rs/pom.xml Outdated Show resolved Hide resolved
helloworld-rs/pom.xml Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hold do not merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants