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

WW-5512 Extends the container to support injecting optional parameters into constructor #1175

Open
wants to merge 1 commit into
base: release/struts-6-7-x
Choose a base branch
from

Conversation

lukaszlenart
Copy link
Member

Copy link

sonarqubecloud bot commented Jan 5, 2025

Quality Gate Failed Quality Gate failed

Failed conditions
22 Security Hotspots
42.2% Coverage on New Code (required ≥ 80%)
3.4% Duplication on New Code (required ≤ 3%)
E Security Rating on New Code (required ≥ A)
E Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@lukaszlenart lukaszlenart marked this pull request as ready for review January 16, 2025 07:00
@kusalk
Copy link
Member

kusalk commented Jan 18, 2025

Does this also work for constructors with multiple optional parameters where 1 is available and the other is not?

@lukaszlenart
Copy link
Member Author

This is strictly for missing all the parameters and no default constructor exists.

@kusalk
Copy link
Member

kusalk commented Jan 18, 2025

How about the following - both parameters are injected as null even though one exists. Is that expected? I guess my concern here is that unintuitive behaviour here could lead to bugs being introduced in the future

@Inject(required = false)
public ConstructorCheck(@Inject(value = "constructorCheck.name") String name, @Inject(value = "nonExistingConstant") String name2) {

@lukaszlenart
Copy link
Member Author

I can add a test case for that, but that should block such a situation

(constructor.getParameterCount() > 0 && parameterInjectors == null)

constructor has parameters defined but there is nothing to inject.

@kusalk
Copy link
Member

kusalk commented Jan 18, 2025

Yep if you can add a test that verifies the behaviour for my given example hasn't changed (injection exception is thrown) that would make me more comfortable

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