Embedded GlassFish - fixes related to system properties #25724

OndroMih · 2025-10-06T01:45:28Z

System properties can be set by the following means:

on command line, with -D Java argument - overrides properties set in the configuration
via create-system-properties admin command (spaces in property names or values must be escaped by '')
via setting the property directly in the --propeties file (e.g. property "my.name" should have line "my.name=GlassFish Server" in the properties file, spaces shouldn't be escaped)
via system-property element in domain.xml

Fixes number 3, which worked in 7.0.25 but stopped working in master.
Allows empty spaces in property values if setting via the create-system-properties admin command - space escaped by \ is treated as value not as a separator. This follows the same pattern as in the asadmin CLI tool.

Properties are not overwritten, if set on command line or by other means before starting the server.

dmatej · 2025-10-06T08:25:34Z

...us/common/simple-glassfish-api/src/main/java/org/glassfish/embeddable/GlassFishVariable.java

        return this.sysPropName;
    }

+    public Optional<String> getSystemPropertyValue() {


This lib should be without tech debt and with good design. (EDIT: originally I thought it is in jdke, but it applies to the simple api too)

Missing javadoc

When I created GlassFishVariable, I had doubts if I should create also getValue methods here, however I have found that it is a not good idea, because they don't save me of any pain. In your case, -Dcom.sun.aas.hostName="" (empty string) is not an empty optional. Also, sometimes I need a supplier, sometimes it would be better to use just optional, sometimes, I need some processing ... and soon it would be messy as usual.

Exactly opposite, evaluation of these variables are always somehow specific to some domain. There it belongs to. This class serves just to replace old set of hashmaps and constants used on older version, and give some order to the mapping of env option and system properties.

So I would move it to the SystemTasksImpl and maybe we could even improve the validation. Actually I am now looking on other usages, one other usages outside this PR contains a bug, another has completely different evaluation of the hostname, maybe depending on this code, not sure ... we should clean it up.

However I did not put these methods to GlassFishVariable intentionally.

Another idea: just remove this and use just nullcheck in SystemTasksImpl and we will target the hostname evaluation in another PR.

OK. I'll just inline this methog SystemTasksImpl and remove it from GlassFishVariable. I added it to GlassFish variable because it makes the code cleaner in SystemTasksImpl, HOST_NAME.getSystemPropertyValue().isEmpty() instead of System.getProperty(HOST_NAME.getPropertyName()) != null), but it's not much different and still readable.

In fact, if I change the code to respect forceOverwrite, then I don't even need this at all :)

I might need it to skip getting hostname if the property is already set and forceOverwrite is false.

I still don't know what to do if the property value is an empty string. Is it the same as an empty string (explicit reset)? Or it can be taken as an error. Maybe the first option is correct, then the optional doesn't work. As a paradox Optional can make it more complicated compared to old and ugly "null or empty" check.

I removed this method already

And I only check for null, not empty. Empty means a value, the only reason why it would mean the property is not set would be if we want to allow deleting the property by setting it to an empty string. Makes sense in some cases, but I wouldn't bother here.

Yeah, but empty hostname doesn't sound something valid for me.

dmatej · 2025-10-06T08:27:45Z

nucleus/core/kernel/src/main/java/org/glassfish/runnablejar/UberMain.java

            for (int i = 1; i < split.length; i++) {
                commandParams[i - 1] = split[i].trim();
-            }
+        }


dmatej · 2025-10-06T08:29:25Z

nucleus/core/kernel/src/main/java/com/sun/enterprise/v3/server/SystemTasksImpl.java

+                LOG.log(Level.SEVERE, KernelLoggerInfo.exceptionHostname, ex);
+            }
+            if (hostname != null) {
+                setProperty(HOST_NAME.getSystemPropertyName(), hostname, false);


I think we both have a very good question ... why to ignore the forceOverwrite here.

OK, I'll change it to follow forceOverwrite also here.

I changed it so that it uses only attempts to compute hostname and set the value if either forceOverwrite=true or if the value is not set. To avoid computing hostname if it's going to be ignored in the end. A very small optimization for an edge case, I know :)

dmatej · 2025-10-07T17:24:33Z

appserver/tests/embedded/runnable

2:28:49  [ERROR] src/test/resources/SystemPropertyTest/glassfish.properties:[2] (regexp) RegexpSingleline: Line has trailing spaces.
22:28:49  [ERROR] src/test/resources/SystemPropertyTest/glassfish.properties:[6] (regexp) RegexpSingleline: Line has trailing spaces.
22:28:49  [ERROR] src/test/resources/SystemPropertyTest/glassfish.properties:[12] (regexp) RegexpSingleline: Line has trailing spaces.

…d GlassFish

dmatej · 2025-10-08T09:42:32Z

So as you explained to me, the whole -Dx=y command line support is a new feature in 7.1, while originally I assumed that it worked years ago and it was just broken from some point.

OndroMih added 3 commits October 5, 2025 08:11

Embedded GlassFish reads system properties from domain.xml

6d51fdd

Properties are not overwritten, if set on command line or by other means before starting the server.

Embedded: Tests for setting sys props

0ff756f

Embedded: Support escaped spaces in admin command arguments

d496888

dmatej reviewed Oct 6, 2025

View reviewed changes

Improvements after code review

3dad4ae

OndroMih requested a review from dmatej October 6, 2025 14:45

OndroMih force-pushed the ondromih-embedded-fixes branch 2 times, most recently from a5763f2 to e0b5f80 Compare October 6, 2025 20:13

Simple system properties in glassfish.properties for runneble Embedde…

ca42dce

…d GlassFish

OndroMih force-pushed the ondromih-embedded-fixes branch from e0b5f80 to ca42dce Compare October 8, 2025 06:46

dmatej approved these changes Oct 8, 2025

View reviewed changes

dmatej added this to the 7.1.0 milestone Oct 8, 2025

dmatej added the enhancement New feature or request label Oct 8, 2025

avpinchuk approved these changes Oct 8, 2025

View reviewed changes

arjantijms approved these changes Oct 8, 2025

View reviewed changes

arjantijms merged commit d2a27ca into eclipse-ee4j:master Oct 8, 2025
6 checks passed

Embedded GlassFish - fixes related to system properties #25724

Embedded GlassFish - fixes related to system properties #25724

Uh oh!

Conversation

OndroMih commented Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmatej Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmatej commented Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmatej commented Oct 8, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

OndroMih commented Oct 6, 2025 •

edited

Loading

dmatej Oct 6, 2025 •

edited

Loading

dmatej commented Oct 7, 2025 •

edited

Loading