oblac / jodd

Jodd! Lightweight. Java. Zero dependencies. Use what you like.
https://jodd.org
BSD 2-Clause "Simplified" License
4.06k stars 724 forks source link

[jodd-props] UseActiveProfilesWhenResolvingMacros not working when using "getValue(String key, String... profiles)" #146

Closed tfabien closed 10 years ago

tfabien commented 10 years ago

Following documentation example properties.

root=/app
root<foo>=/foo
data.path=${root}/data

Resolving macros using active profiles works as expected when calling setActiveProfile() and then calling getValue(String key):

props.setUseActiveProfilesWhenResolvingMacros(true);

assertEquals("/app", props.getValue("root")); // OK
assertEquals("/app/data", props.getValue("data.path")); // OK

props.setActiveProfiles("foo");
assertEquals("/foo", props.getValue("root")); // OK
assertEquals("/foo/data", props.getValue("data.path")); // OK

But resolution is incorrect when using getValue(String key, String... profiles) directly:

props.setUseActiveProfilesWhenResolvingMacros(true);

assertEquals("/app", props.getValue("root"));  // OK
assertEquals("/app/data", props.getValue("data.path"));  // OK

assertEquals("/foo", props.getValue("root", "foo")); // OK
assertEquals("/foo/data", props.getValue("data.path", "foo")); // NOT OK: org.junit.ComparisonFailure: expected:</[foo]/data> but was:</[app]/data>

Note: Present both in 3.5.2 and 3.6-RC1

igr commented 10 years ago

This is very, very good issue/question/example.

Props macro resolving (i.e. replacement of ${} values) works statically and not dynamically. Which means that replacement happens based on whats available during the parsing time. When you call getValue() with profiles, parsing is already done and macros are already resolved, so passed profile is not used for resolving macros.

When you change the active profile in the code, all resolved macros are reset, so macros get resolved on next lookup and things are working.

Thats why we have setUseActiveProfilesWhenResolvingMacros flag, you can define should we use the active profile (which is also known prior to actual parsing) or to use current profile of a property when resolving macros.

To make example work, you would need to add:

data.path<foo>=${root}/data

(or to use profiles). This property is now defined in foo profile, and it will be able resolve ${root} macro from the same profile (if exist).

Why resolving macros on parsing time? One of the starting ideas was to have all property values set, so we don't need to resolve macros on every getValue(). This would also make iteration of properties easier. We thought that profiles would be set once on start and not much during the runtime.

That being said... I must admit this is a bit... nah, not pretty. I had a problem figuring whats wrong with your example, because it's logic was simple :) Let me try to make macros resolving dynamic. It should not change the way how existing code works... much :) If we go dynamic then setUseActiveProfilesWhenResolvingMacros would have no meaning, which is also fine :) I will let you know here the results.

tfabien commented 10 years ago

Ok, seems logic looking at the code of the two methods in fact. I was just a bit confused by the results, and glad I made a unit test that revealed that side-effect of changing the way jodd-props was called in my own code :) I'll try to dig into it this too if I get some free coding-time and see if I can make a pull request

Meanwhile, maybe add a warning to the documentation, as user might expect a totally different behaviour, or even be totally unaware of the unexpected result

igr commented 10 years ago

I made changes, in the separated branch: https://github.com/oblac/jodd/tree/props-dynamic-macros

And I like it this way much better, it is much more obvious what is the value! Like I've said, your logic above is quite strait and simple, and that is fine; moreover, one fuzzy configuration rule is out. The change was not big after all.

Need to check with other users would have a problem with this change, but afaik they will not be affected.

tfabien commented 10 years ago

Very nice, just tested it and it's working great :-)

As a complementary suggestion, would it be possible, with dynamic macro resolving, to have a macro specifying/overriding the profile to be used when resolving? eg:

key1=DEFAULT
key1<foo>=FOO

# Expecting:
# - "FOO" if "foo" profile is active
# - "DEFAULT" otherwise
key2=${key1}

# Macro specifying a profile for resolve
# Expecting:
# - "FOO" whatever the active profile is
key3=${key1<foo>}

# A more complex example
# Expecting:
# - "FOO" if  "foo" profile is active
# - "FOOBAR" if  "bar" profile is active
# - "DEFAULT" otherwise
key4=${key1}
key4<bar>=${key1<foo>}BAR

# Copy operator specifying a resolve profile
[group1]
key=DEFAULT
key<foo>=FOO
[group2]
#: Expanded as 'group2.key=${group1.key<foo>}'
<= group1<foo>

I can see a few use-cases for this, although maybe a bit too specific :)

igr commented 10 years ago

Why not :) Done it: https://github.com/oblac/jodd/commit/c0759bc250e69552321f15050deeb564af1f0598

Enjoy! And thank you for the excellent examples!

igr commented 10 years ago

In fact, your latest addon with profiles in keys (${key1<foo>}) is important for backward compatibility - if someone wants to get the previous behavior, he can just set the profile in the macro name!

Great, because this gives me a green light to merge this branch to master.

tfabien commented 10 years ago

Great news then :-) Also, very impressed by your reactivity, it's a real pleasure, and I'll probably use jodd more and more on my next projects, keep up :-)

igr commented 10 years ago

Thanx for updating the above example with COPY operator, I missed that part in initial fix :)

Meh, I am just trying to give the best code, so to help people who find it useful. Feel free to try and play with Jodd :) I have started working on documentation, so soon its gonna be updated for changes in upcoming 3.6