Skip to content

Conversation

@MFAshby
Copy link
Contributor

@MFAshby MFAshby commented Aug 7, 2025

It should delegate back to ObjectFactory#buildBean instead of directly calling Container#inject, otherwise overrides of buildBean in subclasses of ObjectFactory e.g. SpringObjectFactory are skipped; meaning that TypeConverters cannot make use of Spring dependency injection

Fixes: https://issues.apache.org/jira/projects/WW/issues/WW-5524

It should delegate back to ObjectFactory#buildBean instead of directly
calling Container#inject, otherwise overrides of buildBean in subclasses
of ObjectFactory e.g. SpringObjectFactory are skipped; meaning that
TypeConverters cannot make use of Spring dependency injection

Fixes: https://issues.apache.org/jira/projects/WW/issues/WW-5524
@lukaszlenart lukaszlenart requested a review from Copilot August 18, 2025 14:56
@lukaszlenart lukaszlenart changed the title Fixup StrutsConverterFactory WW-5524 Fixup StrutsConverterFactory Aug 18, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes an issue where StrutsConverterFactory was bypassing ObjectFactory subclass overrides (like SpringObjectFactory) by directly calling Container#inject instead of delegating to ObjectFactory#buildBean. This ensures TypeConverters can properly utilize Spring dependency injection.

  • Changes dependency injection mechanism from Container#inject to ObjectFactory#buildBean
  • Updates injection dependency from Container to ObjectFactory
  • Adds missing @OverRide annotation for interface implementation

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@lukaszlenart
Copy link
Member

@MFAshby do you want to port this change into 6.x?

@MFAshby
Copy link
Contributor Author

MFAshby commented Aug 19, 2025

@MFAshby do you want to port this change into 6.x?

Thanks for looking @lukaszlenart ,

I don't need this backporting for my current project (I'm migrating to 7.x) but others might find it useful. I leave the decision to you.

Copy link
Member

@lukaszlenart lukaszlenart left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@lukaszlenart lukaszlenart merged commit 79c6bf9 into apache:main Aug 20, 2025
6 checks passed
@lukaszlenart
Copy link
Member

Cherry picked #1316

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants