[MNG-8178] Fall back to system properties for missing profile activation context properties #1603

kohlschuetter · 2024-07-09T17:09:13Z

A call to context.getSystemProperties() may yield empty an empty map, or one missing the desired key, which makes a subsequent call of toLowerCase fail with a NullPointerException.

Fall-back to using System.getProperty (with an empty default, in case that one fails too).

Following this checklist to help us incorporate your
contribution quickly and easily:

Make sure there is a JIRA issue filed
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
Each commit in the pull request should have a meaningful subject line and body.
Format the pull request title like [MNG-XXX] SUMMARY,
where you replace MNG-XXX and SUMMARY with the appropriate JIRA issue.
Also format the first line of the commit message like [MNG-XXX] SUMMARY.
Best practice is to use the JIRA issue title in both the pull request title and in the first line of the commit message.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Run mvn clean verify to make sure basic checks pass. A more thorough check will
be performed on your pull request automatically.
You have run the Core IT successfully.

If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.

To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004

A call to context.getSystemProperties() may yield empty an empty map, or one missing the desired key, which makes a subsequent call of toLowerCase fail with a NullPointerException. Fall-back to using System.getProperty (with an empty default, in case that one fails too).

slawekjaranowski · 2024-07-09T18:12:58Z

We should try to find root cause ... why properties is not available in contexts

kohlschuetter · 2024-07-09T18:27:39Z

@slawekjaranowski DefaultProfileActivationContext.setSystemProperties allows empty/null system properties (the default value if setSystemProperties isn't called is an empty map, too). Moreover, I don't think we can make any assumptions on "os.name", etc., to be available.

As to where this from: I've added an exception to DefaultProfileActivationContext that checks for empty property maps, and this is what I get

    at org.apache.maven.model.profile.DefaultProfileActivationContext.setSystemProperties (DefaultProfileActivationContext.java:108)
    at org.apache.maven.model.building.DefaultModelBuilder.getProfileActivationContext (DefaultModelBuilder.java:661)
    at org.apache.maven.model.building.DefaultModelBuilder.build (DefaultModelBuilder.java:265)
    at org.apache.maven.model.building.DefaultModelBuilder.build (DefaultModelBuilder.java:247)
    at org.apache.maven.project.DefaultProjectBuilder.build (DefaultProjectBuilder.java:176)
    at org.apache.maven.project.DefaultProjectBuilder.build (DefaultProjectBuilder.java:123)
    at org.apache.maven.report.projectinfo.ModulesReport$ModulesRenderer.renderBody (ModulesReport.java:174)
    at org.apache.maven.reporting.AbstractMavenReportRenderer.render (AbstractMavenReportRenderer.java:82)
    at org.apache.maven.report.projectinfo.ModulesReport.executeReport (ModulesReport.java:75)
    at org.apache.maven.reporting.AbstractMavenReport.generate (AbstractMavenReport.java:289)
    at org.apache.maven.plugins.site.render.ReportDocumentRenderer.renderDocument (ReportDocumentRenderer.java:198)
    at org.apache.maven.doxia.siterenderer.DefaultSiteRenderer.render (DefaultSiteRenderer.java:298)
    at org.apache.maven.plugins.site.render.SiteMojo.renderNonDoxiaDocuments (SiteMojo.java:280)
    at org.apache.maven.plugins.site.render.SiteMojo.renderLocale (SiteMojo.java:162)
    at org.apache.maven.plugins.site.render.SiteMojo.execute (SiteMojo.java:115)
    at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo (DefaultBuildPluginManager.java:126)
    at org.apache.maven.lifecycle.internal.MojoExecutor.doExecute2 (MojoExecutor.java:328)
    at org.apache.maven.lifecycle.internal.MojoExecutor.doExecute (MojoExecutor.java:316)
    at org.apache.maven.lifecycle.internal.MojoExecutor.execute (MojoExecutor.java:212)
    at org.apache.maven.lifecycle.internal.MojoExecutor.execute (MojoExecutor.java:174)
    at org.apache.maven.lifecycle.internal.MojoExecutor.access$000 (MojoExecutor.java:75)
    at org.apache.maven.lifecycle.internal.MojoExecutor$1.run (MojoExecutor.java:162)
    at org.apache.maven.plugin.DefaultMojosExecutionStrategy.execute (DefaultMojosExecutionStrategy.java:39)
    at org.apache.maven.lifecycle.internal.MojoExecutor.execute (MojoExecutor.java:159)
    at org.apache.maven.lifecycle.internal.LifecycleModuleBuilder.buildProject (LifecycleModuleBuilder.java:105)
    at org.apache.maven.lifecycle.internal.LifecycleModuleBuilder.buildProject (LifecycleModuleBuilder.java:73)
    at org.apache.maven.lifecycle.internal.builder.singlethreaded.SingleThreadedBuilder.build (SingleThreadedBuilder.java:53)
    at org.apache.maven.lifecycle.internal.LifecycleStarter.execute (LifecycleStarter.java:118)
    at org.apache.maven.DefaultMaven.doExecute (DefaultMaven.java:261)
    at org.apache.maven.DefaultMaven.doExecute (DefaultMaven.java:173)
    at org.apache.maven.DefaultMaven.execute (DefaultMaven.java:101)
    at org.apache.maven.cli.MavenCli.execute (MavenCli.java:903)
    at org.apache.maven.cli.MavenCli.doMain (MavenCli.java:280)
    at org.apache.maven.cli.MavenCli.main (MavenCli.java:203)
    at jdk.internal.reflect.DirectMethodHandleAccessor.invoke (DirectMethodHandleAccessor.java:103)
    at java.lang.reflect.Method.invoke (Method.java:580)
    at org.codehaus.plexus.classworlds.launcher.Launcher.launchEnhanced (Launcher.java:255)
    at org.codehaus.plexus.classworlds.launcher.Launcher.launch (Launcher.java:201)
    at org.codehaus.plexus.classworlds.launcher.Launcher.mainWithExitCode (Launcher.java:361)
    at org.codehaus.plexus.classworlds.launcher.Launcher.main (Launcher.java:314)

cstamas · 2024-07-10T10:50:51Z

    at org.apache.maven.doxia.siterenderer.DefaultSiteRenderer.render (DefaultSiteRenderer.java:298)

?

You are building a site here? As we have related issue https://issues.apache.org/jira/browse/DOXIASITETOOLS-343 that may be related...

kohlschuetter · 2024-07-10T10:56:08Z

@cstamas fwiw, I'm seeing this with 3.9.7/3.9.8. The behavior of the site tools probably was like this long before, but the bug is only triggered due to changes made with MNG-5726

kohlschuetter · 2024-07-10T10:57:10Z

Bug description here https://issues.apache.org/jira/browse/MNG-8135

gnodet · 2024-07-10T11:06:58Z

@slawekjaranowski DefaultProfileActivationContext.setSystemProperties allows empty/null system properties (the default value if setSystemProperties isn't called is an empty map, too). Moreover, I don't think we can make any assumptions on "os.name", etc., to be available.

As to where this from: I've added an exception to DefaultProfileActivationContext that checks for empty property maps, and this is what I get

    at org.apache.maven.model.profile.DefaultProfileActivationContext.setSystemProperties (DefaultProfileActivationContext.java:108)
    at org.apache.maven.model.building.DefaultModelBuilder.getProfileActivationContext (DefaultModelBuilder.java:661)
    at org.apache.maven.model.building.DefaultModelBuilder.build (DefaultModelBuilder.java:265)
    at org.apache.maven.model.building.DefaultModelBuilder.build (DefaultModelBuilder.java:247)
    at org.apache.maven.project.DefaultProjectBuilder.build (DefaultProjectBuilder.java:176)
    at org.apache.maven.project.DefaultProjectBuilder.build (DefaultProjectBuilder.java:123)
    at org.apache.maven.report.projectinfo.ModulesReport$ModulesRenderer.renderBody (ModulesReport.java:174)
    at org.apache.maven.reporting.AbstractMavenReportRenderer.render (AbstractMavenReportRenderer.java:82)
    at org.apache.maven.report.projectinfo.ModulesReport.executeReport (ModulesReport.java:75)
    at org.apache.maven.reporting.AbstractMavenReport.generate (AbstractMavenReport.java:289)
    at org.apache.maven.plugins.site.render.ReportDocumentRenderer.renderDocument (ReportDocumentRenderer.java:198)
    at org.apache.maven.doxia.siterenderer.DefaultSiteRenderer.render (DefaultSiteRenderer.java:298)
    at org.apache.maven.plugins.site.render.SiteMojo.renderNonDoxiaDocuments (SiteMojo.java:280)
    at org.apache.maven.plugins.site.render.SiteMojo.renderLocale (SiteMojo.java:162)
    at org.apache.maven.plugins.site.render.SiteMojo.execute (SiteMojo.java:115)
    at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo (DefaultBuildPluginManager.java:126)
    at org.apache.maven.lifecycle.internal.MojoExecutor.doExecute2 (MojoExecutor.java:328)
    at org.apache.maven.lifecycle.internal.MojoExecutor.doExecute (MojoExecutor.java:316)
    at org.apache.maven.lifecycle.internal.MojoExecutor.execute (MojoExecutor.java:212)
    at org.apache.maven.lifecycle.internal.MojoExecutor.execute (MojoExecutor.java:174)
    at org.apache.maven.lifecycle.internal.MojoExecutor.access$000 (MojoExecutor.java:75)
    at org.apache.maven.lifecycle.internal.MojoExecutor$1.run (MojoExecutor.java:162)
    at org.apache.maven.plugin.DefaultMojosExecutionStrategy.execute (DefaultMojosExecutionStrategy.java:39)
    at org.apache.maven.lifecycle.internal.MojoExecutor.execute (MojoExecutor.java:159)
    at org.apache.maven.lifecycle.internal.LifecycleModuleBuilder.buildProject (LifecycleModuleBuilder.java:105)
    at org.apache.maven.lifecycle.internal.LifecycleModuleBuilder.buildProject (LifecycleModuleBuilder.java:73)
    at org.apache.maven.lifecycle.internal.builder.singlethreaded.SingleThreadedBuilder.build (SingleThreadedBuilder.java:53)
    at org.apache.maven.lifecycle.internal.LifecycleStarter.execute (LifecycleStarter.java:118)
    at org.apache.maven.DefaultMaven.doExecute (DefaultMaven.java:261)
    at org.apache.maven.DefaultMaven.doExecute (DefaultMaven.java:173)
    at org.apache.maven.DefaultMaven.execute (DefaultMaven.java:101)
    at org.apache.maven.cli.MavenCli.execute (MavenCli.java:903)
    at org.apache.maven.cli.MavenCli.doMain (MavenCli.java:280)
    at org.apache.maven.cli.MavenCli.main (MavenCli.java:203)
    at jdk.internal.reflect.DirectMethodHandleAccessor.invoke (DirectMethodHandleAccessor.java:103)
    at java.lang.reflect.Method.invoke (Method.java:580)
    at org.codehaus.plexus.classworlds.launcher.Launcher.launchEnhanced (Launcher.java:255)
    at org.codehaus.plexus.classworlds.launcher.Launcher.launch (Launcher.java:201)
    at org.codehaus.plexus.classworlds.launcher.Launcher.mainWithExitCode (Launcher.java:361)
    at org.codehaus.plexus.classworlds.launcher.Launcher.main (Launcher.java:314)

The assumption is not context.getSystemProperties() returns... system properties. So imho, the PR is wrong and it means the caller does not set the system properties correctly. In Maven itself, the only place is in DefaultModelBuilder, where properties are set correctly afaik. So the bug is that the system properties are not correctly set on the request:
https://github.com/apache/maven-project-info-reports-plugin/blob/master/src/main/java/org/apache/maven/report/projectinfo/ModulesReport.java#L160-L162

gnodet

The assumption is not context.getSystemProperties() returns... system properties. So imho, the PR is wrong and it means the caller does not set the system properties correctly. In Maven itself, the only place is in DefaultModelBuilder, where properties are set correctly afaik. So the bug is that the system properties are not correctly set on the request:
https://github.com/apache/maven-project-info-reports-plugin/blob/master/src/main/java/org/apache/maven/report/projectinfo/ModulesReport.java#L160-L162

This is a problem in maven-project-info-reports-plugin and should be fixed there imho.

kohlschuetter · 2024-07-10T11:09:49Z

@gnodet I see. Can you please coordinate this? I'm just a frustrated user. Thanks a lot!

gnodet · 2024-07-10T11:16:21Z

We could mitigate the issue by supporting null values though instead of calling toLowerCase() on the maybe null property.

kohlschuetter · 2024-07-10T11:20:20Z

@gnodet By just "handling" null values (e.g., converting to ""), we wouldn't get the correct "active" state for profiles based on the OS.

I really don't see what we'd lose by falling back to System.getProperty.

kohlschuetter · 2024-07-10T11:21:27Z

@gnodet Moreover, this is a regression from 3.9.6. Who knows, maybe more plugins are affected by this.

gnodet · 2024-07-10T14:24:05Z

@gnodet Moreover, this is a regression from 3.9.6. Who knows, maybe more plugins are affected by this.

Ah, I see now.
We're trying to chase down the calls to System.getProperty, so can we rather use the following instead:

        String actualOsName = context.getSystemProperties().getOrDefault("os.name", Os.OS_NAME).toLowerCase(Locale.ENGLISH);
        String actualOsArch = context.getSystemProperties().getOrDefault("os.arch", Os.OS_ARCH).toLowerCase(Locale.ENGLISH);
        String actualOsVersion = context.getSystemProperties().getOrDefault("os.version", Os.OS_VERSION).toLowerCase(Locale.ENGLISH);

It should have the desired behaviour without introducing new calls.

michael-o · 2024-07-10T14:59:11Z

Am I stupid or why are these props empty?

gnodet · 2024-07-10T15:08:03Z

Am I stupid or why are these props empty?

Look at apache/maven-project-info-reports-plugin#75

michael-o · 2024-07-10T15:12:28Z

Am I stupid or why are these props empty?

Look at apache/maven-project-info-reports-plugin#75

Nice catch, let me please review all reporting plugins before we merge this. I do remember some other stuff.

michael-o

Fine with me, will continue on the fix in MPIR.

kohlschuetter force-pushed the ck/MNG-8178 branch from 1029de6 to 34d63f0 Compare July 9, 2024 17:09

kohlschuetter changed the title ~~MNG-8178: Fall back to System.getProperty for missing context props~~ [MNG-8178]: Fall back to System.getProperty for missing context props Jul 9, 2024

kohlschuetter force-pushed the ck/MNG-8178 branch from 34d63f0 to b3a0523 Compare July 9, 2024 17:10

kohlschuetter changed the title ~~[MNG-8178]: Fall back to System.getProperty for missing context props~~ [MNG-8178] Fall back to System.getProperty for missing context props Jul 9, 2024

cstamas requested a review from kwin July 9, 2024 18:25

gnodet requested changes Jul 10, 2024

View reviewed changes

gnodet mentioned this pull request Jul 10, 2024

The project builder should be given the correct system properties apache/maven-project-info-reports-plugin#75

Merged

Use Os class to grab values instead

Loading
Loading status checks…

a770d61

gnodet approved these changes Jul 10, 2024

View reviewed changes

cstamas approved these changes Jul 10, 2024

View reviewed changes

slawekjaranowski approved these changes Jul 10, 2024

View reviewed changes

michael-o approved these changes Jul 10, 2024

View reviewed changes

gnodet added this to the 3.9.9 milestone Jul 10, 2024

gnodet merged commit 5223ff6 into apache:maven-3.9.x Jul 10, 2024
12 checks passed

gnodet changed the title ~~[MNG-8178] Fall back to System.getProperty for missing context props~~ [MNG-8178] Fall back to system properties for missing profile activation context properties Jul 10, 2024

gnodet mentioned this pull request Jul 10, 2024

[MNG-8178] Fall back to system properties for missing profile activation context properties #1609

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MNG-8178] Fall back to system properties for missing profile activation context properties #1603

[MNG-8178] Fall back to system properties for missing profile activation context properties #1603

kohlschuetter commented Jul 9, 2024 •

edited

Loading

slawekjaranowski commented Jul 9, 2024

kohlschuetter commented Jul 9, 2024

cstamas commented Jul 10, 2024

kohlschuetter commented Jul 10, 2024

kohlschuetter commented Jul 10, 2024

gnodet commented Jul 10, 2024

gnodet left a comment

kohlschuetter commented Jul 10, 2024

gnodet commented Jul 10, 2024

kohlschuetter commented Jul 10, 2024

kohlschuetter commented Jul 10, 2024

gnodet commented Jul 10, 2024

michael-o commented Jul 10, 2024

gnodet commented Jul 10, 2024

michael-o commented Jul 10, 2024

michael-o left a comment

[MNG-8178] Fall back to system properties for missing profile activation context properties #1603

[MNG-8178] Fall back to system properties for missing profile activation context properties #1603

Conversation

kohlschuetter commented Jul 9, 2024 • edited Loading

slawekjaranowski commented Jul 9, 2024

kohlschuetter commented Jul 9, 2024

cstamas commented Jul 10, 2024

kohlschuetter commented Jul 10, 2024

kohlschuetter commented Jul 10, 2024

gnodet commented Jul 10, 2024

gnodet left a comment

Choose a reason for hiding this comment

kohlschuetter commented Jul 10, 2024

gnodet commented Jul 10, 2024

kohlschuetter commented Jul 10, 2024

kohlschuetter commented Jul 10, 2024

gnodet commented Jul 10, 2024

michael-o commented Jul 10, 2024

gnodet commented Jul 10, 2024

michael-o commented Jul 10, 2024

michael-o left a comment

Choose a reason for hiding this comment

kohlschuetter commented Jul 9, 2024 •

edited

Loading