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

Fix issue with 'springProperty' not loaded from context scope when initializing with 'playSaxEvents' in logback #35670

Closed

Conversation

geminiKim
Copy link
Contributor

related issue : #35664

I fixed an issue that worked in boot 2.7 but has been happening since boot 3.x.

I think this issue was caused by the deletion of this line in this commit.
(And while I'm not entirely certain, I think this might also be due to the effects of updating logback to version 1.4.x)

[Causes and actions]

Looking at the doConfigure(...) function in ch.qos.logback.core.joranGenericXMLConfigurator.java, it works like this.

        Model top = buildModelFromSaxEventList(recorder.getSaxEventList());
        if (top == null) {
            addError(ErrorCodes.EMPTY_MODEL_STACK);
            return;
        }
        sanityCheck(top);
        processModel(top);

In the current situation, the processModel is lagging behind, and buildModelFromSaxEventList(...) isn't loading the springProperty.

Hence, I've injected springProperty into the context using a SpringPropertyAction, which is a part of the flow executed by playSaxEvents(...).

(Referring to the previously mentioned commit, it appears that initialization was done via SpringPropertyAction in the past.)

Additional comments

I haven't given much thought to this revision, as I'm unaware of the historical context.
However, this issue has prevented me from upgrading the current project to boot 3.x.
It would be truly beneficial if this issue could be resolved.

I'm always using Spring-Boot well. Thank you for your efforts.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label May 29, 2023
Comment on lines +61 to +66
String getSourceOrDefaultValue() {
if (this.source == null) {
return this.defaultValue;
}
return this.source;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not certain if it's appropriate to find the default values here.
However, since the logic was handled within the object, I made sure this class took responsibility for it.

Comment on lines +51 to +52
if (ActionUtil.stringToScope(model.getScope()) == ActionUtil.Scope.CONTEXT) {
interpretationContext.getContext().putProperty(model.getName(), model.getSourceOrDefaultValue());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to use an appropriate Util Class in logback, but I couldn't find it.
ActionUtil, ModelUtil were not applicable in this situation.

@philwebb philwebb changed the title Fixed issue with springProperty not loaded from context scope when initializing with playSaxEvents in logback. #35664 Fix issue with 'springProperty' not loaded from context scope when initializing with 'playSaxEvents' in logback May 31, 2023
@philwebb philwebb added for: team-meeting An issue we'd like to discuss as a team to make progress status: declined A suggestion or change that we don't feel we should currently apply and removed status: waiting-for-triage An issue we've not yet triaged for: team-meeting An issue we'd like to discuss as a team to make progress labels May 31, 2023
@philwebb
Copy link
Member

Thanks for the PR, but I don't think we can accept this change due to the reason documented in #35664 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants