Qute - Ignore classes defined in the unnamed package in BeanArchives.index

[michalvavrik]

fixes: #26524

When TypeInfos.create is looking for a parameter declaration type, it checks a couple of options and fallbacks to a prefix java.lang. The Qute reference guide contains an example of parameter declaration using only {@String foo} without java.lang, thus when user follows the example, he should not be warned. That is fixed by ignoring classes in the unnamed package.

[mkouba]

I don't think this is a good solution. Note that the index wrapper can be used concurrently.

I wonder if we should get rid of the warning instead, and use e.g. the debug level...

@manovotn WDYT?

[manovotn]

@mkouba hm, I think the warning in general has its value.
How about we don't attempt to index a class that has no package information? Classes with no packages have been bad practice for years.
Or, at least, we can detect this and first try to prepend java.lang.util and find such class and failing that fallback to attempted loading of such class?

[michalvavrik]

That's great idea, (though here it's just java.lang.), however there is no obligation to have a Java class in the package (I realize it won't ever happen in real use cases, maybe some tests), so it should be at least mentioned in the docs. I can do it if you want (if not, please ping me so I can close the PR)

[michalvavrik]

What about introducing whitelist for selected classes in java.lang that are likely to be used this way (String, wrapper, ...)?

I know my original solution is clumsy, but it's pretty easy to work around concurrency as only non-immutable thing there is log level, so adding copy constructor (creating a new IndexWrapper instance) would fix that issue... or am I wrong?

[manovotn]

I'll let @mkouba decide, I am just tossing ideas around :-)

[mkouba]

Classes with no packages have been bad practice for years.

Bad practice and discouraged but still legal...

Or, at least, we can detect this and first try to prepend java.lang.util and find such class and failing that fallback to attempted loading of such class?

I don't think we can try the prefixed version first because it's legal e.g. to have a class String (with no package) and this class should take precedence.

I'd prefer to ignore the classes with the default package inside the BeanArchives.index(), i.e. something similar to what we do with primitive types and arrays. If you think about it then it can't be reliable anyway...

[manovotn]

Or, at least, we can detect this and first try to prepend java.lang.util and find such class and failing that fallback to attempted loading of such class?

I don't think we can try the prefixed version first because it's legal e.g. to have a class String (with no package) and this class should take precedence.

I'd prefer to ignore the classes with the default package inside the BeanArchives.index(), i.e. something similar to what we do with primitive types and arrays. If you think about it then it can't be reliable anyway...

The two approaches seem similar in a way since you try to avoid indexing no-package classes.
Your suggestion sounds good, so +1

[manovotn]

Martin is currently out of the office, I'll review this today.

[manovotn]

@michalvavrik added two comments, if you don't have time, I can update the PR as well, just let me know.

[michalvavrik]

Thank you for the review

[manovotn]

LGTM
Thanks for the PR @michalvavrik!

[Ladicek]

I know the subject of this PR mentions Qute, but the change here basically means that classes in the unnamed package can't ever be beans, if I understand the code change correctly? I wonder how that works with JBang scripting, which by convention uses the unnamed package (judging from https://quarkus.io/guides/scripting).

[manovotn]

Hmm, I have no idea nor did I know JBang uses unnamed package.
@maxandersen might know?

[maxandersen]

Yes, we don't require use of package before you actually need it.

Why is it we would not allow use of default packaged beans in Qute?
Sure - it might not be showing up in large enterprise apps, but why ban it when its perfectly valid java and for scripts/templating is just pure noise to type out?

[Ladicek]

I'd like to reiterate -- this PR, albeit mentioning Qute in the subject, actually changes a core piece of ArC. This has a potential to affect much more than just Qute, but I'll also confess right away that I didn't really investigate what the impact actually is.

EDIT: okay, just looking at the usages of the changed code and this seems pretty safe. Sorry for the false alarm! (Though I'm not exactly sure how the "additional classes" in ArC's IndexWrapper are supposed to be used and why would user classes actually end up there...)

[manovotn]

"additional classes" in ArC's IndexWrapper are supposed to be used and why would user classes actually end up there...)

@Ladicek If memory serves, those were deps from 3rd party JARs that you didn't initially index or marked for indexation in any way.
During bean discovery, it can still happen that some bean has a type coming from such JAR in which case you need to additionally index that.

[mkouba]

"additional classes" in ArC's IndexWrapper are supposed to be used and why would user classes actually end up there...)

@Ladicek If memory serves, those were deps from 3rd party JARs that you didn't initially index or marked for indexation in any way. During bean discovery, it can still happen that some bean has a type coming from such JAR in which case you need to additionally index that.

Originally the IndexWrapper was used to index JDK classes that are never part of the index and still can be used e.g. as a bean type. But then we realized that the same approach must be used e.g. if a bean type comes from an unindexed 3rd party library (as Matej mentioned).

[Ladicek]

OK, thanks! In that case, with this PR, bean types coming from unindexed 3rd party library can't be in the unnamed package, right? :-) Or something like that. Probably not a big deal, but it's a little strange.

[mkouba]

OK, thanks! In that case, with this PR, bean types coming from unindexed 3rd party library can't be in the unnamed package, right? :-) Or something like that. Probably not a big deal, but it's a little strange.

I believe that it wouldn't work anyway... and a 3rd party library with classes in the unnamed package is IMO really bad idea.

[maxandersen]

So default package is ok if it's in the jar the main class is in
Or how does it work ?

[mkouba]

So default package is ok if it's in the jar the main class is in

Yes, it should be fine if it's in the application sources root, i.e. src/main/java in your project. Because the root is always indexed.