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

Chore/upgrade spring to 5 3 #216

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

cbeach47
Copy link

@cbeach47 cbeach47 commented Sep 12, 2023

Spring upgrade to 5.3.

  • Brought in Liferay portlet support
  • Fixed some deprecation warnings due to dependencies
    • Removed the json mapper view. The JSON portlet type does not appear to leverage that mapper view.
  • Deprecated Gateway SSO portlet type
    • Compiles, but did not test runtime functionality
  • Added lombok dependency and replace log instantiation with @Slf4j.
  • Ensured all log4j dependencies/references were removed
  • [EDIT]: Hook in uPortal parent pom and clean up deps.

Testing note: Due to available time, not all classes with the logging switch were tested. However - some were tested, and the logging switch was a simple switch across the board without logic adjustments.

Copy link
Member

@ChristianMurphy ChristianMurphy left a comment

Choose a reason for hiding this comment

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

Thanks @cbeach47!
From skimming the code (I have not tested this locally) it looks good!

@groybal
Copy link
Contributor

groybal commented Sep 12, 2023

I see some issues with dependencies when I deploy the portlet to Tomcat. In WEB-INF/lib, I noticed:

  • I see both of these jars but I think we only want the slf4j one: commons-logging-1.2.jar and
    jcl-over-slf4j-1.7.5.jar
  • there are two jaxb-core jars (jaxb-core-2.2.7.jar and jaxb-core-2.3.0.1.jar)
  • I see this jar,which I think is no longer needed: spring-webmvc-portlet-4.3.30.RELEASE.jar
  • I see mockito-core-4.9.0.jar, which is only needed for testing and therefore should not be here

Comment on lines -201 to -206
<dependency>
<groupId>commons-logging</groupId>
<artifactId>commons-logging</artifactId>
<version>1.2</version>
<scope>provided</scope>
</dependency>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this removed? When I deploy the portlet to Tomcat, I see both these jars in WEB-INF/lib, but I think we only want the slf4j one:

  • commons-logging-1.2.jar
  • jcl-over-slf4j-1.7.5.jar

Copy link
Author

Choose a reason for hiding this comment

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

For logging, the goal is logback (via the slf4j api). All other logging implementations (even transitive ones) should be removed. I'll look into how commons-logging is still getting deployed.

The JCL bridges (-over-) are now included in spring-jcl, so we don't have to explicitly declare them in the pom, however, I'll need to check if jcl-over-slf4j-1.7.5.jar is from spring-jcl, or a transitive dependency that should be excluded.

Copy link
Contributor

Choose a reason for hiding this comment

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

So, lines 201-206 actually prevent any dependency from pulling in commons-logging, which is why I think it should remain. Otherwise, you have to find which dependencies are pulling in commons-logging and add excludes.

Copy link
Author

Choose a reason for hiding this comment

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

As per our side discussion with @bjagg , I'm going in the direction of using the maven enforcer plugin, and then excluding the commons-logging jars as needed. @groybal - please let me know if you don't feel this is sufficient.

Copy link
Author

Choose a reason for hiding this comment

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

discussed in PR - change the parent pom to have a dependencies section with the excluded deps as 'provided'

pom.xml Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
@cbeach47
Copy link
Author

cbeach47 commented Sep 25, 2023

@groybal -

  • I see both of these jars but I think we only want the slf4j one: commons-logging-1.2.jar and
    jcl-over-slf4j-1.7.5.jar

I've now set up the maven enforcer to ban commons-logging, and adjusted the pom to not emit the commons-logging dep.

  • there are two jaxb-core jars (jaxb-core-2.2.7.jar and jaxb-core-2.3.0.1.jar)

Took a deeper look at the jaxb-core jars - These 'duplicate' jars were present prior to upgrade, and deals with CAS and personDir flows. In order to control scope, I am to delay this. Added #217 for a future effort.

  • I see this jar,which I think is no longer needed: spring-webmvc-portlet-4.3.30.RELEASE.jar

Good catch - I've removed it now.

  • I see mockito-core-4.9.0.jar, which is only needed for testing and therefore should not be here

Ah - the scope was misconfigured. This is now removed in the deployment.

</build>
</profile>
<profile>
<id>doclint-java8-disable</id>
Copy link
Author

Choose a reason for hiding this comment

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

Since we only support Java 8+ , doclint is disabled at the maven-javadoc-plugin level in the parent pom.

@@ -51,18 +52,8 @@
<!-- Dependency Version Properties -->
<properties>
Copy link
Author

Choose a reason for hiding this comment

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

Collapse dependencyManagement

</configuration>
</plugin>
<plugin>
<!-- Overrides the parent pom due to the overlays -->
<groupId>org.apache.maven.plugins</groupId>
Copy link
Author

Choose a reason for hiding this comment

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

Leave a note similar to SCP's plugin

</plugins>
</build>

Copy link
Author

Choose a reason for hiding this comment

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

Good to remove - note, that this locally builds, and pluto-izes the war file. Add this to the release notes.

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.

3 participants