fix: resource references with property resolving

[maxandersen]

minimal draft fix for #2152

this feels almost too easy...but would this work ?

[maxandersen]

might make a PropertyResourceResolver(ResourceResolver) to separate concerns.

[maxandersen]

Things to check before approving: that we don't cache based on originalresourceref keys.

[quintesse]

That's too easy, yes. There's no guarantee, nor reason, to assume that the CombinedResourceResolver is always the "first" (or "top") resolver to be called. Right now it is, but that's not required. So if that changes for some reason, perhaps even just for a single specific use-case in the future, this wouldn't work and we'd have to add property replacers to other resolvers as well.

So given the fact that you do nothing more than replace properties on the input I don't see why this can't be done before that input gets passed to the resolver. It just seems to me we're mixing responsibilities here.

Edit: Personally I'd just look to all the places where we call the resolver and see if we can't put the property replacement there.

If that's somehow too much work I'd suggest a "fake" resolver which does nothing more than replace the properties before passing on the work to a wrapper resolver. ANd then make sure that we always return that one as the "top" resolver.

[maxandersen]

Edit: Personally I'd just look to all the places where we call the resolver and see if we can't put the property replacement there.

that made me go look and i located resolveChecked which is the "root" of all the CombinedResource resolvers I could find.

I updated it to do the property replacement - and since its in the project builder it can get and use the buildContext which has user specifcied propertiers.

So far thats nice; but I see two issues:

1. to my surprise we actually put system properties into this build context - not sure we should allow that ..or at least clean out a few keys like user.home...but not sure
2. it is also used for source ref resolution...meaning //SOURCE ${user.home}/somewhere suddenly becomes accessible.

...writing this out I think i'm going to update this to take a parameter where the property replacement will be enabled and only do that for the "root" build/alias call.

[maxandersen]

note: just realizing prperty replacement as is implemented now also support env. for environment variable lookup so definitly dont want it available everywhere

[maxandersen]

well heck - we already support property replacement in all Tags (incl. files) I just realize. look for "replaceProperties"

we avoid/reduce the impact on //FILES by files not allowing absolute paths only relative ones.

[maxandersen]

so yeah "//FILES myfile=../../../.bash_profile" works ...but so does Path.of("../../../.bash_profile") in a main method of any java code ...so yeah; not any worse or better risk than what running anything from online apps does.

Hence adding verification/checksum checks is the more important part.

[maxandersen]

@quintesse assuming tests passes I think this is good to go.

[quintesse]

This is already merged, but original_resource? Wtf? Did you use AI to generate this? 😁

[maxandersen]

This is what was already there afaik. Unless you identify as AI it was human :)

[quintesse]

Don't gaslight me, this code is new, the diff shows you just added it :-)

I would never ever write a (Java) variable name with an underscore, the very idea gives me shivers 😁

[maxandersen]

The only answer must be that I'm an AI then :)

[quintesse]

Either put proper docs or remove the @params :-)

[quintesse]

Logging this is good, but I think the wording could be improved. In this case if there are no properties to be replaced you won't get a "resolving ..." message, which is wrong because we definitely are resolving. So I'd say that we'd either have to always log something or change the wording to something like "Applying properties" instead of "Resolving".

[maxandersen]

but something gets logged everytime - its just only when applying properties we put the "-> "

I did that to avoid it to say 99% of the time "Resolving resource ref: abc -> abc"

but if you can think of a better one do open pr.

[quintesse]

Well, that's why I said to simply change the text. Because it's not resolving, it's "applying properties to resource ref" or something like that. Just to avoid any confusion that that line suggests that resolving has been done/performed.

[maxandersen]

But resolve xyz is what we wrote before applying any properties. So are you saying the old one was wrong?

[quintesse]

Maybe I'm just misunderstanding things. Doesn't matter. Not important,