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

-undefined- does not work for ERXObjectStoreCoordinatorPool.maxCoordinators #955

Closed
ocsocs opened this issue Jun 27, 2021 · 2 comments
Closed
Assignees

Comments

@ocsocs
Copy link

ocsocs commented Jun 27, 2021

in my Properties, there is non-zero ERXObjectStoreCoordinatorPool.maxCoordinators. For some testing, I would need to override it to null through the launch arguments, like this:

-er.extensions.ERXObjectStoreCoordinatorPool.maxCoordinators -undefined-

Nevertheless, what I am getting is

[2021-6-23 18:3:1 CEST] <main> java.lang.IllegalArgumentException: Failed to parse an integer from the value '-undefined-'.
    at er.extensions.foundation.ERXValueUtilities.IntegerValueWithDefault(ERXValueUtilities.java:202)
    at er.extensions.foundation.ERXProperties.intForKeyWithDefault(ERXProperties.java:1003)
    at er.extensions.foundation.ERXProperties.intForKey(ERXProperties.java:830)
    at er.extensions.eof.ERXObjectStoreCoordinatorPool.initialize(ERXObjectStoreCoordinatorPool.java:72)
    at er.extensions.eof.ERXObjectStoreCoordinatorPool.initializeIfNecessary(ERXObjectStoreCoordinatorPool.java:62)

I would assume this should work, given

// in ERXObjectStoreCoordinatorPool.java
    public static void initializeIfNecessary() {
    	if (ERXProperties.stringForKey("er.extensions.ERXObjectStoreCoordinatorPool.maxCoordinators") != null) {
    		ERXObjectStoreCoordinatorPool.initialize();
    	}
    }
// in ERXProperties.java
    private static String UndefinedMarker = "-undefined-";
... ...
    public static String stringForKeyWithDefault(final String s, final String defaultValue) {
...
        return stringValue == UndefinedMarker ? null : stringValue;
    }

I've even tried ERXProperties.stringForKey("er.extensions.ERXObjectStoreCoordinatorPool.maxCoordinators") in my own code, and I am getting null all right.

@paulhoadley paulhoadley self-assigned this Jun 28, 2021
@paulhoadley
Copy link
Contributor

paulhoadley commented Jun 28, 2021

As often seems to be the case with Wonder, there's quite a bit going on here.

Despite claiming elsewhere that I could reproduce your finding that stringForKey() returns null as expected if the property value is -undefined-, I actually can't: it always returns the string literal -undefined-. This is unsurprising, though, as line 950 is:

return stringValue == UndefinedMarker ? null : stringValue;

That is, the method is doing a string comparison using ==. If we changed that to use equals(), we could get stringForKeyWithDefault() to return the default value if the property value is actually -undefined-.

Next, intForKeyWithDefault() is never going to work this way, because at line 764 it calls ERXValueUtilities.IntegerValueWithDefault() with the string -undefined- as its first argument, and that method throws an IllegalArgumentException if it can't parse a number.

Finally, I'm not actually convinced we're looking at a broken feature here. It's certainly not documented, though given the historical state of Wonder documentation, that observation probably doesn't carry much weight. But:

  • UndefinedMarker is a private string, used only in this class;
  • the string literal -undefined- occurs nowhere else in the project, including any sample apps;
  • it's not tested in ERXTest; and
  • perhaps most convincingly, the way UndefinedMarker is used in ERXProperties makes it look like it's just an internal token to put in the cache when a property is undefined, meaning it doesn't need to be retrieved from system properties again.

None of these observations is particularly convincing alone, but overall I do get the impression that being able to put -undefined- in your Properties file (or as a command-line override) simply isn't an intended feature. It's not documented, I don't see a mention of it on the mailing list until last week, and, as you note, it doesn't work. My bet would be that UndefinedMarker was intended only as a sentinel value in the ERXProperties cache.

@ocsocs
Copy link
Author

ocsocs commented Jun 28, 2021

Oh, my bad. Using Groovy for years, I've completely forgot that in pure Java, == does an identity compare. I do apologise. Given that, it makes perfect sense (but for the side and really unimportant question why my — and, it seems, your first — test really returned a null).

Aside of that, I would recommend to replace == by equals here (perhaps with a different string, like e.g. “er.extensions.foundation.ERXProperties.undefined” or so to decrease the probability somebody might really need the value for a property), along with a similar support for the other accessors like allProperties etc., for, far as I can say, there is currently no way to override a non-null value in Properties by a null in the launch arguments; and I would argue that sometimes could get handy.

(Another solution for my particular case would be if ERXObjectStoreCoordinatorPool.maxCoordinators switched off on 0 just as on a null, but the above would be considerably more flexible.)

Thanks!

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

No branches or pull requests

2 participants