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

WW-5437 Swap order of sysStrSubstitutor and envStrSubstitutor in substitute method #977

Conversation

stefansielaff
Copy link
Contributor

@stefansielaff stefansielaff commented Jul 2, 2024

According to the documentation at https://struts.apache.org/core-developers/constant-configuration it should be possible to use both system and environment variables in the constants section. Currently environment variables are ignored if a default value is defined.

The sysStrSubstitutor has a less specific prefix which also includes and replaces those, which should be passed to the envStrSubstitutor later.

Given
<constant name="struts.devMode" value="${env.STRUTS_DEV_MODE:false}"/>
and System.getenv('STRUTS_DEV_MODE') is "true"

The old code:

String substituted = sysStrSubstitutor.replace(value);
return envStrSubstitutor.replace(substituted);

The sysStrSubstitutor checks, if there is a system property with the key "env.STRUTS_DEV_MODE" which is unset. It then replaces the expression with its default. substituted is "false" now. Afterwards the envStrSubstitutor doesn't find any expression to substitute, because the string is "false".

The new code:

String substituted = envStrSubstitutor.replace(value);
return sysStrSubstitutor.replace(substituted);

The envStrSubstitutor only accepts expressions prefixed with "env.", so it ignores any unprefixed (aka system-) variables. If not set, substituted remains unchanged and is replaced by the system variable or eventually the default in the next step.

I'm not sure if this all is right and understandable but for me, the new behavior is what I've exprected after reading the docs and telling my DevOps team how to enable the devMode for my test-container :)

Closes WW-5437

@lukaszlenart
Copy link
Member

Nice, could you create a ticket in JIRA? It will be easier for users to find what and when it has changed, thanks in advance!

@lukaszlenart lukaszlenart changed the title Swap order of sysStrSubstitutor and envStrSubstitutor in substitute method WW-5437 Swap order of sysStrSubstitutor and envStrSubstitutor in substitute method Jul 5, 2024
Copy link
Member

@lukaszlenart lukaszlenart left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@lukaszlenart lukaszlenart merged commit 82b364d into apache:master Jul 5, 2024
7 checks passed
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.

2 participants