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

Adds @External annotation #553

Merged
merged 6 commits into from
May 2, 2024
Merged

Adds @External annotation #553

merged 6 commits into from
May 2, 2024

Conversation

SentryMan
Copy link
Collaborator

@SentryMan SentryMan commented Apr 25, 2024

Adds a similar annotation to @Nullable such that compile time validations can be disabled for beans not managed by Inject, but will still fail at runtime if the bean is not present. An example use case is beans provided by Jooby

  • Adds an @External marker annotation
  • updates generator to use this new annotation
  • exempts Nullable/Optional beans from strict wiring calculus
  • renames the processor

@SentryMan SentryMan enabled auto-merge April 25, 2024 19:34
@SentryMan SentryMan added the enhancement New feature or request label Apr 25, 2024
@SentryMan SentryMan requested a review from rbygrave April 25, 2024 19:35
@SentryMan SentryMan self-assigned this Apr 25, 2024
@rbygrave
Copy link
Contributor

renames the processor

Any reason for that?

@SentryMan
Copy link
Collaborator Author

Any reason for that?

Aesthetics

@SentryMan
Copy link
Collaborator Author

hmm wait a sec

@SentryMan SentryMan marked this pull request as draft April 26, 2024 04:54
auto-merge was automatically disabled April 26, 2024 04:54

Pull request was converted to draft

@SentryMan SentryMan marked this pull request as ready for review April 26, 2024 05:32
@SentryMan SentryMan enabled auto-merge April 26, 2024 13:05
@SentryMan
Copy link
Collaborator Author

Okay, I'm good with this

@SentryMan SentryMan added this to the 10.0 milestone Apr 26, 2024
@rbygrave
Copy link
Contributor

rbygrave commented May 2, 2024

I'm pretty sure this does not support partial compile.

Edit: Seems like it isn't needed - all good !!

@rbygrave rbygrave disabled auto-merge May 2, 2024 09:47
@rbygrave rbygrave merged commit 5daa258 into avaje:master May 2, 2024
4 checks passed
@SentryMan SentryMan deleted the external branch May 2, 2024 12:02
@ponyjoy
Copy link

ponyjoy commented Jul 17, 2024

@SentryMan

@Factory
public class JdbiFactory {
    private static final String JDBC_URL = "jdbc:mysql://localhost:3306/myapp?serverTimeZone=Asia/Shanghai";
    private static final String DB_USER = "demo";
    private static final String DB_PASSWORD = "demo";
    private static Jdbi JDBI = null;

    @Bean
    public DataSource getDataSource() {
        HikariConfig config = new HikariConfig();
        config.setJdbcUrl(JDBC_URL);
        config.setUsername(DB_USER);
        config.setPassword(DB_PASSWORD);
        DataSource ds = new com.zaxxer.hikari.HikariDataSource(config);
        return ds;
    }

    @Bean
    public Jdbi getJdbi(DataSource ds) {
        JDBI = Jdbi.create(ds);
        JDBI.installPlugin(new org.jdbi.v3.sqlobject.SqlObjectPlugin());
        return JDBI;
    }
}

public class JdbiTodoService implements TodoService {

    final Jdbi jdbi;

    //  @Inject  
   //need to add @External before Jdbi parameter to compile 
    public JdbiTodoService(Jdbi jdbi) {
        if (jdbi == null) {
            throw new IllegalArgumentException("jdbi is null");
        }
        this.jdbi = jdbi;
    }
}

When I use version 10.1, why required to use @External to compile when using the constructor injection method with bean returned by the @Bean method? Isn’t the Jdbi object in this code managed by avaje-inject?

@SentryMan
Copy link
Collaborator Author

SentryMan commented Jul 17, 2024

is it the same in inject 10.0? Also does the JdbiTodoService service exist in a separate module than the factory?

@ponyjoy
Copy link

ponyjoy commented Jul 23, 2024

is it the same in inject 10.0? Also does the JdbiTodoService service exist in a separate module than the factory?
@SentryMan
In 10.0
does not add annotations and compiles successfully.

JdbiTodoService in same project
image

@SentryMan
Copy link
Collaborator Author

Is it possible for you to upload an example repo to GitHub so that I can reproduce?

@SentryMan
Copy link
Collaborator Author

SentryMan commented Jul 23, 2024

Also is it the same on inject 10.2-RC1? If it is fine there then we should be good.

@ponyjoy
Copy link

ponyjoy commented Jul 23, 2024

@SentryMan
jdbc-demo.zip
OK, it's a demo project

@SentryMan
Copy link
Collaborator Author

did you have any luck with 10.2-RC1? Also where are you seeing this error? I'm compiling with @External removed and 10.1 and I don't see the error when I run mvn clean package

@ponyjoy
Copy link

ponyjoy commented Jul 25, 2024

did you have any luck with 10.2-RC1? Also where are you seeing this error? I'm compiling with @External removed and 10.1 and I don't see the error when I run mvn clean package

It’s really strange. Without adding @External, the compilation passed with versions 10.0, 10.1, and 10.2-RC1. It must be because I didn’t run mvn clean compile before.

@rob-bygrave
Copy link
Contributor

There were 2 regressions for "Partial Compile" starting in version 10.0-RC5 including 10, 10.1, 10.2-RC*.
The regressions were only fixed in 10.2.

and I don't see the error when I run mvn clean package

You don't see these bugs with a full compile - so mvn clean package should have always worked, but partial compile with any 10.x prior to 10.2 could definitely produce those errors.

I suspect that was what this issue is. You should see no issue with 10.2 with partial compilation now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants