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

Revision of jetty dependencies and maven profiles to preserve runnability as webapp, spring-boot, and testing #624

Merged
merged 14 commits into from
Jan 25, 2024

Conversation

stmsat
Copy link
Contributor

@stmsat stmsat commented Dec 21, 2023

This PR (NOTE: for branch ja_20231203_hapi_7_0) is to solve some issues when trying to run spring-boot with jetty and tomcat, and deploying as web application, while preserving testing.

What I had to do:

  • remove servletWebServerFactory bean that breaks things when using anything other than jetty
  • revise test dependencies of jetty because they were overriding compile/runtime dependencies so spring-boot didn't work with jetty
  • re-allow jetty dependencies of hapi-fhir-test-utilities so they can be used in test suites (without overriding compile/runtime dependencies) (this isn't true anymore, no jetty dependency is now necessary in tests)
  • revise profiles so running spring-boot with both tomcat and jetty is a possibility
  • in particular, added a profile "tomcat" for running spring-boot with tomcat (while "jetty" is for running it with jetty) (this isn't true anymore, tomcat is run by default)

…t_hapi_7_0

# Conflicts:
#	src/main/java/ca/uhn/fhir/jpa/starter/Application.java
…rting with tomcat, and running as web application doesn't work if the bean is not at least lazy)

I'm removing it, it shouldn't be necessary (to run spring-spring boot with jetty, it's enough to enable the jetty profile?)
…nability with jetty and tomcat, tests, and deployment as web application
@XcrigX
Copy link
Contributor

XcrigX commented Dec 21, 2023

This bit looks iffy:

        <!-- Use the boot profile for development and debugging options when using your IDE -->
        <profile>
            <id>boot</id>
            <activation>
                <activeByDefault>true</activeByDefault>
            </activation>
            <dependencies>
                <dependency>
                    <groupId>org.springframework.boot</groupId>
                    <artifactId>spring-boot-starter-web</artifactId>
                    <version>${spring_boot_version}</version>
                    <exclusions>
                        <exclusion>
                            <groupId>org.springframework.boot</groupId>
                            <artifactId>spring-boot-starter-tomcat</artifactId>
                        </exclusion>
                    </exclusions>
                </dependency>
            </dependencies>
        </profile>
        <!-- examples of how to start the server using the default profile-->
        <!-- mvn clean package jetty:run -->
        <!-- java -jar jetty-runner.jar target/hapi-fhir-jpaserver.war -->
        <profile>
            <id>jetty</id>
            <dependencies>
                <dependency>
                    <groupId>org.springframework.boot</groupId>
                    <artifactId>spring-boot-starter-web</artifactId>
                    <version>${spring_boot_version}</version>
                    <exclusions>
                        <exclusion>
                            <groupId>org.springframework.boot</groupId>
                            <artifactId>spring-boot-starter-tomcat</artifactId>
                        </exclusion>
                    </exclusions>
                </dependency>
                <dependency>
                    <groupId>org.springframework.boot</groupId>
                    <artifactId>spring-boot-starter-jetty</artifactId>
                    <version>${spring_boot_version}</version>
                </dependency>
            </dependencies>
        </profile><profile>
            <id>tomcat</id>
            <dependencies>
                <dependency>
                    <groupId>org.springframework.boot</groupId>
                    <artifactId>spring-boot-starter-web</artifactId>
                    <version>${spring_boot_version}</version>
                </dependency>
            </dependencies>
        </profile>

The boot profile is active by default, but excludes spring-boot-starter-tomcat.
Neither of the tomcat or jetty profiles appear to be auto-activated.
Does this force you to specify one or the other?

@XcrigX
Copy link
Contributor

XcrigX commented Dec 21, 2023

Also a side question (not necessarily for you, @stmsat) - what's the advantage of supporting Jetty at all? Why not just stick with Spring Boot's default (tomcat) and not bother with the extra complexity?

EDIT: or vice-versa - use Jetty exclusively and not Tomcat.

@stmsat
Copy link
Contributor Author

stmsat commented Dec 21, 2023

The boot profile is active by default, but excludes spring-boot-starter-tomcat. Neither of the tomcat or jetty profiles appear to be auto-activated. Does this force you to specify one or the other?

Well, to say the truth I'm not sure what purpose the "boot" profile serves (or served), because mvn spring-boot:run may not find an appropriate environment and result in an execution error; but I changed some things that maybe once made the profile useful, while causing issues elsewhere.

The profiles now are for:

  • boot: not sure
  • jetty: to run mvn -P jetty spring-boot:run and have jetty as container
  • tomcat (new addition): to run mvn -P tomcat spring-boot:run and have tomcat as container

I'm open to proposals for these profiles because I won't be using them directly, I just wanted to get rid of the servletWebServerFactory bean (which, being bound to Jetty, caused issues with other containers) while preserving functionality.

I may also have overlooked something that had sense before and now hasn't, or is wrong; my knowledge of the project is not yet very deep.

@XcrigX
Copy link
Contributor

XcrigX commented Dec 21, 2023

It used to be that you had to explicitly enable the boot profile on compile if you wanted a jar file that could be started and bootstrap an embedded web server (the default was to create a deployable war file instead to deploy to a separate web server instance). That was changed recently, which probably makes running the integration tests other things simpler because you don't have to specify a profile in the IDE.
I'm not certain of the history behind Jetty vs Tomcat.
@jkiddo may have more info or perspective.

I haven't tried to compile or run your branch, but it looks like it could break some peoples' configurations (I could be wrong).

What is your ultimate goal? Are you trying to deploy to a standalone Tomcat instance or something?

@stmsat
Copy link
Contributor Author

stmsat commented Dec 22, 2023

What is your ultimate goal? Are you trying to deploy to a standalone Tomcat instance or something?

Yes, the main goal is to deploy on a standalone tomcat instance, but I'd also like to use spring-boot with tomcat sometimes.

I use hapi-fhir-jpaserver-starter as a dependency for a project of mine, so I have full control on my pom.xml, but I fully depend on the hapifhir java implementation; so I needed some java changes (which by the way are only in this WIP branch targeted to hapi 7.0), but I didn't want them to break things for others, so I tried to fix things that didn't seem to work in the pom.xml (sometimes making assumptions that may be wrong).

What didn't work:

  • deploying on standalone tomcat didn't work if I didn't at least declare servletWebServerFactory bean as @Lazy
  • starting spring-boot wouldn't work in any way (not with tomcat, nor with jetty): servletWebServerFactory bean would be instantiated and the required jetty dependencies simply weren't there, because they were declared with scope=testing

So my "fix" is partly something I need (the java change) and partly a proposal of a way to keep things going for other people, but with more feedback we might find better options.

added required jetty dependencies through spring-boot-starter-jetty so this is the only dependency with scope=testing and transitive dependencies (core jetty libraries) aren't marked as such

mvn -P jetty spring-boot:run seems to work
jetty-maven-plugin is obsolete, and jetty-ee10-maven-plugin doesn't work at the moment. Using spring-boot to run local server.
@stmsat
Copy link
Contributor Author

stmsat commented Dec 22, 2023

I tried to make smoke-tests.yml work again with jetty-ee10-maven-plugin but I wasn't able to, so I changed mvn jetty:run with mvn -P jetty spring-boot:run. It works, we just have to decide if it fits into the big picture.

@jkiddo
Copy link
Collaborator

jkiddo commented Dec 22, 2023

I would need to hear a really good argument if we are going away from our new default being spring boot

@stmsat
Copy link
Contributor Author

stmsat commented Dec 22, 2023

I would need to hear a really good argument if we are going away from our new default being spring boot

May I ask what made you take this decision, and where you discussed this? This is obvioulsy a legitimate position if you're going to benefit from what spring-boot has to offer.

On the other hand, are you going to force a particular container (jetty over tomcat, undertow, etc.), or leaving some choice? Is there a need to hard-code references to jetty, or you might leave that choice in a profile in the pom?

I was just going to try if parallel deployment on tomcat is feasible, to reduce downtime for users, but I'll spare myself this if deployment as a web application will disappear as an option.

@jkiddo
Copy link
Collaborator

jkiddo commented Dec 22, 2023

Tomcat, jetty and undertow all work with spring boot. The majority of HAPI FHIR starter users are expected to be spring boot users and the general use of standalone app server use is not growing AFAIK.

@jkiddo
Copy link
Collaborator

jkiddo commented Dec 22, 2023

The starter project is a sandbox project, IMHO. It is not intended to be directly used as a service for production. It serves as a place for showing how the HAPI FHIR framework can be used

…ing-boot:run by defaults starts tomcat, while the jetty profile enables jetty

added a fix to run the generated war with spring-boot on tomcat, due to issues with spring-boot 3.2.0 spring-projects/spring-boot#38585
the jar generated with jetty doesn't work at the moment
@stmsat
Copy link
Contributor Author

stmsat commented Jan 3, 2024

I reverted the profiles to what is in the master branch, so that spring-boot:run starts tomcat by default while jetty is runnable adding -P jetty. I don't see advantages from doing differently at the moment.

The current situation with my branch is:

  • mvn spring-boot:run starts tomcat and it works, except for the tester pages (only /fhir works, but this is also true for master branch)
  • mvn -P jetty spring-boot:run starts jetty and it works, except for the tester pages (only /fhir works, but this is also true for master branch)
  • mvn clean package && java -jar ./target/ROOT.war starts tomcat and everything works, but only if we add <loaderImplementation>CLASSIC</loaderImplementation> to the configuration of spring-boot-maven-plugin, apparently because of this issue with spring-boot 3.2.0; i tried upgrading to spring-boot 3.2.1 but didn't work
  • mvn -P jetty clean package && java -jar ./target/ROOT.war doesn't work in any way at the moment
  • jetty plugin doesn't work (even if we use the ee10 version)
  • deploying to tomcat works
  • deploying to wildfly doesn't work (issues with hibernate-search/jandex), probable issues with spring-boot and undertow (didn't test)

@stmsat
Copy link
Contributor Author

stmsat commented Jan 3, 2024

Ah, no, here's the reason apparently: tests don't work if the boot profile doesn't exclude spring-boot-starter-tomcat. I'll dig into this.

@jkiddo
Copy link
Collaborator

jkiddo commented Jan 3, 2024

Its a known issue that the tester pages only work if you run it as a war/jar file or within docker due to the way resources (the html pages and css and so on) are loaded. That is a minor thing, IMHO. The main functionality is that the FHIR endpoints work regardless of how it is run.

…ess "jetty" profile is active (in which case they're done with jetty)

Test class JpaStarterWebsocketDispatcherConfig emptied, not sure if necessary (it's now missing jetty dependencies)
@stmsat
Copy link
Contributor Author

stmsat commented Jan 3, 2024

Maybe I'm talking too soon, but I hope that everything I could fix is now fixed: tests now aren't failing (and they work both on tomcat and on jetty), and all of the above seems to work. It was a nasty short blanket dilemma.

I emptied the JpaStarterWebsocketDispatcherConfig because I removed jetty dependencies and it didn't compile anymore, but nothing seems to have broken because of this. It looks removable to me, but I don't know the backstory.

Having a runnable war with jetty will hopefully be possible with a future release of some dependency, I don't think we can fix this here.

If you have feedback you're welcome, as always.

@XcrigX
Copy link
Contributor

XcrigX commented Jan 10, 2024

I haven't had a chance to test this yet, but I'm definitely a fan of the simplifications made, FWIW.

@dotasek dotasek self-requested a review January 24, 2024 19:21
Copy link
Contributor

@dotasek dotasek left a comment

Choose a reason for hiding this comment

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

I have tested this locally using the smoke tests with the following executions:

mvn -P jetty spring-boot:run       
mvn spring-boot:run   
mvn clean package && java -jar ./target/ROOT.war  

All of these run, all the unit and IT tests pass, and all the smoke tests pass. I bumped up the HAPI version to 7.0.0-SNAPSHOT as well.

It looks like additional config is removed here that doesn't impact the serving of basic resources. I can't speak to what that impacts, but if @jkiddo's commentary is an indication, we only need to ensure minimum functionality regardless of running Jetty/Tomcat, which I see evidenced here.

The only changes I would request before approving:

  • completely delete the commented out code, unless there are portions that CAN be added in to enhance functionality.
  • Bump to 7.0.0-SNAPSHOT of HAPI

@XcrigX
Copy link
Contributor

XcrigX commented Jan 24, 2024

I have now deployed this in a test environment also. I haven't run it through the ringer (i.e. thoroughly tested it) , but can say it built/deployed without any issues and basic FHIR APIs work. FWIW - I'm building a SpringBoot runnable jar and and packaging into a custom docker image which is deployed to Kubernetes.

… JpaStarterWebsocketDispatcherConfig and references

unrequested changes: commons-logging dependency marked provided (to avoid redundant jar inclusion), removed unused imports in classes that were modified
@stmsat
Copy link
Contributor Author

stmsat commented Jan 25, 2024

I upgraded the parent to version 7.0.0-SNAPSHOT, and removed JpaStarterWebsocketDispatcherConfig from the test sources completely as there isn't a use for it at the moment.

I also added a provided commons-logging dependency, to avoid having both commons-logging and jcl-over-slf4j jars in the target (the usual trick).

@stmsat stmsat requested a review from dotasek January 25, 2024 08:37
@dotasek dotasek merged commit b06b5c8 into hapifhir:ja_20231203_hapi_7_0 Jan 25, 2024
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.

None yet

4 participants