spring-boot-migrator icon indicating copy to clipboard operation
spring-boot-migrator copied to clipboard

Fix some trivial issues learned on real project

Open tan9 opened this issue 1 year ago • 1 comments

We are currently evaluating using spring-boot-migrator on our legacy applications and encountering some trivial exceptions. Hope this pull request helps make this project more robust.

tan9 avatar Oct 20 '22 07:10 tan9

@fabapp2 would you please approve me as a first-time contributor to run workflows?

tan9 avatar Oct 21 '22 00:10 tan9

@tan9 Welcome to Spring Boot Migrator. Thanks for the commit 🎉

You can also emulate what our workflows do locally by doing mvn clean test in the meantime.

sanagaraj-pivotal avatar Oct 24 '22 09:10 sanagaraj-pivotal

Hi @sanagaraj-pivotal , the problem is that I can't manage to pass all tests in main on my local machine like:

[WARNING] Tests run: 1, Failures: 0, Errors: 0, Skipped: 1, Time elapsed: 0 s - in org.springframework.sbm.jee.jsf.recipes.AddJoinfacesDependencies_MyFaces_Test
[INFO] 
[INFO] Results:
[INFO] 
[ERROR] Failures: 
[ERROR]   AddJmsConfigTest.testAddJmsConfig:122 [

Here's the diff between 1. (---) actual and 2. (+++) expected:

--- package com.example.foo;

import javax.jms.ConnectionFactory;


import org.springframework.boot.autoconfigure.jms.DefaultJmsListenerContainerFactoryConfigurer;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.jms.annotation.EnableJms;
import org.springframework.jms.config.DefaultJmsListenerContainerFactory;
import org.springframework.jms.config.JmsListenerContainerFactory;

Am I missing something? I am using OpenJDK 17.0.5

tan9 avatar Oct 27 '22 09:10 tan9

@fabapp2 I have had rebase this branch onto main and passed all the tests, would you please provide some feedback :)

tan9 avatar Nov 01 '22 02:11 tan9

HI @tan9

thank you for your contribution 🚀 ... and sorry for my bad responsiveness, I've been on vacation and got a pile of work.

Your PR made me write a test for BuildFile.getDeclaredDependencies() BuildFile.getRequestedDependencies() and BuildFile.getEffectiveDependencies() with a multi-module setup which unfortunately surfaced some more issues.

fabapp2 avatar Nov 01 '22 11:11 fabapp2

Hi @tan9

I deleted my previous comment as I misunderstood your problem. My test was failing because of an invalid artifactId that I created but after fixing this dependencies to modules in the same multi-module project were properly resolved and

if (dependencies.isEmpty()) {
    // requested dependency from another module in this multi-module project won't be resolvable
    return d;
}

was not required.

Could you maybe provide an example that made you add this if block? I would like to see if my test (not pushed yet, probably tomorrow) does not cover the issue you were facing.

fabapp2 avatar Nov 02 '22 00:11 fabapp2

@fabapp2 All I can remember right now was described in the comment:

requested dependency from another module in this multi-module project won't be resolvable

If we are working on ear packaging module to assemble one or more war packaging module, the war module somehow won't be resolvable from getPom(),findDendendepcies() call, hence returning empty dependencies, causing dependenceis.get(0) to throw IndexOutOfBoundsException.

Please let me know if the information above is enough to spot the root cause, Or I can try to reproduce this issue and give more details later today or tomorrow.

tan9 avatar Nov 02 '22 03:11 tan9

Hi @tan9

That's valuable insight, thank you! As we're mostly focussing on Boot applications at the moment we might just not have realized this problem. Actually, there's a good chance that I'd need to discuss this with the people from OpenRewrite who are doing the heavy lifting here. I will try with a JEE setup using ear and war and see if I can reproduce this problem. It is not the current focus (which is the Boot 3 Upgrade report) but I will create a new issue for this and look into it. I am excited that you're using SBM with a real JEE application (they are hard to find publicly). So, please keep us posted about issues and problems you might encounter.

I will finish and commit the test (and fix some things on the way) I am working on and ping you here. Hoping this test setup would make it easy to provide something to reproduce the problem. If you find the time it'd be awesome if you could try to provide a minimal example highlighting the issues you had. Please, don't feel pressured though.

fabapp2 avatar Nov 02 '22 08:11 fabapp2

I added #516 to progress on this

fabapp2 avatar Nov 02 '22 09:11 fabapp2

@fabapp2 I am glad that I can help.

I am going to migrate tons of Java EE projects (100+) that are all tight to Spring Framework 3 under Java 6. We tried using TypeScript to migrate our projects with little success. However, due to the leak of AST, we cannot do further refactoring in an effortless way. Until I read SBM from the Preparing for Spring Boot 3.0 blog post.

Thanks to all of you guys working on this project, I will try my best to make this project more robust against our legacy and fragile projects :)

tan9 avatar Nov 02 '22 09:11 tan9

@fabapp2 I am glad that I can help.

I am going to migrate tons of Java EE projects (100+) that are all tight to Spring Framework 3 under Java 6. We tried using TypeScript to migrate our projects with little success. However, due to the leak of AST, we cannot do further refactoring in an effortless way. Until I read SBM from the Preparing for Spring Boot 3.0 blog post.

Thanks to all of you guys working on this project, I will try my best to make this project more robust against our legacy and fragile projects :)

I am glad you give SBM a spin and help us to become better and more stable 🤩
Feel free to use discussions or any other way of contact to ask questions, we will try our best to help wherever we can!

fabapp2 avatar Nov 02 '22 13:11 fabapp2