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

Feature: Preprocess JVM properties at app start to adjust to system environment. #2957

Merged
merged 25 commits into from Jun 30, 2023

Conversation

infeo
Copy link
Member

@infeo infeo commented Jun 15, 2023

Fixes #2838. Relates to #2885 (comment)

This PR introduces a preprocessing of JVM properties at application start to react system environments settings. The preprocessing is done in way which will be compatible with the JDK 21 preview feature of String templates.

To substitute a string in a property, it needs to be enclosed in @( ) @{} and may only contain the characters A-Z, a-z, 0-9 and _. Only the keywords appdir,appdata, localappdata and userhome are supported for now.

TODO:

  • adjust buildscripts
  • replace ~ by @(userhome) in code

@infeo infeo added the type:feature-request New feature or request label Jun 15, 2023
@infeo infeo added this to the 1.10.0 milestone Jun 15, 2023
@infeo infeo self-assigned this Jun 15, 2023
@infeo infeo marked this pull request as draft June 15, 2023 09:02
@infeo
Copy link
Member Author

infeo commented Jun 15, 2023

After merge, issue #2853 can also be fixed.

Co-authored-by: Sebastian Stenzel <overheadhunter@users.noreply.github.com>
Copy link
Member

@overheadhunter overheadhunter left a comment

Choose a reason for hiding this comment

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

Can you add a test for the process function with various inputs and their expected outputs as parameters?

@infeo
Copy link
Member Author

infeo commented Jun 20, 2023

Can you add a test for the process function with various inputs and their expected outputs as parameters?

Done in 2c0474e


I had to circumvent a problem making the situation less clean: The logDir is specified by a property, which gets resolved at class initiatlization (due to creating a logger for Cryptomator.java). I moved the PropertiesPreprocesser::run directive now to the constructor of the Environment object and initialize it also in Cryptomator.java to make it more obvious.

Addiitonally, i changed on Windows some locations from appdata to localappdata.

@infeo infeo marked this pull request as ready for review June 20, 2023 16:12
Copy link
Member

@overheadhunter overheadhunter left a comment

Choose a reason for hiding this comment

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

I dislike the static ENV in the main class. Let's have a call to discuss alternative solutions for the initialization order problem.

* decorate Properties class
* set the system properties to decorator
* for logging setup, skip enviroment and access props over decorator
@infeo
Copy link
Member Author

infeo commented Jun 29, 2023

After some suggestions from @overheadhunter, I refactored the code.

PropertiesPreprocessor is replaced a decorator of the system Properties. getProperties() is overriden to lazily substitute variables in the requested property, if it starts with cryptomator.. The main class Cryptomator.java has now a static initalizer block, in which the default system properties are replaced by its decorator via System.setProperties().

The log configuration class now directly access the decorated properties instead of Environment, because it loaded before the fully initiallization of the main class.

@infeo infeo merged commit 067814d into develop Jun 30, 2023
6 checks passed
@infeo infeo deleted the feature/preprocess-properties branch June 30, 2023 10:38
purejava added a commit to purejava/cryptomator that referenced this pull request Jul 1, 2023
infeo added a commit to flathub/org.cryptomator.Cryptomator that referenced this pull request Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature-request New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use Windows environment variable %APPDATA% for paths to profile path
2 participants