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.

Sign up for GitHub

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

Jump to bottom

Replace SpringPrototypeAggregateFactory BeanDefinition introspection for direct method invocation #2637

Merged
merged 2 commits into from Mar 9, 2023
Merged

Replace SpringPrototypeAggregateFactory BeanDefinition introspection for direct method invocation #2637

merged 2 commits into from Mar 9, 2023

Conversation

smcvb
Copy link
Member

@smcvb smcvb commented Mar 9, 2023

For some reason, Spring Framework logs the following message open usages of the static factory method of the SpringPrototypeAggregateFactory inside the SpringAggregateLookup component:

LocalVariableTableParameterNameDiscoverer : Using deprecated '-debug' fallback for parameter name resolution. Compile the affected code with '-parameters' instead or avoid its introspection: org.axonframework.spring.eventsourcing.SpringPrototypeAggregateFactory

Although none of the methods are marked deprecated on the BeanDefinition API, we are, apparently, dealing with undesired behavior for Spring.
Regardless, we can easily use a Supplier<SpringPrototypeAggregateFactory> in this case since the required properties (the aggregate name and its subtypes) for constructing the SpringPrototypeAggregateFactory are known upfront.

By switching from introspection to using a Supplier, we resolve #2630.

@smcvb
Replace introspection for direct method invocation
596bd9e 
For some reason, Spring Framework logs the following message open usages
 of the static factory method of the SpringPrototypeAggregateFactory:

LocalVariableTableParameterNameDiscoverer : Using deprecated '-debug'
fallback for parameter name resolution. Compile the affected code with
'-parameters' instead or avoid its introspection: org.axonframework.
spring.eventsourcing.SpringPrototypeAggregateFactory

Although none of the methods are marked deprecated on the BeanDefinition
 API, apparently this is undesired behavior. Regardless, we can easily
 use a Supplier<SpringPrototypeAggregateFactory> in this case, since the
 required properties for the factory are known upfront.

#2630
@smcvb smcvb added Type: Bug Use to signal issues that describe a bug within the system. Priority 2: Should High priority. Ideally, these issues are part of the release they’re assigned to. Status: In Progress Use to signal this issue is actively worked on. labels Mar 9, 2023
@smcvb smcvb added this to the Release 4.7.3 milestone Mar 9, 2023
@smcvb smcvb requested a review from a team March 9, 2023 10:32
@smcvb smcvb self-assigned this Mar 9, 2023
@smcvb smcvb requested review from gklijs and CodeDrivenMitch and removed request for a team March 9, 2023 10:32
@smcvb smcvb mentioned this pull request Mar 9, 2023
Closed
@sonarcloud
Copy link

sonarcloud bot commented Mar 9, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

gklijs
gklijs approved these changes Mar 9, 2023
View reviewed changes
Copy link
Contributor

@gklijs gklijs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍

@smcvb smcvb merged commit 986c41c into axon-4.7.x Mar 9, 2023
@smcvb smcvb deleted the bug/2630-spring-property-warning branch March 9, 2023 12:41
@smcvb smcvb added Status: Resolved Use to signal that work on this issue is done. and removed Status: In Progress Use to signal this issue is actively worked on. labels Mar 9, 2023
@abuijze
Copy link
Member

abuijze commented Mar 9, 2023

Unfortunately, this approach is not compatible with Spring AOT, nor Spring Native.

@abuijze
Copy link
Member

abuijze commented Mar 9, 2023

Looking at the code, I feel this is a slight variation of the Spring issue that is referred to in the original Axon issue. In this case, it's not a constructor, but a static method that's being used. In either case, it shouldn't use parameter names, as the parameters are defined based on index name.

@smcvb , I would suggest reverting the merge of this PR. It doesn't look like our bug

@smcvb
Copy link
Member Author

smcvb commented Mar 10, 2023

Thanks for investigating the support on Spring AOT, @abuijze.
I've chipped in on the referred-to conversation following this comment of Sébastien Deleuze.

In the meantime, I think it is safe to assume the warning is not impacting our users.
As such, I have reverted the commit, as can be seen here.

Nudging @nils-christian here, as he's the constructor of the issue.

@smcvb smcvb added Status: Obsolete Use to signal this issue is no longer necessary. and removed Status: Resolved Use to signal that work on this issue is done. labels Mar 10, 2023
@smcvb
Copy link
Member Author

smcvb commented Mar 10, 2023

Note that a follow-up issue has just been created by Sébastien Deleuze concerning this warning, which you can find here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gklijs gklijs gklijs approved these changes

@CodeDrivenMitch CodeDrivenMitch Awaiting requested review from CodeDrivenMitch CodeDrivenMitch was automatically assigned from AxonFramework/framework-developers

Assignees

@smcvb smcvb

Labels
Priority 2: Should High priority. Ideally, these issues are part of the release they’re assigned to. Status: Obsolete Use to signal this issue is no longer necessary. Type: Bug Use to signal issues that describe a bug within the system.
Projects
None yet
Milestone
Release 4.7.3
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@smcvb @abuijze @gklijs