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

Spring Boot starter does not bring spring-boot-starter #2866

Closed
snicoll opened this issue Jul 26, 2023 · 9 comments · Fixed by #2880
Closed

Spring Boot starter does not bring spring-boot-starter #2866

snicoll opened this issue Jul 26, 2023 · 9 comments · Fixed by #2880
Assignees
Labels
Platform: Java Type: Bug Something isn't working

Comments

@snicoll
Copy link

snicoll commented Jul 26, 2023

Integration

sentry

Java Version

17

Version

6.26.0

Steps to Reproduce

  • Add only sentry-spring-boot-starter-jakarta to a Spring Boot application
  • Compilation fails as core Spring Boot infrastructure is not available.

See also https://docs.spring.io/spring-boot/docs/current/reference/html/features.html#features.developing-auto-configuration.custom-starter.starter-module

Expected Result

compilation succeed.

Actual Result

> Task :compileJava
/Users/snicoll/Downloads/demo-sentry-test/src/main/java/com/example/demosentrytest/DemoSentryTestApplication.java:3: error: package org.springframework.boot does not exist
import org.springframework.boot.SpringApplication;
                               ^
/Users/snicoll/Downloads/demo-sentry-test/src/main/java/com/example/demosentrytest/DemoSentryTestApplication.java:4: error: package org.springframework.boot.autoconfigure does not exist
import org.springframework.boot.autoconfigure.SpringBootApplication;
                                             ^
/Users/snicoll/Downloads/demo-sentry-test/src/main/java/com/example/demosentrytest/DemoSentryTestApplication.java:6: error: cannot find symbol
@SpringBootApplication
 ^
  symbol: class SpringBootApplication
/Users/snicoll/Downloads/demo-sentry-test/src/main/java/com/example/demosentrytest/DemoSentryTestApplication.java:10: error: cannot find symbol
                SpringApplication.run(DemoSentryTestApplication.class, args);
                ^
  symbol:   variable SpringApplication
  location: class DemoSentryTestApplication
4 errors

> Task :compileJava FAILED
@adinauer
Copy link
Member

adinauer commented Jul 26, 2023

Hello @snicoll and thanks for the feedback. We'll discuss internally how we want to handle this.

Is it the naming that contains starter that makes you think we'll bring spring-boot-starter as well?
With all our integrations we set the dependencies to compileOnly thus requiring the user to bring their own version of the framework.

One of the reasons is our auto install feature (not released yet) in https://github.com/getsentry/sentry-android-gradle-plugin where we could be causing problems (e.g. circular dependency lookups) in combination with other libraries that analyze / manipulate dependencies. While we could look into sending PRs to fix those cases in each of the affected libraries (at least for open source ones), that would still mean we can't support versions before the fix.

If this is mostly just for having a better experience on start.spring.io, can we somehow automatically add spring-boot-starter in the config there to solve this case?

Edit: Sorry I missed the following statement in the linked issue:

I've configured start.spring.io to add the core starter if Sentry is the only entry selected.

@snicoll
Copy link
Author

snicoll commented Jul 26, 2023

Is it the naming that contains starter that makes you think we'll bring spring-boot-starter as well?

I don't know. Is it a Spring Boot starter? If so, it should bring spring-boot-starter(as described in the documentation). If it isn't, I'd argue it should be named differently and/or the code move to another module.

With all our integrations we set the dependencies to compileOnly thus requiring the user to bring their own version of the framework.

What framework?

that would still mean we can't support versions before the fix.

Sorry I didn't get any of that.

If this is mostly just for having a better experience on start.spring.io

No. The concept of a starter is that adding it is all that's needed to get started with X (where X is the target of the starter).

Can we somehow automatically add spring-boot-starter in the config there to solve this case?

I did, but only as of workaround until we find a fix for this issue. Unfortunately, we're in an odd situation right now. Either the starter should comply with our requirement or it shouldn't be named like one.

Either way is fine, even if I have a preference for the former. But the current state is not okay.

@adinauer
Copy link
Member

Is there a definition as to what should be called a starter and what not? Or is this the full definition?

The concept of a starter is that adding it is all that's needed to get started with X (where X is the target of the starter).


What framework?

Spring (boot) in this case.

Sorry I didn't get any of that.

Basically we'd be causing builds to break (with exceptions making it hard to find the cause like StackOverflowError) if people use our Gradle plugin with auto install turned on plus certain other gradle plugins if we switched from compileOnly to api/implementation.

@snicoll
Copy link
Author

snicoll commented Jul 26, 2023

Is there a definition as to what should be called a starter and what not? Or is this the full definition?

I gave a link to the reference guide already. Here it is again. This section a bit below is also of interest.

Spring (boot) in this case.

Sorry, I don't understand. If you have a Spring Boot starter or, rather an artifact called "sentry-spring-boot-starter", I don't get why users would have to bring Spring Boot. It's very obvious from the name it's tied to Spring Boot.

Basically we'd be causing builds to break (with exceptions making it hard to find the cause like StackOverflowError) if people use our Gradle plugin with auto install turned on plus certain other gradle plugins if we switched from compileOnly to api/implementation.

OK, that looks like something that should be fixed in the Gradle plugin and unrelated to Spring Boot.

@romtsn
Copy link
Member

romtsn commented Jul 26, 2023

@adinauer Alex I think we should not auto-install sentry-spring-boot-starters. As I understood this is supposed to be like a convenient BOM of dependencies that has everything to get you started (lol) with an integration/framework/technology, so there's nothing really to auto-install from our side (no dependency to tie the starter with).

@adinauer
Copy link
Member

@snicoll sorry I skimmed the link but didn't read far enough up and down. Thanks for linking again.

tl;dr: Our auto install feature in our gradle plugin should add dependency on a new module that should have auto-configure in its name. We keep the current module named starter and change the dependency away from compileOnly to bring spring-boot-starter (as well as Sentry and the new auto-configure module).


Sorry, I don't understand. If you have a Spring Boot starter or, rather an artifact called "sentry-spring-boot-starter", I don't get why users would have to bring Spring Boot. It's very obvious from the name it's tied to Spring Boot.

I think of it this way: A user decides on a certain library to solve their problem. In this case the user would chose to build their application using Spring. They then decide they want Sentry features on top of that so they add our dependency for Spring (Boot). It's not like they choose Sentry first and then use our Starter that brings in Spring Boot. Will try to provide more detail below.

A typical Spring Boot starter contains code to auto-configure and customize the infrastructure of a given technology

That's true for sentry-spring-boot-starter and sentry-spring-boot-starter-jakarta. They configure Spring and Sentry to work together.

The autoconfigure module that contains the auto-configuration code for "acme".
The starter module that provides a dependency to the autoconfigure module as well as "acme" and any additional dependencies that are typically useful. In a nutshell, adding the starter should provide everything needed to start using that library.

This makes me think our existing modules should rather be called autoconfigure or drag in dependencies required as you suggested.

Besides, you have the ability to craft a starter that provides an opinion about those optional dependencies. At the same time, others can rely only on the autoconfigure module and craft their own starter with different opinions.

Makes me think we should have separate starters for WebFlux and WebMVC, maybe even GraphQL (not released yet). We currently rely on conditional checks for our auto configuration. IMO it's easier for users to add a single dependency and it automatically figures out what features can / should be enabled rather than requiring them to add a starter per feature. This again ties in with what I wrote further up where users chose libraries that solve their problem and then add Sentry for getting more insights into errors / performance etc. of those libraries.

The starter is really an empty jar. Its only purpose is to provide the necessary dependencies to work with the library. You can think of it as an opinionated view of what is required to get started.

This would include spring-boot-starter as well as otherwise adding sentry-spring-boot-starter wouldn't make sense.


OK, that looks like something that should be fixed in the Gradle plugin and unrelated to Spring Boot.

Yes, this has nothing to do with Spring Boot. And I agree it should be fixed somewhere else in theory but doing so has drawbacks where we break users builds with cryptic exceptions. A workaround would be to have another module that is added by the auto install feature. If I understand what I read correctly, this should be called auto-configure. The current module we already have could bring spring-boot-starter by changing the dependency type away from compileOnly.


I think we should not auto-install sentry-spring-boot-starters. As I understood this is supposed to be like a convenient BOM of dependencies that has everything to get you started (lol) with an integration/framework/technology, so there's nothing really to auto-install from our side (no dependency to tie the starter with).

@romtsn we can tie it to spring-boot-starter. We just shouldn't drag in our Starter (if that drags in spring-boot-starter) but instead a new module called auto-configure.

@snicoll please let me know if you agree with the module names and structure of dependencies.

@adinauer
Copy link
Member

@snicoll is there any urgency to changing our module structure or can we keep the existing workaround in start.spring.io?

@snicoll
Copy link
Author

snicoll commented Jul 27, 2023

It's not like they choose Sentry first and then use our Starter that brings in Spring Boot.

That doesn't matter. The semantic of a Spring Boot starter should not change based on the fact that a particular starter is likely to be added on top of other starters.

This makes me think our existing modules should rather be called autoconfigure or drag in dependencies required as you suggested.

I think you lost me now. What do you mean by "makes me think". This is what I am reporting and asking in this issue since the very beginning.

Makes me think we should have separate starters for WebFlux and WebMVC, maybe even GraphQL

You are going in the opposite direction now. Spring Boot with web, security and JPA is very popular. Yet we haven't created a "spring-boot-starter-web-security-data-jpa". Rather we have auto-configuration that reacts on the presence of certain libraries and combining starters does the right thing. In your case, the sentry starter + the "official" graphQL starter could enable whatever is necessary without the need of a dedicated starter on your end.

If I understand what I read correctly, this should be called auto-configure. The current module we already have could bring spring-boot-starter by changing the dependency type away from compileOnly.

That could be an option, yes. However, the doc does not suggest to name it auto-configure (but that's fine if you prefer that). Basically rename the current module to sentry-spring-boot (or spring-boot-autoconfigure) and create a sentry-spring-boot-starter that brings this module + spring-boot-starter. While we're about renaming things, you may want the jakarta bit to go away as it's already the default and the "non jakarta" will be soon EOL anyway.

Is there any urgency to changing our module structure or can we keep the existing workaround in start.spring.io?

I wouldn't have accepted the entry before this issue was handled one way or another. However, given how much time we took to process it, I added it with the workaround for now. No urgency no, but this will need to be adressed for the entry to stay on start.spring.io.

@adinauer
Copy link
Member

I just tried to write down my thoughts while reading through the docs you linked. Mostly for myself when I come back and reread in the future.

In your case, the sentry starter + the "official" graphQL starter could enable whatever is necessary without the need of a dedicated starter on your end.

Sounds good. That's the way it works at the moment.

Basically rename the current module to sentry-spring-boot (or spring-boot-autoconfigure) and create a sentry-spring-boot-starter that brings this module + spring-boot-starter. While we're about renaming things, you may want the jakarta bit to go away as it's already the default and the "non jakarta" will be soon EOL anyway.

Thanks. We'll discuss naming internally as people come back from vacation. We can also discuss changing the whole jakarta and javax naming in the next major. Majority of our users at the moment is still on Spring Boot 2 though so might be too early for us to rename it now.

I wouldn't have accepted the entry before this issue was handled one way or another. However, given how much time we took to process it, I added it with the workaround for now. No urgency no, but this will need to be adressed for the entry to stay on start.spring.io.

Thanks, we'll make sure to resolve this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Platform: Java Type: Bug Something isn't working
Projects
Archived in project
3 participants