Skip to content

WW-5468 Fixes @StrutsParameter for ModelDriven#1071

Closed
lukaszlenart wants to merge 1 commit into
release/struts-7-0-xfrom
fix/WW-5468-modeldriven
Closed

WW-5468 Fixes @StrutsParameter for ModelDriven#1071
lukaszlenart wants to merge 1 commit into
release/struts-7-0-xfrom
fix/WW-5468-modeldriven

Conversation

@lukaszlenart

Copy link
Copy Markdown
Member

Fixes WW-5468

@lukaszlenart lukaszlenart requested a review from kusalk October 11, 2024 08:49
@lukaszlenart lukaszlenart force-pushed the fix/WW-5468-modeldriven branch 3 times, most recently from 517e724 to 0e56a79 Compare October 11, 2024 09:19
@lukaszlenart lukaszlenart force-pushed the fix/WW-5468-modeldriven branch from 0e56a79 to 5c56839 Compare October 11, 2024 09:31
@sonarqubecloud

Copy link
Copy Markdown

@kusalk

kusalk commented Oct 11, 2024

Copy link
Copy Markdown
Member

It seems that the java.beans.PropertyDescriptor API will always return the getter method on the ModelDriven interface rather than the Action (I didn't actually realise this when I wrote this feature). This means it doesn't matter if getModel on the actual Action class is annotated or not, it will only ever check the interface method.

To be honest, I don't mind this behaviour - what if we just exempt all model driven Actions from the annotation requirement? The @StrutsParameter requirement was designed to protect against random getters/setters on the Action class from being injected. But if an Action is using a dedicated model object (and the Action class is indeed not on the root of the ValueStack) it is already much safer.

Here is my alternative solution for your consideration: f67f593

@lukaszlenart

Copy link
Copy Markdown
Member Author

I'm fine either way, my PR is just a starting point for discussion. Feel free to open PR and we can discuss further.

@kusalk

kusalk commented Oct 12, 2024

Copy link
Copy Markdown
Member

I've created #1072 which exempts ModelDriven Actions from the annotation requirement entirely - I've also retained your other changes, including the additional debug logging, with some minor adjustments

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