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

MYFACES-4695: Allow searching for ids with separator character [5.0] #806

Merged
merged 2 commits into from
Nov 22, 2024

Conversation

volosied
Copy link
Contributor

No description provided.

@volosied volosied requested a review from tandraschko November 20, 2024 21:28
@tandraschko
Copy link
Member

We need a unittest for this case before applying here something

@volosied
Copy link
Contributor Author

I'll add it next. Just making sure the change looks okay and the existing tests pass.

@volosied volosied changed the title MYFACES-4695: Allow searching for ids with separator character MYFACES-4695: Allow searching for ids with separator character [5.0] Nov 20, 2024
@volosied
Copy link
Contributor Author

Added a test -- without my fix it fails with

[ERROR] Errors: 
[ERROR]   SearchExpressionImplTest.testMyFaces4695:423 » ComponentNotFound Cannot find component for expression "form1:one" referenced from "form2:submit".
[INFO] 

@tandraschko
Copy link
Member

i dont think the case is valid, it needs to be
<f:ajax render=":form1:one" />

@volosied
Copy link
Contributor Author

volosied commented Nov 21, 2024

I see this as more of a convenience.

The spec describes the search algorithm here: https://jakarta.ee/specifications/faces/4.1/apidocs/jakarta.faces/jakarta/faces/component/search/searchexpressionhandler

However, it also states "The search algorithm must operate as follows, though alternate algorithms may be used as long as the end result is the same:"

If the component can't be found, then it will throw an exception (due to your change), but, in my opinion, it should keep searching, if possible, to find the component.

As for the code, we do a bottom - up search (see this line)

if we can't find the component, should we use try from the root instead?

Let me know what you think. Thanks

@tandraschko
Copy link
Member

its clearly described in the specs:
Otherwise, search up the parents of this component. If a [NamingContainer](https://docs.oracle.com/javaee/7/api/javax/faces/component/NamingContainer.html) is encountered, it will be the base.

https://docs.oracle.com/javaee/7/api/javax/faces/component/UIComponentBase.html#findComponent-java.lang.String-

@tandraschko
Copy link
Member

tandraschko commented Nov 21, 2024

and its like this for ages, in both JSF impls and also in the past in the native PF expressions
either we dont support it or we add a config, disabled per default, to allow this

@volosied
Copy link
Contributor Author

One more data point -- this previously worked in 2.2, but then changed in 2.3 due to addition of the SearchExpressionHandler API.

@tandraschko
Copy link
Member

@tandraschko
Copy link
Member

im really happy that both myfaces and mojarra uses almost the exact same impl for this very old topic and that we could even remove the native PF expressions

if add such a hack again, we should make it disabled per default, just to allow users to migrate their old app
but still then... its not really a big change to fix their expressions

@tandraschko
Copy link
Member

Can you Test it in mojarra?

@volosied
Copy link
Contributor Author

volosied commented Nov 21, 2024

Mojarra 4.1.2 works without needing the prepended : -- it finds the component.

@tandraschko
Copy link
Member

Then let me check it
In mojarra they use a invokeOnComponent Fallback, Leo optimized the algorithm in mf

@tandraschko
Copy link
Member

+1
just proceed

@volosied volosied merged commit 8a6d0bb into apache:main Nov 22, 2024
4 checks passed
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