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

4.0.0: Bean Validation doesn't work in composite components #5171

Closed
NicolaIsotta opened this issue Nov 12, 2022 · 9 comments
Closed

4.0.0: Bean Validation doesn't work in composite components #5171

NicolaIsotta opened this issue Nov 12, 2022 · 9 comments

Comments

@NicolaIsotta
Copy link
Contributor

Describe the bug
Bean validation is not called for values of composite components.

To Reproduce
myComposite.xhtml:

<!DOCTYPE html>
<html xmlns="http://www.w3.org/1999/xhtml"
	  xmlns:cc="http://xmlns.jcp.org/jsf/composite"
	  xmlns:h="http://xmlns.jcp.org/jsf/html">

	<cc:interface>
		<cc:attribute name="value"/>
	</cc:interface>

	<cc:implementation>
		<h:inputText id="input" value="#{cc.attrs.value}" rendered="#{cc.rendered}" label="Composite input"/>
	</cc:implementation>
</html>

index.xhtml:

<!DOCTYPE html>
<html xmlns="http://www.w3.org/1999/xhtml"
	  xmlns:h="http://xmlns.jcp.org/jsf/html"
	  xmlns:ezcomp="http://xmlns.jcp.org/jsf/composite/ezcomp">
	<h:head/>
	<h:body>
		<h:form id="form1">
			<div>
				<h:outputLabel for="composite" value="Composite"/>
				<ezcomp:myComposite id="composite" value="#{index.string1}"/>
				<h:message for="composite:input"/>
			</div>
			<div>
				<h:outputLabel for="simple" value="Simple"/>
				<h:inputText id="simple" label="Simple input" value="#{index.string2}"/>
				<h:message for="simple"/>
			</div>
			<h:commandButton value="Submit"/>
		</h:form>
	</h:body>
</html>

Index.java:

package it.emmeduei.mojarra.error;

import jakarta.enterprise.context.RequestScoped;
import jakarta.inject.Named;
import jakarta.validation.constraints.NotEmpty;

@Named
@RequestScoped
public class Index {

	@NotEmpty
	private String string1;

	@NotEmpty
	private String string2;

	public String getString1() {
		return string1;
	}

	public void setString1(String string1) {
		this.string1 = string1;
	}

	public String getString2() {
		return string2;
	}

	public void setString2(String string2) {
		this.string2 = string2;
	}
}

Just press the button to submit the form
NB: I have a reproducer project if needed

Expected behavior
For both inputs validation should be performed. Instead, only the simple input is validated.

Screenshots
mojarra-error

Additional context
This seems to be a 4.0.0 regression, using 3.0.2 the validation is performed correctly.
Tried to debug, I found out that here in BeanValidator.validate valueReference is null:

ValueReference valueReference = getValueReference(context, component, valueExpression);
if (valueReference == null || valueReference.getBase() == null) {
return;
}

Maybe the cause is the missing override of getValueReference() in com.sun.faces.facelets.el.ContextualCompositeValueExpression?

@NicolaIsotta NicolaIsotta changed the title Bean Validation doesn't work in composite components 4.0.0: Bean Validation doesn't work in composite components Nov 12, 2022
@NicolaIsotta
Copy link
Contributor Author

Overriding getValueReference is not enough, because later it fails in BeanValidator.isResolvable.
I had to bring back ValueExpressionAnalyzer from 3.x to make validation work again.

@melloware
Copy link
Contributor

@NicolaIsotta did you want to submit a PR for review?

@NicolaIsotta
Copy link
Contributor Author

Well I'm not sure bringing back ValueExpressionAnalyzer is the right way, guess they cut it from 4.0 for a good reason?
Maybe @BalusC can enlighten us.
Also I think this needs a test, it's a pretty unpleasant regression (we found unvalidated data in the production db...)
I'll share if I discover anything else.

BalusC added a commit to jakartaee/faces that referenced this issue Nov 13, 2022
@BalusC
Copy link
Contributor

BalusC commented Nov 13, 2022

Reproduced. I've added an IT.

BalusC added a commit that referenced this issue Nov 13, 2022
the ValueExpressionAnalyzer from 3.0
@BalusC
Copy link
Contributor

BalusC commented Nov 13, 2022

Ok, the IT passed but the TCK failed on faces22/cdiMethodValidation .. I'll have to take a second look later.

@BalusC
Copy link
Contributor

BalusC commented Nov 13, 2022

Hmm .. it was intermittent .. all tests passed now .. PRs created nonetheless.

@arjantijms
Copy link
Contributor

Well I'm not sure bringing back ValueExpressionAnalyzer is the right way, guess they cut it from 4.0 for a good reason?

Well, to reduce the overhead of maintaining our own code, in 4.0 we have been replacing code with standard alternatives where available.

Maybe we could / should analyse why exactly the standard version doesn't work correctly, and whether that may be a bug in the expression language implementation library or the spec.

BalusC added a commit that referenced this issue Nov 15, 2022
Further improved by adjusting ContextualCompositeValueExpression so we
can get rid of the ValueExpressionAnalyzer
@BalusC
Copy link
Contributor

BalusC commented Nov 15, 2022

@NicolaIsotta has already figured out it. PR has been updated.

@BalusC
Copy link
Contributor

BalusC commented Nov 16, 2022

Merged now.

@BalusC BalusC closed this as completed Nov 16, 2022
jasondlee pushed a commit to jboss/mojarra that referenced this issue May 18, 2023
the ValueExpressionAnalyzer from 3.0
jasondlee pushed a commit to jboss/mojarra that referenced this issue May 18, 2023
Further improved by adjusting ContextualCompositeValueExpression so we
can get rid of the ValueExpressionAnalyzer
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

No branches or pull requests

4 participants