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

Add a "stacked" composite index #142

Closed
Ladicek opened this issue Sep 3, 2021 · 8 comments · Fixed by #346
Closed

Add a "stacked" composite index #142

Ladicek opened this issue Sep 3, 2021 · 8 comments · Fixed by #346
Assignees
Milestone

Comments

@Ladicek
Copy link
Contributor

Ladicek commented Sep 3, 2021

Jandex includes CompositeIndex, which combines multiple indices that don't overlap. When the indices do overlap, CompositeIndex will return duplicates.

We could add an OverlayIndex which would overlay one index on top of another (or perhaps a stack of indices, not just two). Some methods on IndexView are perhaps hard to define, but the most common ones should be possible.

Something like this:

public class OverlayIndex implements IndexView {
    private final List<IndexView> indices;

    Collection<ClassInfo> getKnownClasses() {
        // call `getKnownClasses` on each index, in order, keep track of already seen class names
        // and skip a class if its name was already seen
    }

    ClassInfo getClassByName(DotName className) {
        // iterate through `indices` and return the first found
    }

    Collection<ClassInfo> getKnownDirectSubclasses(DotName className) {
        // don't know here? perhaps call `getKnownDirectSubclasses` on each index, in order,
        // keep track of already seen class names and skip a class if its name was already seen
    }

    Collection<ClassInfo> getAllKnownSubclasses(DotName className) {
        // don't know here? perhaps call `getAllKnownSubclasses` on each index, in order,
        // keep track of already seen class names and skip a class if its name was already seen
    }

    Collection<ClassInfo> getKnownDirectImplementors(DotName className) {
        // don't know here? perhaps call `getKnownDirectImplementors` on each index, in order,
        // keep track of already seen class names and skip a class if its name was already seen
    }

    Collection<ClassInfo> getAllKnownImplementors(final DotName interfaceName) {
        // don't know here? perhaps call `getAllKnownImplementors` on each index, in order,
        // keep track of already seen class names and skip a class if its name was already seen
    }

    Collection<AnnotationInstance> getAnnotations(DotName annotationName) {
        // call `getAnnotations` on each index, keep track of already seen `AnnotationTarget`s
        // and skip an `AnnotationTarget` if it was already seen
        //
        // note that `AnnotationTarget` doesn't have well defined equality, so this may be harder than it seems
    }

    Collection<AnnotationInstance> getAnnotationsWithRepeatable(DotName annotationName, IndexView index) {
        // same as getAnnotations
    }

    Collection<ModuleInfo> getKnownModules() {
        // same as `getKnownClasses` probably?
    }

    ModuleInfo getModuleByName(DotName moduleName) {
        // same as `getClassByName` probably?
    }

    Collection<ClassInfo> getKnownUsers(DotName className) {
        // call `getKnownUsers` on each index, in order, keep track of already seen class names
        // and skip a class if its name was already seen
    }
}
@Ladicek
Copy link
Contributor Author

Ladicek commented Sep 3, 2021

An alternative would be to modify CompositeIndex to do this. If the indices don't overlap, the behavior wouldn't change. If they do overlap, I'd argue this behavior is more useful than current CompositeIndex behavior.

@Ladicek
Copy link
Contributor Author

Ladicek commented Sep 3, 2021

And it turns out that CompositeIndex.getKnownUsers already implement the overlay behavior described above.

@FroMage
Copy link
Contributor

FroMage commented Nov 2, 2021

Arc and RESTEasy Reactive already have annotation transformers to achieve this, you should take a look at the io.quarkus.arc.processor.AnnotationsTransformer API. If we can remove those two (copied) APIs from Quarkus and use a Jandex equivalent, that'll be much better.

@Ladicek
Copy link
Contributor Author

Ladicek commented Nov 2, 2021

At this moment, I know the callback-based annotation transformation in Quarkus by heart, and I can confidently say that 1. it is a bad fit for Jandex, and 2. it doesn't solve all use cases people have.

Ad 1., it would either require massive refactoring in Jandex to make the Jandex public API return the transformed annotations (and would violate the immutability promise), or Jandex would be just a carrier of a completely independent annotation overlay mechanism. Having a completely independent annotation overlay mechanism isn't necessarily a bad thing, but I'm going to claim that Jandex shouldn't be its home. Because...

Ad 2., see e.g. #117. Annotations are not the only thing people want to transform.

I'd like to have a proper overlay mechanism in Jandex for sure -- but it's not going to be a copy of what Quarkus does.

@mkouba
Copy link
Contributor

mkouba commented Nov 2, 2021

I also think that the annotation transformers API from Quarkus should not be incorporated into Jandex, or at least not incorporated into the Index class hierarchy. It was designed as a thin layer built on top the AnnotationTarget where:

  1. The transformations are performed lazily, i.e. the first time a client requests the annotations of a specific target.
  2. A client should be aware of the fact that the result is possibly a transformed view of the real index, i.e. the client basically calls a simple function that accepts an AnnotationTarget and returns the Collection<AnnotationInstance>.

If we can remove those two (copied) APIs from Quarkus and use a Jandex equivalent, that'll be much better.

Well, I think that it was really a bad idea to copy the ArC API into the RESTEasy Reactive project. Think about maintenance and API evolution... The API should have been probably extracted into a shared library and into the quarkus-core.

Ad 2., see e.g. #117. Annotations are not the only thing people want to transform.

All I can say is that I don't believe it's a good idea to allow users to do something like this. It's just too easy to shoot yourself in the leg. We've seen this many times in the CDI API where people got too creative when supplying AnnotatedTypes and the result was the simplified AnnotatedTypeConfigurator API which only allows users to modify annotations ;-).

@Ladicek
Copy link
Contributor Author

Ladicek commented Nov 3, 2021

FTR, I've got one more thing I would like to add to Jandex that should help a lot with overlaying -- the notion of equivalence.

Jandex classes often don't have equals at all, or have equals that isn't necessarily useful. That makes perfect sense, because if you want to overlay a ClassInfo for class com.example.MyClass with a different set of annotations, you'd have two ClassInfo objects for the same class. They should not be considered equal, but they should be considered equivalent.

Let me file an issue for that.

EDIT: #158

@Ladicek
Copy link
Contributor Author

Ladicek commented May 18, 2022

Note to self: vast majority of use cases I've seen so far only need transforming annotations (including the use cases behind #117). There's one more interesting use case in quarkusio/quarkus#19883 that is more complex.

@Ladicek Ladicek changed the title Add an OverlayIndex Add a "stacked" composite index Sep 8, 2022
@Ladicek
Copy link
Contributor Author

Ladicek commented Sep 8, 2022

I renamed this issue, because it suggested a concrete name: OverlayIndex. I plan to use the term "overlay" for an annotation overlay, and I don't want these 2 different features to share the same distinctive word.

Instead, I'm thinking this could be called a StackedCompositeIndex or just StackedIndex or something like that (or even CompositeIndex.stacked()). That name better highlights how the multiple indices are queried and how possible duplicates are resolved.

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

Successfully merging a pull request may close this issue.

3 participants