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

@PostConstruct import being removed from beans #457

Closed
melloware opened this issue Apr 17, 2024 · 20 comments · Fixed by #525
Closed

@PostConstruct import being removed from beans #457

melloware opened this issue Apr 17, 2024 · 20 comments · Fixed by #525
Assignees
Labels
bug Something isn't working

Comments

@melloware
Copy link
Contributor

melloware commented Apr 17, 2024

What version of OpenRewrite are you using?

Latest

How are you running OpenRewrite?

CLI using mvn -U org.openrewrite.maven:rewrite-maven-plugin:run -Drewrite.recipeArtifactCoordinates=org.openrewrite.recipe:rewrite-migrate-java:RELEASE -Drewrite.activeRecipes=org.openrewrite.java.migrate.jakarta.JakartaEE10

CDI Spec: https://jakarta.ee/specifications/cdi/2.0/cdi-spec-2.0.pdf

Before conversion:

package org.primefaces.showcase.view.overlay;

import org.primefaces.showcase.domain.Movie;

import javax.annotation.PostConstruct;
import javax.enterprise.context.RequestScoped;
import javax.inject.Named;
import java.util.ArrayList;
import java.util.List;

import javax.faces.application.FacesMessage;
import javax.faces.context.FacesContext;

import org.primefaces.event.SelectEvent;

@Named
@RequestScoped
public class MovieView {

    private Movie movie;

    private List<Movie> movieList;

    public List<Movie> getMovieList() {
        return movieList;
    }

    @PostConstruct
    public void init() {
        movieList = new ArrayList<Movie>();

        movieList.add(new Movie("The Lord of the Rings: The Two Towers", "Peter Jackson", "Fantasy, Epic", 179));
        movieList.add(new Movie("The Amazing Spider-Man 2", "Marc Webb", "Action", 142));
        movieList.add(new Movie("Iron Man 3", "Shane Black", "Action", 109));
        movieList.add(new Movie("Thor: The Dark World", "Alan Taylor", "Action, Fantasy", 112));
        movieList.add(new Movie("Avatar", "James Cameron", "Science Fiction", 160));
        movieList.add(new Movie("The Lord of the Rings: The Fellowship of the Ring", "Peter Jackson", "Fantasy, Epic", 165));
        movieList.add(new Movie("Divergent", "Neil Burger", "Action", 140));
        movieList.add(new Movie("The Hobbit: The Desolation of Smaug", "Peter Jackson", "Fantasy", 161));
        movieList.add(new Movie("Rio 2", "Carlos Saldanha", "Comedy", 101));
        movieList.add(new Movie("Captain America: The Winter Soldier", "Joe Russo", "Action", 136));
        movieList.add(new Movie("Fast Five", "Justin Lin", "Action", 132));
        movieList.add(new Movie("The Lord of the Rings: The Return of the King", "Peter Jackson", "Fantasy, Epic", 200));

    }

    public Movie getMovie() {
        return movie;
    }

    public void setMovie(Movie movie) {
        this.movie = movie;
    }

    public void onRowSelect(SelectEvent<Movie> event) {
        FacesMessage msg = new FacesMessage("Movie Selected", event.getObject().getMovie());
        FacesContext.getCurrentInstance().addMessage(null, msg);
    }
}

After Conversion:

package org.primefaces.showcase.view.overlay;

import org.primefaces.showcase.domain.Movie;

import jakarta.enterprise.context.RequestScoped;
import jakarta.inject.Named;
import java.util.ArrayList;
import java.util.List;

import jakarta.faces.application.FacesMessage;
import jakarta.faces.context.FacesContext;

import org.primefaces.event.SelectEvent;

@Named
@RequestScoped
public class MovieView {

    private Movie movie;

    private List<Movie> movieList;

    public List<Movie> getMovieList() {
        return movieList;
    }

    @PostConstruct
    public void init() {
        movieList = new ArrayList<Movie>();

        movieList.add(new Movie("The Lord of the Rings: The Two Towers", "Peter Jackson", "Fantasy, Epic", 179));
        movieList.add(new Movie("The Amazing Spider-Man 2", "Marc Webb", "Action", 142));
        movieList.add(new Movie("Iron Man 3", "Shane Black", "Action", 109));
        movieList.add(new Movie("Thor: The Dark World", "Alan Taylor", "Action, Fantasy", 112));
        movieList.add(new Movie("Avatar", "James Cameron", "Science Fiction", 160));
        movieList.add(new Movie("The Lord of the Rings: The Fellowship of the Ring", "Peter Jackson", "Fantasy, Epic", 165));
        movieList.add(new Movie("Divergent", "Neil Burger", "Action", 140));
        movieList.add(new Movie("The Hobbit: The Desolation of Smaug", "Peter Jackson", "Fantasy", 161));
        movieList.add(new Movie("Rio 2", "Carlos Saldanha", "Comedy", 101));
        movieList.add(new Movie("Captain America: The Winter Soldier", "Joe Russo", "Action", 136));
        movieList.add(new Movie("Fast Five", "Justin Lin", "Action", 132));
        movieList.add(new Movie("The Lord of the Rings: The Return of the King", "Peter Jackson", "Fantasy, Epic", 200));

    }

    public Movie getMovie() {
        return movie;
    }

    public void setMovie(Movie movie) {
        this.movie = movie;
    }

    public void onRowSelect(SelectEvent<Movie> event) {
        FacesMessage msg = new FacesMessage("Movie Selected", event.getObject().getMovie());
        FacesContext.getCurrentInstance().addMessage(null, msg);
    }
}

ERROR

Instead of converting import javax.annotation.PostConstruct; it just removed the import entirely.

@melloware melloware added the bug Something isn't working label Apr 17, 2024
@melloware melloware changed the title @PostConstruct being removed from @RequestScopedBeans @PostConstruct being removed from beans Apr 17, 2024
@timtebeek
Copy link
Contributor

Thanks for the report @melloware & welcome back! @cjobinabo Does this ring familiar to you or your team? Did we miss a test case to handle this properly?

@timtebeek timtebeek moved this to Backlog in OpenRewrite Apr 17, 2024
@melloware
Copy link
Contributor Author

Nice to be back! I am lurking always checking on your progress :)

@melloware melloware changed the title @PostConstruct being removed from beans @PostConstruct import being removed from beans Apr 18, 2024
@melloware
Copy link
Contributor Author

melloware commented Apr 18, 2024

I am looking through PR's and I can't see anything in particular that would have changed this? This was definitely not broken a couple of months ago when I did all my JakartaEE10 testing so it must have changed more recently?

@melloware
Copy link
Contributor Author

Interesting in my log i see that it ran the correct recipes:

RNING] Changes have been made to primefaces-showcase\src\main\java\org\primefaces\showcase\view\overlay\MovieView.java by:
[WARNING]     org.openrewrite.java.migrate.jakarta.JakartaEE10
[WARNING]         org.openrewrite.java.migrate.jakarta.JavaxMigrationToJakarta
[WARNING]             org.openrewrite.java.migrate.jakarta.JavaxAnnotationMigrationToJakartaAnnotation
[WARNING]                 org.openrewrite.java.ChangeType: {oldFullyQualifiedTypeName=javax.annotation.PostConstruct, newFullyQualifiedTypeName=jakarta.annotation.PostConstruct}
[WARNING]             org.openrewrite.java.migrate.jakarta.JavaxEnterpriseToJakartaEnterprise
[WARNING]                 org.openrewrite.java.ChangePackage: {oldPackageName=javax.enterprise, newPackageName=jakarta.enterprise, recursive=true}
[WARNING]             org.openrewrite.java.migrate.jakarta.JavaxFacesToJakartaFaces
[WARNING]                 org.openrewrite.java.ChangePackage: {oldPackageName=javax.faces, newPackageName=jakarta.faces, recursive=true}
[WARNING]             org.openrewrite.java.migrate.jakarta.JavaxInjectMigrationToJakartaInject
[WARNING]                 org.openrewrite.java.ChangePackage: {oldPackageName=javax.inject, newPackageName=jakarta.inject, recursive=true}

But it definitely dopped the import.

The code I am running against is here: https://github.com/primefaces/primefaces/tree/master/primefaces-showcase

@melloware
Copy link
Contributor Author

although interesting its a ChangeType instead of a ChangePackage?

@bsmahi
Copy link

bsmahi commented Apr 22, 2024

@melloware I have tried replicating your issue, for me PostConstruct import is not deleted

diff --git a/devworkspace/oss/openrewrite-app/src/main/java/com/learnopenrewrite/app/Demo.java b/devworkspace/oss/openrewrite-app/src/main/java/com/learnopenrewrite/app/Demo.java
index 9c2ec3c..ef57d3e 100644
--- a/devworkspace/oss/openrewrite-app/src/main/java/com/learnopenrewrite/app/Demo.java
+++ b/devworkspace/oss/openrewrite-app/src/main/java/com/learnopenrewrite/app/Demo.java
@@ -1,6 +1,6 @@ org.openrewrite.config.CompositeRecipe
 package com.learnopenrewrite.app;
 
-import javax.annotation.PostConstruct;
+import jakarta.annotation.PostConstruct;
 
 public class Demo {

However, when I ran your code, am able to replicate that PostConstruct import statement got deleted.

Keep you posted.

Thanks,
Mahi

@melloware
Copy link
Contributor Author

Thanks @bsmahi i agree when I write a small test case it works but on that large project it is happening. Thanks for replicating!

@timtebeek
Copy link
Contributor

We did a new release Wednesday which also includes these recent changes

Would you mind checking if this still affects you using these versions?

It's odd that this has started happening. We hadn't added an explicit unit test for this one as it's just a few lines of yaml

- org.openrewrite.java.ChangeType:
oldFullyQualifiedTypeName: javax.annotation.PostConstruct
newFullyQualifiedTypeName: jakarta.annotation.PostConstruct

@melloware
Copy link
Contributor Author

Confirmed this is still happening with latest release. It works in a small isolated test case but not in a large project like PrimeFaces Showcase its still stripping the import

@AndreasALoew
Copy link

AndreasALoew commented Aug 7, 2024

Any news with regards to this one? It affects our EE10 migration as well...

The weird thing about it is that indeed the "ChangeType" recipe is so simple that it should "just work", and the big frightening question is: Why does it still do so only in a simple test case, but not in real world scenarios?

I fear that the underlying issue is not just about javax.annotation.PostConstruct, but could also apply "arbitrary" other cases where org.openrewrite.java.ChangeType has been used (and they silently fail to replace or even remove lines)!? 😢

@timtebeek , many thanks for an update on the current status...

@timtebeek
Copy link
Contributor

No news unfortunately, although we'd definitely like to see this fixed, especially if there's a suspected underlying issue with a core recipe. Would you be able to provide us with an ideally minimal reproducer? As unit tests for now appear not to cover this just yet.

@melloware
Copy link
Contributor Author

I am not positive but I have a feeling that two different rules are somehow applying here in the EE10 migration causing this. I will try and study it further but it is very strange.

@melloware
Copy link
Contributor Author

melloware commented Aug 7, 2024

I submitted a PR and all tests are passing but I can't figure out with Gradle how do I do the equivalent of mvn clean install to test the SNAPSHOT locally.. Man. I hate gradle.

melloware added a commit to melloware/rewrite-migrate-java that referenced this issue Aug 7, 2024
@AndreasALoew
Copy link

Would you be able to provide us with an ideally minimal reproducer?

Which was exactly the main culprit here. With any sufficiently stripped-down test case, it works fine, and in the full complex scenario, it fails...

But thanks a million to @melloware 👍 - I think you chose a very valid approach, and as I was testing myself to move from ChangeType to ChangePackage with good results, I am more than confident that this type of resolution will work - 🤞

Are you already able to tell us which official release version will contain this fix?

many thx for the super-fast response time! Highly appreciated! 😄 👍

@melloware
Copy link
Contributor Author

@AndreasALoew i know once @timtebeek approves the team is good about doing releases every week or two weeks

I think this fix works as well!

@AndreasALoew
Copy link

@melloware , after looking into your PR, great catch to notice that you have to revert the renaming for "javax.annotation.processing"... 👍 👍 👍 😉

Just out of curiosity: How did you notice - I myself would have overlooked this from what I can see in our code base...

@melloware
Copy link
Contributor Author

@timtebeek noticed in commit history of why it was done like this in the first place. I would not have noticed!

@github-project-automation github-project-automation bot moved this from Backlog to Done in OpenRewrite Aug 7, 2024
@timtebeek
Copy link
Contributor

Thanks all! Our latest snapshot versions should contain this fix; would appreciate a quick check if the problem is indeed resolved, but for now we'll assume this is fixed.

@AndreasALoew
Copy link

Yes, I can confirm that the latest snapshot fixes the issue for "my"/our fully complex scenario👍

Thanks again! 😄

@melloware
Copy link
Contributor Author

I am glad we got to the bottom of this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants