Skip to content
This repository has been archived by the owner on Feb 23, 2023. It is now read-only.

AOT plugin does not recognize static inner classes #671

Closed
odrotbohm opened this issue Mar 26, 2021 · 17 comments
Closed

AOT plugin does not recognize static inner classes #671

odrotbohm opened this issue Mar 26, 2021 · 17 comments
Assignees
Labels
type: bug A general bug
Milestone

Comments

@odrotbohm
Copy link

In this arrangement, the static inner class is not ending up in the spring.components file:

@Entity
class MyEntity {

  @Embeddable
  static class MyEmbeddable {}
}

Moving the MyEmbeddable type into a dedicated file makes it appear.

@sdeleuze sdeleuze added this to the 0.9.2 milestone Mar 26, 2021
@sdeleuze sdeleuze added the type: bug A general bug label Mar 26, 2021
@sdeleuze
Copy link
Contributor

Do you mean this is a pattern recognized by the regular spring-context-indexer AP when added as a dependency but not when using Spring AOT + Spring Native?

@odrotbohm
Copy link
Author

I haven't yet verified which of the components is the root cause here. Let me try the context indexer standalone.

@odrotbohm
Copy link
Author

There's some interesting observations, I think: In general, it looks like the AOT plugin skips the creation of a spring.components on its own. That's unfortunate, as apparently the algorithms for the creation of that file differ, i.e. the plain output of spring-context-indexer is different than from the AOT plugin. I assume that's due to SCI operating on the sole compile result and thus does not see annotations potentially added through post-compile byte-code manipulation adding annotations. The AOT plugin seems to see those, but not on nested classes.

@sdeleuze
Copy link
Contributor

it looks like the AOT plugin skips the creation of a spring.components on its own

Isn't it created in for example target/generated-sources/spring-aot/src/main/resources/META-INF/spring.components when using Maven?

We should ideally use the same algorithm but due to the AP nature of spring-context-indexer we have to mimic it for now in Spring AOT, even if not ideal, if I remember what @aclement said to me some times ago.

@odrotbohm
Copy link
Author

Actually, I prefer what Spring AOT does. Mostly because it's running later than the APT based content indexer and thus can see the class files after a post-processing step (ByteBuddy based adding of annotations in my case). I wonder if Spring AOT should just always create a spring.components on its own as you might end up with an incomplete listing of classes if you by accident use the spring-context-indexer and miss half the files that Spring AOT would actually include.

@odrotbohm
Copy link
Author

For the original request, it looks like the creation only considers nested classes if they are Spring components, but not any other stereotypes like JPA entities, embeddables etc.

@aclement
Copy link
Contributor

few things to comment on. Early versions of synthesizing spring.components were being conservative, its a new idea - hence not treading on the toes of an existing file. But yes, AOT synthesis may catch more things resulting in a more complete list, especially in a system using spring libraries that introduce their own components (like spring cloud and its controllers) which were not built with the indexer. (the current indexer - deliberately - only catches things in the source it is compiled with).

Sebastien and I have talked about turning it ON always. But I honestly am not sure who wins the race condition between having one already existing in your current project and the one Spring Native would generate. It needs checking. If there is an issue, I suppose a temporary substitution on the existing indexer to ask "is spring native around? If so I won't do anything" would avoid that because spring.components is never manually maintained.

We can easily extend the model to catch other things, if you can describe to me what they are. That code you linked to Ollie is separate to the code that does the synthesis. And again, written in a way to 'get something going', definitely open to improve for covering more scenarios.

@odrotbohm
Copy link
Author

Great feedback, Andy, thanks. The main difference I found was @Embeddables not being written by spring-context-indexer but implicitly kept around by the AOT indexer as it just creates entries for everything javax.…. Still the AOT indexer didn't find the inner class @Embeddable as it apparently doesn't process the inner classes unless they're Spring components. I thought the code I linked to was the actual handling of the classes found in the first pass.

In the end I'd like to be able to keep @Embeddable classes as inner classes and have them shop up in the index as well. The actual effect of them missing is Hibernate complaining about them not carrying identifier properties as it doesn't recognize them as embeddables. They are, however, identifier implementations in my case.

I really like that the AOT indexer produces a more complete set of components as – as mentioned above – it allows me to add the annotations it's looking for through byte code manipulation, so that the original code can stay free of technical annotations but the code would still be treated correctly by the infrastructure.

I was kind of hoping / assuming that the AOT functionality would – at some point in the future – end up in the Spring Boot build plugin anyway and spring-context-indexer deprecated effectively as I assume that the more complete index created would also be beneficial to plain JVM based arrangements, too.

@sdeleuze
Copy link
Contributor

sdeleuze commented Mar 29, 2021

I was kind of hoping / assuming that the AOT functionality would – at some point in the future – end up in the Spring Boot build plugin anyway and spring-context-indexer deprecated effectively as I assume that the more complete index created would also be beneficial to plain JVM based arrangements, too.

That's my hope as well, I am totally in favor of getting more consistency across the build-time transformations we do but I guess that's a Spring Framework 6 / Spring Boot 3 discussion we need to have with @jhoeller and the wider team.

@aclement
Copy link
Contributor

Sounds like the simple enhancement first is to get inner classes treated properly. Then we should raise an issue that really evaluates always synthesizing it and working through potential clashes with any existing index.

@aclement
Copy link
Contributor

I think the code you linked is actually the problem area here, not the synthesis. The synthesis is working, build your project then:

cat target/generated-sources/spring-aot/src/main/resources/META-INF/spring.components

it will contain the inner classes just fine (at least my testcase does...)

So it is more about where you linked making decisions about what to do with that afterwards, which is separate to creating the list.

@odrotbohm
Copy link
Author

odrotbohm commented Mar 29, 2021

I don't see this working for RESTBucks. Steps to reproduce:

$ git clone https://github.com/odrotbohm/spring-restbucks
$ cd spring-restbucks/server
$ git checkout hacking/native
$ mvn clean package -DskipTests

The annotations on the inner class get added via the Maven ByteBuddy plugin. The log output of that should document the annotations being added. Note how the spring.components does not contain entries for e.g. OrderIdentifier or LineItemIdentifier. Also note, how it contains an entry for CreditCardNumber, which also gets the annotations added via ByteBuddy but is a top level type.

When I run the build using mvndebug and attach a debugger, I see LineItem being processed, ResourcesHandler finding the nested class LineItemIdentifier but the type.isComponent() check returning false causing the processing of the nested type to be skipped.

@odrotbohm
Copy link
Author

odrotbohm commented Mar 29, 2021

Debugging this closer, you have a point that the code I point to is not the cause for the entries being removed. synthesizeSpringComponents() is where things go off. While scanForSpringComponents() still contains the entry, the call to filterOutNestedTypes() right after that drops them based on the sole fact that they're nested types.

@aclement
Copy link
Contributor

Yep, i do see the rogue call but it doesn't affect my output so clearly something a little more subtle going on, let me dig into it. thanks for the real example code to kick around.

@aclement
Copy link
Contributor

Refreshing my memory! The reason the removal was occurring is that we don't really wanted nested configuration classes listed. That is because the outer may have conditions on that mean we don't want to process the inner. When processing configurations we dive into the inner after successfully handling the outer.

Of course, this was all done before Spring Data was considered. That's why it is breaking down now.

So I've modified it so that the filtering only occurs for configuration classes.

This allows OrderIdentifier through and LineItemIdentifier too:

That's the first part of things.

@aclement
Copy link
Contributor

aclement commented Apr 1, 2021

hey @odrotbohm how can I test this is doing what you need? I think they are no longer discarded - but I can't build restbucks reliably (even without native-image). I can't get a runnable jar (via mvn clean package -DskipTests) and I can't get a docker image via mvn clean spring-boot:build-image - is there something special I have to do?

@odrotbohm
Copy link
Author

odrotbohm commented Apr 1, 2021

I think we're fine here. I've upgraded the hacking/native branch to 0.9.2 snapshots of spring-native and the nested classes appear in spring.components. I've got the native image build to complete with that and the bootstrap now failing with some other Hibernate error (#692). Also, I think @christophstrobl's work on the Spring Data REST related hints is much more advanced than what I have thrown together here. I.e. I don't think we're going to get RESTBucks started as native image as is.

I think we can definitely resolve this ticket here.

Sorry for the confusion about the branches. I guess you have tried the master branch. I only keep it around for reference. Primary develop one main, all tweaks trying to make it work for native is in hacking/native. A flavor of this using Buildpacks in hacking/native-buildpacks.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: bug A general bug
Development

No branches or pull requests

3 participants