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

3.0.0-M3 Logging Date Format #225

Closed
Tracked by #168
fabapp2 opened this issue Jul 15, 2022 · 13 comments · Fixed by #296
Closed
Tracked by #168

3.0.0-M3 Logging Date Format #225

fabapp2 opened this issue Jul 15, 2022 · 13 comments · Fixed by #296
Assignees
Labels
3.0.0 Spring Boot 3.0.0

Comments

@fabapp2
Copy link
Collaborator

fabapp2 commented Jul 15, 2022

The default format for the date and time component of log messages for Logback and Log4j2 has changed to align with the ISO-8601 standard. The new default format yyyy-MM-dd’T’HH:mm:ss.SSSXXX uses a T to separate the date and time instead of a space character and adds the timezone offset to the end. The LOG_DATEFORMAT_PATTERN environment variable or logging.pattern.dateformat property can be used to restore the previous default value of yyyy-MM-dd HH:mm:ss.SSS.

What needs to be done

Finder

Finds if logging.pattern.dateformat is set

Recipe

Set logging.pattern.dateformat=yyyy-MM-dd HH:mm:ss.SSS if this property is not currently set.

Report

Informs user about the change and how to reset it and why this is a good idea

@fabapp2 fabapp2 added this to the Spring Boot Upgrade 3.0 (M3) milestone Jul 15, 2022
@fabapp2 fabapp2 added 3.0.0 Spring Boot 3.0.0 and removed 3.0.0-M3 labels Jul 21, 2022
@ravig-kant
Copy link
Contributor

Hi @fabapp2

If no one is working on this issue, I can pick it up.

Ravi

@fabapp2
Copy link
Collaborator Author

fabapp2 commented Aug 1, 2022

Hi @fabapp2

If no one is working on this issue, I can pick it up.

Ravi

Sure @ravig-kant, but can we close #159 first?

@ravig-kant
Copy link
Contributor

Thanks, @fabapp2

I agree that we need to close #159 first. I have updated the PR with the comments addressed. I will wait for further review.

@fabapp2
Copy link
Collaborator Author

fabapp2 commented Aug 3, 2022

Thanks, @ravig-kant for picking up this issue!

@ravig-kant
Copy link
Contributor

Hi @fabapp2

In the case of a multimodule project, which project should be used to add the "logging.pattern.dateformat" pattern? Ideally, it should be the one containing the "SpringBootApplication" annotation. Also, can we pick anyone to add the property if the project contains multiple property files ( stitched together using "spring.config.location")?

@fabapp2
Copy link
Collaborator Author

fabapp2 commented Aug 5, 2022

Hi @ravig-kant
I would propose this algo:

In case the property is not set in any *.properties|*.yaml file...

  • Search for all application.properties / application.yaml / application.yml
  • Found matches?
    • add the property to each matching file
  • Found no matches?
    • Spring Boot application without application properties --> create new application.properties in all modules that contain a class annotated with @SpringBootApplication
    • add the property to the created file(s)

As all other property files inherit the properties in the default application.properties, adding the property there should be enough to have it effective in all existing profiles.

@ravig-kant
Copy link
Contributor

ravig-kant commented Aug 7, 2022

Hi @fabapp2

I have a question about leveraging ProjectResourceFinder for a specific module ( in a multi-module project). The finder works for the whole project but not for a specific module. The reason being the ApplicationModule doesn't expose any API to get its ResourseSets. It provides an option to get the JavaSourceSets that don't inherit from ProjectResourceSets.
Without the module-specific ProjectResouceSets, the finder can not be used. I worked it around by creating JavaSourceFinder which is somewhat like ProjectResourceFinder, but accepts JavaSourceSet. Let me know if I can go ahead with this workaround until we fix Application Module. I have created the PR ( though its WIP) so you can refer to the code for this discussion.

JavaSourceFinder - https://github.com/spring-projects-experimental/spring-boot-migrator/pull/296/files#diff-5c5279f2af2f8b2b5f99ca45654de9e4d76474b049593e4dc406b837f2505ae3

JavaSourceFinder Implementation - https://github.com/spring-projects-experimental/spring-boot-migrator/pull/296/files#diff-505b4759b4a11a16ec12a8158af4cbe84ddb6eafee7f767986e0082966500d5e

@fabapp2 fabapp2 linked a pull request Aug 8, 2022 that will close this issue
@fabapp2
Copy link
Collaborator Author

fabapp2 commented Aug 8, 2022

Hi @ravig-kant,

thanks for your PR!

The Maven multi-module support is in the making. But you're right, we need some functionality to ask ApplicationModules if they contain SpringBootApplicationProperties for this recipe.

I think the ApplicationModule should allow applying ProjectResourceFinder<T>s to a search method, similar to ProjectContext and there should not be a separate hierarchy of Finders.
For modules we'll need a way to apply these finders on main, test, sources and resources.
Maybe public <T> T searchMainResources(ProjectResourceFinder<T> finder) {...}
Obviously the ProjectResourceSet in ApplicationModule.search... needs to only contain/be filtered to contain relevant resources before applying the ProjectResourceFinder.

These need to be fixed

Then the algo could be implemented as

  • Always run AddSpringBootApplicationPropertiesAction beforehand (as separate, preceding action) to guarantee that a default application.properties exists
context.getApplicaitonModules()
    .getTopMostApplicationModules()
    .map(m -> m.searchMainResources())
    .filter(Optional::isPresent) // just in case
    .map(Optional::get)
    .forEach(p -> {
        if(!p.hasProperty("...") {
            p.setProperty("...", "...")
        }
    });

@fabapp2
Copy link
Collaborator Author

fabapp2 commented Aug 11, 2022

Hi @ravig-kant
I merged #304 and you could proceed with #305 and this issue if you like?

@ravig-kant
Copy link
Contributor

Hi @fabapp2

Thanks for prioritizing the search feature in ApplicationModule. I will proceed.

@ravig-kant
Copy link
Contributor

Hi @fabapp2

I raised PR for #305. However, for this issue, are you proposing to add application.properties to every top-level module? Earlier we decided to add the application.properties only for the modules containing classes annotated with SpringBootApplication.
Here are some thoughts on how we can still achieve the original goal.

  • Create a new recipe with the condition that no override of logging date format exists in the project. This recipe can be nested inside Spring Boot 3.0.0 recipe
  • Call action AddSpringBootApplicationPropertiesAction. Modify AddSpringBootApplicationPropertiesAction to take a flag ( addDefaultPropertiesFileToSpringBootApplications)
  • Call action AddLoggingDateFormat

I am not sure if we can nest a recipe. In case it's not possible, we can think of an alternate approach.

@fabapp2
Copy link
Collaborator Author

fabapp2 commented Aug 15, 2022

Hi @ravig-kant

I raised PR for #305. However, for this issue, are you proposing to add application.properties to every top-level module? Earlier we decided to add the application.properties only for the modules containing classes annotated with SpringBootApplication. Here are some thoughts on how we can still achieve the original goal.

You're right 😊
What made me change this (without further notice 😬 ) was the fact that the existence of @SpringBootApplication is not enough as a condition for Spring Boot applications.
While adding it to application modules will work in many cases but does not break anything in all others.

This has to be improved. I added an issue
#326

  • Create a new recipe with the condition that no override of logging date format exists in the project. This recipe can be nested inside Spring Boot 3.0.0 recipe

That's the report bit. 👍
See DatabaseDriverGaeSectionBuilder as an example. The beans will be picked up automatically by the org.springframework.sbm.boot.upgrade_27_30.SpringBoot30UpgradeReport and used to render the report.
The recipe is the one you linked: Spring Boot 3.0.0 recipe

  • Call action AddSpringBootApplicationPropertiesAction. Modify AddSpringBootApplicationPropertiesAction to take a flag ( addDefaultPropertiesFileToSpringBootApplications)
  • Call action AddLoggingDateFormat

I am not sure if we can nest a recipe. In case it's not possible, we can think of an alternate approach.

Not sure if that's what you meant with nesting, but you can (and should) add Actions to a recipe and make them execute sequentially, see boot-2.7-3.0-dependency-version-update.yaml

I'd say let's use your #305 and the report bit you mentioned and we add the properties files to application modules for now
to get this PR merged.
I'll know better if things work as planned when #270 is done and we can then look into #326 and rework things where required.

@ravig-kant
Copy link
Contributor

Hi @fabapp2

I have raised PR 296 as per the suggested approach.

Spring Boot 3.0 Upgrade automation moved this from In progress to Done Aug 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.0.0 Spring Boot 3.0.0
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

2 participants