Skip to content

Fix some trivial issues learned on real project #492

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

Conversation

tan9
Copy link
Contributor

@tan9 tan9 commented Oct 20, 2022

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
Copy link
Contributor Author

tan9 commented Oct 21, 2022

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

@sanagaraj-pivotal
Copy link
Contributor

sanagaraj-pivotal commented Oct 24, 2022

@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.

@tan9
Copy link
Contributor Author

tan9 commented Oct 27, 2022

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

@fabapp2 fabapp2 added the type: enhancement New feature or request label Oct 31, 2022
@fabapp2 fabapp2 added this to the v0.13.0 milestone Oct 31, 2022
@tan9 tan9 force-pushed the fix/trivial-issues-learned-on-real-project branch from 939bba4 to 4bb23c1 Compare November 1, 2022 00:58
@tan9
Copy link
Contributor Author

tan9 commented Nov 1, 2022

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

@fabapp2
Copy link
Contributor

fabapp2 commented Nov 1, 2022

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.

Copy link
Contributor

@fabapp2 fabapp2 left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution @tan9 🚀

@fabapp2 fabapp2 merged commit 6ae8739 into spring-projects-experimental:main Nov 1, 2022
@fabapp2
Copy link
Contributor

fabapp2 commented Nov 2, 2022

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.

@tan9 tan9 deleted the fix/trivial-issues-learned-on-real-project branch November 2, 2022 03:22
@tan9
Copy link
Contributor Author

tan9 commented Nov 2, 2022

@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.

@fabapp2
Copy link
Contributor

fabapp2 commented Nov 2, 2022

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
Copy link
Contributor

fabapp2 commented Nov 2, 2022

I added #516 to progress on this

@tan9
Copy link
Contributor Author

tan9 commented Nov 2, 2022

@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 :)

@fabapp2
Copy link
Contributor

fabapp2 commented Nov 2, 2022

@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!

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

Successfully merging this pull request may close these issues.

3 participants