Skip to content

NoSuchBeanDefinitionException in spring-bean's old version#807

Merged
bruno-garcia merged 6 commits intogetsentry:masterfrom
Patrick0308:fix-issue-806
Feb 15, 2020
Merged

NoSuchBeanDefinitionException in spring-bean's old version#807
bruno-garcia merged 6 commits intogetsentry:masterfrom
Patrick0308:fix-issue-806

Conversation

@Patrick0308
Copy link
Copy Markdown

fixed issue #806 NoSuchBeanDefinitionException. @Autowired(required = false) doesn't work in spring-bean's old version.

@bruno-garcia
Copy link
Copy Markdown
Member

Thanks for raising this @Patrick0308 .
Do you mind adding a test so we can make sure the bootstrap works alright?

@Patrick0308
Copy link
Copy Markdown
Author

Do you mean adding a spring boot test? @bruno-garcia

@bruno-garcia
Copy link
Copy Markdown
Member

We have some tests here
https://github.com/getsentry/sentry-java/blob/master/sentry-spring-boot-starter/src/test/java/io/sentry/spring/autoconfigure/SentryAutoConfigurationTest.java
I’m wondering is something can be added that exemplifies the change (repro the error before your change and fixes after).

@Patrick0308
Copy link
Copy Markdown
Author

Patrick0308 commented Jan 8, 2020

@bruno-garcia I don't know how to test in different spring-bean versions. May be I use previous version in test?

@bruno-garcia
Copy link
Copy Markdown
Member

The versions we're testing are here:

Did you experience errors with this one or a different?

@Patrick0308
Copy link
Copy Markdown
Author

In a 1.5.3.RELEASE version of spring-boot-starter-parent, an error occurred on application startup. In pom.xml , we use 1.5.17.RELEASE version now.

@bruno-garcia
Copy link
Copy Markdown
Member

1.5.17.RELEASE is what our tests are targeting so it should work without this code change.
Have you tested this code change on 1.5.3.RELEASE before upgrading to 1.5.17.RELEASE to validate the fix works?
I'm just unsure if it's worth changing this since there are not tests to validate the behavior before/after. Since we test against 1.5.17.RELEASE it can be enough to say we don't support version 1.5.3.RELEASE, for example.

@Patrick0308
Copy link
Copy Markdown
Author

Yeah, I've tested in my application on 1.5.3.RELEASE. I suggest to support 1.5.x versions, because a lot of company use one of these versions.

@metlos
Copy link
Copy Markdown
Contributor

metlos commented Jan 14, 2020

Would you please then try reverting our spring-boot-dependencies version to say 1.5.0.RELEASE and write a test for the changed behavior?

Just an FYI, 1.5.0.RELEASE was released on 30 Jan 2017, 1.5.3.RELEASE on 21 Apr 2017, the latest 1.5.x is 1.5.22.RELEASE from 6 Aug 2019, the latest release is 2.2.2.RELEASE from 6 Dec 2019 with the first 2.x release, 2.0.0.RELEASE from 1 Mar 2018.

So IMHO it still does make sense to support the 1.5.x release train and maybe even go back to 1.5.0 but we should also make sure we work with the 2.x train.

Btw. to test our spring boot support with different versions of spring boot, we'd need to have an integration test maven module per spring boot version and run some shared tests within each such module. Maybe would be good to set this up at least for each minor version of spring boot (e.g. currently for 1.5.0, 2.0.0, 2.1.0 and 2.2.0)?

Copy link
Copy Markdown
Contributor

@metlos metlos left a comment

Choose a reason for hiding this comment

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

Generally looks good but I would like to see a test for what happens if the user application provides a similarly configured list of helpers and callbacks.

That said I am not that experienced with Spring or Spring Boot so maybe I'm talking trash right now :)

@Patrick0308
Copy link
Copy Markdown
Author

@metlos Do you have some examples about testing application in different versions?

@metlos
Copy link
Copy Markdown
Contributor

metlos commented Jan 16, 2020

@metlos Do you have some examples about testing application in different versions?

I rather meant reverting our spring dep to a version where the error would exhibit itself and write a test that makes sure that it doesn't fall over anymore.

But I can imagine this is quite hard given this change basically just changes how we instruct Spring to wire stuff together...

@Patrick0308
Copy link
Copy Markdown
Author

Patrick0308 commented Jan 17, 2020

Why this pr does't pass CI testing? Here is the error message.

Caused by: org.apache.maven.wagon.TransferFailedException: Failed to transfer file: http://repo.maven.apache.org/maven2/org/springframework/boot/spring-boot-test-support/1.5.0.RELEASE/spring-boot-test-support-1.5.0.RELEASE.pom. Return code is: 501 , ReasonPhrase:HTTPS Required.
	at org.apache.maven.wagon.providers.http.AbstractHttpClientWagon.fillInputData(AbstractHttpClientWagon.java:1039)
	at org.apache.maven.wagon.providers.http.AbstractHttpClientWagon.fillInputData(AbstractHttpClientWagon.java:977)
	at org.apache.maven.wagon.StreamWagon.getInputStream(StreamWagon.java:116)
	at org.apache.maven.wagon.StreamWagon.getIfNewer(StreamWagon.java:88)
	at org.apache.maven.wagon.StreamWagon.get(StreamWagon.java:61)
	at org.eclipse.aether.transport.wagon.WagonTransporter$GetTaskRunner.run(WagonTransporter.java:560)
	at org.eclipse.aether.transport.wagon.WagonTransporter.execute(WagonTransporter.java:427)
	at org.eclipse.aether.transport.wagon.WagonTransporter.get(WagonTransporter.java:404)
	at org.eclipse.aether.connector.basic.BasicRepositoryConnector$GetTaskRunner.runTask(BasicRepositoryConnector.java:447)
	at org.eclipse.aether.connector.basic.BasicRepositoryConnector$TaskRunner.run(BasicRepositoryConnector.java:350)
	... 48 more

I found spring-boot-test-support-1.5.0.RELEASE in central maven repo.

@metlos
Copy link
Copy Markdown
Contributor

metlos commented Jan 17, 2020

Huh, I recall someone on the twitter mentioning HTTP transport is being switched off on maven central...

@metlos
Copy link
Copy Markdown
Contributor

metlos commented Feb 9, 2020

It took me a looong time to get back here but reading https://spring.io/blog/2017/01/30/spring-boot-1-5-1-released#what-happened-to-1-5-0 I assume 1.5.0 can't really be used and the minimum version of the 1.5.x should be 1.5.1.

@Patrick0308
Copy link
Copy Markdown
Author

@metlos thanks very much. What I should do next?

@bruno-garcia
Copy link
Copy Markdown
Member

Thanks @Patrick0308 and @metlos

@bruno-garcia bruno-garcia merged commit e7e150c into getsentry:master Feb 15, 2020
@Patrick0308 Patrick0308 deleted the fix-issue-806 branch February 19, 2020 11:46
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 this pull request may close these issues.

3 participants