Reading configuration from config files by rPraml · Pull Request #1722 · mozilla/rhino

rPraml · 2024-11-08T09:10:18Z

This could be a first draft for #1720

gbrail · 2024-11-13T07:27:18Z

I think that this approach makes sense in that it lets configuration come from the environment, system properties, file, or classpath. I think that's a good thing and that we should follow this pattern.

For actually checking the setting of various flags, however, I think that the config class should parse all this input and set boolean (or enum) values that can be checked directly, rather than relying on a hash lookup. I think that can give us two advantages:

It keeps all the logic about naming and renaming things in one place
It will be faster, since many of these properties, like the various debug flags, will be checked many millions of times potentially while Rhino runs, and all those hash table lookups will dominate performance pretty soon.

rPraml · 2024-11-20T19:19:21Z

I think I can implement this next week or so...

gbrail · 2024-11-20T21:54:09Z

If you're looking for the kinds of debug flags I'd like to potentially replace with a real debugging mechanism, I would look at these:

rhino/rhino/src/main/java/org/mozilla/javascript/Token.java

Line 27 in 3e899f6

public static final boolean printTrees = false;

rhino/rhino/src/main/java/org/mozilla/classfile/ClassFileWriter.java

Line 4476 in 3e899f6

private static final boolean DEBUGSTACK = false;

rhino/rhino/src/main/java/org/mozilla/javascript/optimizer/DefaultLinker.java

Line 25 in 3e899f6

static final boolean DEBUG;

And as for feature flags, the first one IMO should be for the reflect and proxy support that's currently languishing in a PR by @rbri

rPraml · 2024-12-13T11:10:43Z

Unfortunately, I didn't have as much time as I thought, but I wanted to update the draft before Christmas of how I want to do it now.

The idea is, to check all sources (classpath, configfile, system-properties, env - the last one wins) for the settings and parse them immediately in the correct datatype. When a variable is bound to the property rhino.debugLinker for example. I try to check also the upper-camel RHINO_DEBUG_LINKER version.

@gbrail I hope, this goes in the right direction... Unfortunately (or fortunately) I won't be able to continue until next year due to vacation. :)

rPraml · 2025-01-17T14:27:55Z

So, I found some time to update this PR.

What do we have now

The RhinoProperties class is a container of serveral config maps, that are loaded from different location:

looks for rhino.config & rhino-test.config in several locations. (CHECKME: do we need rhino-test.config - similar to logback-test.xml?)
add system-env & system-properties
or optional: Implement RhinoConfigLoader service with your own loading strategy

When checking for a value, RhinoProperties tries all configs in loaded order in exact spelling and camel case-conversion. (e.g rhino.printICode and RHINO_PRINT_ICODE - you can use both writings in any config source. (CHECKME: This camel case conversion is required for system-env. Do we want it for other config sources, too? It theoretically slows down the parse time, as several permutations are checked, but every config is only parsed once, if assigned to a static field, so this should not have a big impact)

There is a helper class RhinoConfig which should be used in most cases. It adds some helper methods to get properties as String, Integer or Enum.

For actually checking the setting of various flags, however, I think that the config class should parse all this input and set boolean (or enum) values that can be checked directly, rather than relying on a hash lookup. I think that can give us two advantages:

It keeps all the logic about naming and renaming things in one place

It will be faster, since many of these properties, like the various debug flags, will be checked many millions of times potentially while Rhino runs, and all those hash table lookups will dominate performance pretty soon.

I tried that, but did not like the idea to add configs from different packages in one class:

This is not optimal, if we want to split rhino in more modules. I can imagine to have one config class per module or package (please give feedback)
This should be no problem in the current PR. Each config is assigned to a static constant, so it is read only one time.

Other findings, I identified:

I tried to attach the current config to the current context (so to have one context with PRINT_ICODE and an other without), but this would require to pass the context/config object down to Token etc. So I think, we have to accept, that we have only ONE config per classloader and every rhino context uses the same settings. It is important in the future, to decide, if some config values should be global or at context level.
I read all properties (also system-properties & env). So someone that have access to this class will effectively bypass the (deprecated) security-manager

it would be good, if I get some feedback, if this goes in a way we want.

@gbrail FYI

rPraml · 2025-01-23T18:33:33Z

@gbrail ping... Did you find already time to give some feedback?
Thanks

gbrail

I think this is good progress, and minus the attached minor comments, I think that this is the right implementation.

I also want to know how we can test this -- there are a lot of options with properties files, system properties, environment variables, and a service loader.

I really think that we should go forward with this, but only if we can put in some way to exercise more of this codebase in our test suite.

gbrail · 2025-01-24T03:34:01Z

+        if (ret != null) {
+            Class<T> enumType = (Class<T>) defaultValue.getClass();
+            // We assume, that enums all are in UPPERCASES
+            return Enum.valueOf(enumType, ret.toString().toUpperCase(Locale.ROOT));


Should we catch "IllegalArgumentException" in case the input value is not valid and return the default in that case?

gbrail · 2025-01-24T03:35:02Z

+        if (ret instanceof Boolean) {
+            return (Boolean) ret;
+        } else {
+            return "1".equals(ret) || "true".equals(ret);


I wonder if you'd consider numeric values other than 1 as "true," and doing a case-insensitive comparison to "true"

gbrail · 2025-01-24T03:36:38Z

+            props.load(new InputStreamReader(in, StandardCharsets.UTF_8));
+            addConfig(props);
+        } catch (IOException e) {
+            System.err.println(


I don't love that we "println" anywhere, but I don't see that we have a choice in this case. But if it happens, it'll be buried in someone's big log file and they won't know what it means -- should we consider adding "Rhino:" or something to identify this message so that people can know why it's appearing?

gbrail · 2025-01-24T03:37:04Z

+            props.load(new InputStreamReader(in, StandardCharsets.UTF_8));
+            addConfig(props);
+        } catch (IOException e) {
+            System.err.println(


Same here -- we should add "Rhino" or something to this.

rPraml

From my perspective, this is ready for merge.

I would like to add some documentation to https://rhino.github.io
Can anyone tell me, how? (i did not find the repo/instruction how to contribute)

@gbrail FYI

rPraml · 2025-02-03T12:05:50Z

    public static void main(String args[]) {
        try {
-            if (Boolean.getBoolean("rhino.use_java_policy_security")) {
+            if (RhinoConfig.get("rhino.use_java_policy_security", false)) {


Q: Should we change this to

Suggested change

if (RhinoConfig.get("rhino.use_java_policy_security", false)) {

if (RhinoConfig.get("rhino.useJavaPolicySecurity", false)) {

A: Probably not, as this will break other things!
And JavaPolicySecurity has to be removed anyway in the future (see #1353)

rPraml · 2025-02-03T12:08:08Z

+    private static String[] CONFIG_FILES = {"rhino.config", "rhino-test.config"};
+    private List<Map<?, ?>> configs = new ArrayList<>();
+    // this allows debugging via system property "rhino.debugConfig"
+    private final boolean debug = Boolean.getBoolean("rhino.debugConfig");


I've added this flag in a separate commit in this PR. It will help to trouble-shoot any loading issues. So I find this useful and would be happy, if we can keep this

rPraml · 2025-02-03T14:01:28Z

+ * this module will also use this loader. <br>
+ * The loader does NOT load the properties from the default locations as mentioned in the
+ * documentation. So, do not wonder, if setting config values in 'rhino.config' or
+ * 'rhino-test.config' do not take effect. Instead use `rhino-config-for-this-module.config`.


Testing is a bit difficult, so I tried to cover all aspects in this tests

rPraml · 2025-02-03T14:02:34Z

+        if (k.startsWith("rhino.")) {
+            systemProperty k, v
+        }
+    }


I copy all rhino system properties to the sub process. So you can pass properties with -Drhino.xyz from a gradle buld

gbrail · 2025-02-04T03:03:45Z

I think this PR is looking very good, but why do we have this file and the other .config files in the top-level directory? I suppose that we need one just to test the file reading part of the codebase, but since they won't be used by the runtime or tests otherwise, do we really need all of them? That's really my only concern with this PR at this point. Thanks!

Hello @gbrail

I also want to know how we can test this -- there are a lot of options with properties files, system properties, environment variables, and a service loader.

This is the problem, why I need so much test files. Let me explain for what they are.

rhino/rhino-config-for-this-module.config
The file rhino-config-for-this-module.config is used internally by RhinoPropertiesTest.Loader and is the only config-file loaded in this module for tests.
This file, along with rhino/rhino-test.config, serves as a placeholder for where properties should be placed.
For example, if there is a new experimental feature 'xyz' that can be enabled with rhino.experimental.xyz=true, it can be enabled in rhino-config-for-this-module.config or rhino-test.config, so that is available for test cases.
As these files are not included in the build jars, the feature stays disabled, unless someone enables it.

rhino/rhino-test.config
rhino/src/test/resources/rhino-testcase.config
rhino/src/test/resources/rhino.config
These files are used for various tests

tests/rhino-test.config
This file is mainly for documentation/example (there are only comments) and can be used to enable features for testing.

I see the problem, that this is now a bit confusing, but the files are required for the (complex) tests.

I see the following options:

keep everything as it is (this will confuse people)

create one (or more) rhino-config-tests module, to test various config ways (with/without ServiceLoader)

remove/rewrite the complex test cases (especially the one with serviceLoader), which means, that the one or other properties-file will be obsolete.

I will prefer option 3 ;)

rPraml

I've refactored the tests a bit. There are currently two top level rhino-test.config files, which are mainly used for documentation (and one test)

I can delete these files, if you want.
Should I add the documentation to https://github.com/rhino/rhino.github.io/blob/main/docs/_docs/configuration.md or is there a better/preferred place?

rPraml · 2025-02-05T09:04:56Z

+#
+# Please keep this values for RhinoPropertiesTest
+test.file.rhino-test-config.loaded=true
+test.config.bar=value4-mod


This file is a placeholder/for documentation and is also used to demonstrate, that a top-level file can override values in a classpath file.
Should I delete it (and the related test)?

rPraml · 2025-02-05T09:07:18Z

+# rhino.printTrees=false
+# rhino.printICode=false
+# rhino.debugLinker=false
+# rhino.use_java_policy_security=false


This file is really only a placeholder/for documentation. Should I delete it?

gbrail · 2025-02-08T01:58:16Z

Sorry, lost track of where in the thread to comment.

I think it's fine to leave "rhino.test-config" files in the "rhino" and "tests" modules. Could you please add a comment explaining why they're there, and say that it's an example of how to use these files, or that people could uncomment them in order to do further testing? Once you do that I think that we should merge this.

As for the docs, I think it'd be great if you updated the docs on "rhino.io". @p-bakker owns that repo and I'm sure he'd accept a PR.

Spotbugs

rPraml · 2025-02-10T19:24:40Z

Hello @gbrail I rebased this PR and made these changes: 28277a4

gbrail · 2025-02-11T06:20:57Z

Do you have time to look at the two warnings that come from ErrorProne?

/home/runner/work/rhino/rhino/rhino/src/main/java/org/mozilla/javascript/config/RhinoProperties.java:173: warning: [AnnotateFormatMethod] This method uses a pair of parameters as a format string and its arguments, but the enclosing method wasn't annotated @FormatMethod. Doing so gives compile-time rather than run-time protection against malformed format strings.
private void logDebug(String msg, Object... args) {
^
(see https://errorprone.info/bugpattern/AnnotateFormatMethod)
/home/runner/work/rhino/rhino/rhino/src/main/java/org/mozilla/javascript/config/RhinoProperties.java:180: warning: [AnnotateFormatMethod] This method uses a pair of parameters as a format string and its arguments, but the enclosing method wasn't annotated @FormatMethod. Doing so gives compile-time rather than run-time protection against malformed format strings.
private void logError(String msg, Object... args) {
^
(see https://errorprone.info/bugpattern/AnnotateFormatMethod)

I can look at them myself later if you'd like, but I'm trying to keep the code clean of those warnings so I'd like to see if you have a chance to fix the underlying issue first. Thanks!

rPraml · 2025-02-11T06:39:12Z

Hello @gbrail,

tried to fix this, but com.google.errorprone.annotations.FormatMethod is not on the classpath.
I see two options:

add errorprone dependency (maybe in an extra PR?)
add @SuppressWarnings("AnnotateFormatMethod") in the meantime.

I can look at them myself later if you'd like, but I'm trying to keep the code clean of those warnings so I'd like to see if you have a chance to fix the underlying issue first. Thanks!

In this case I would accept the offer and leave the fix to you ;)

Roland

gbrail · 2025-02-11T06:48:42Z

OK -- I'll dig deeper into ErrorProne later in the week and fix it, and will merge what you already have soon. Thanks!

…

On Mon, Feb 10, 2025 at 10:39 PM Roland Praml ***@***.***> wrote: Hello @gbrail <https://github.com/gbrail>, tried to fix this, but com.google.errorprone.annotations.FormatMethod is not on the classpath. I see two options: - add errorprone dependency (maybe in an extra PR?) - add @SuppressWarnings("AnnotateFormatMethod") in the meantime. I can look at them myself later if you'd like, but I'm trying to keep the code clean of those warnings so I'd like to see if you have a chance to fix the underlying issue first. Thanks! In this case I would accept the offer and leave the fix to you ;) Roland — Reply to this email directly, view it on GitHub <#1722 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAD7I23G2OY7CFXQDHET3XT2PGLKLAVCNFSM6AAAAABRNBCUF2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMNBZHEZTQNJUGM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

gbrail · 2025-02-12T03:23:08Z

Thanks for your work on this and I'll look at the warning soon.

Documentation for this PR mozilla/rhino#1722

rPraml marked this pull request as draft November 8, 2024 09:10

rPraml force-pushed the rhino-config branch from dd2bf28 to 659ebc5 Compare December 13, 2024 10:48

rPraml force-pushed the rhino-config branch from 2d3e39d to ffff7d0 Compare January 17, 2025 13:01

gbrail reviewed Jan 24, 2025

View reviewed changes

rPraml force-pushed the rhino-config branch 2 times, most recently from c09c606 to 5799ee2 Compare February 3, 2025 11:08

rPraml commented Feb 3, 2025

View reviewed changes

rPraml changed the title ~~Draft for reading configuration from config files~~ Reading configuration from config files Feb 3, 2025

rPraml marked this pull request as ready for review February 3, 2025 14:06

gbrail reviewed Feb 4, 2025

View reviewed changes

rPraml force-pushed the rhino-config branch from 0744b39 to 0713a5f Compare February 5, 2025 09:05

rPraml commented Feb 5, 2025

View reviewed changes

rPraml added 6 commits February 10, 2025 15:17

Add RhinoConfig parser/loader

c2e4c0c

Spotbugs

Add testcases for RhinoConfig

90711c4

Read config values from RhinoConfig

536ca19

RhinoProperties has debug flag now

06084a5

Better debug logging

dbf2e0a

Simplified test and removed some config files

8a6c7e5

rPraml force-pushed the rhino-config branch from 0713a5f to ca40fd6 Compare February 10, 2025 16:11

Better documentation

28277a4

rPraml force-pushed the rhino-config branch from ca40fd6 to 28277a4 Compare February 10, 2025 17:21

gbrail merged commit 34d9bec into mozilla:master Feb 12, 2025

rPraml added a commit to FOCONIS/rhino.github.io that referenced this pull request Feb 13, 2025

Provide documentation for reading config files

3f88c5b

Documentation for this PR mozilla/rhino#1722

rPraml mentioned this pull request Feb 13, 2025

Provide documentation for reading config files rhino/rhino.github.io#4

Open

rPraml mentioned this pull request Sep 16, 2025

Improve debug output on class file writing. #2090

Merged

	if (RhinoConfig.get("rhino.use_java_policy_security", false)) {
	if (RhinoConfig.get("rhino.useJavaPolicySecurity", false)) {

Conversation

rPraml commented Nov 8, 2024

Uh oh!

gbrail commented Nov 13, 2024

Uh oh!

rPraml commented Nov 20, 2024

Uh oh!

gbrail commented Nov 20, 2024

Uh oh!

rPraml commented Dec 13, 2024

Uh oh!

rPraml commented Jan 17, 2025

Uh oh!

rPraml commented Jan 23, 2025

Uh oh!

gbrail left a comment

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!

rPraml left a comment

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!

rPraml left a comment

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!

gbrail commented Feb 8, 2025

Uh oh!

rPraml commented Feb 10, 2025

Uh oh!

gbrail commented Feb 11, 2025

Uh oh!

rPraml commented Feb 11, 2025

Uh oh!

gbrail commented Feb 11, 2025 via email

Uh oh!

gbrail commented Feb 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants