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

Fix Issue #4265 - New keywords not available in f:ajax #4274

Merged
merged 3 commits into from Feb 4, 2018

Conversation

Projects
None yet
5 participants
@Toberumono

Toberumono commented Aug 30, 2017

This fixes #4265. The fix is fairly minimal. Theoretically, depending on how often some of the new keywords are used (i.e. @parent), additional static arrays might be useful.

Note: This pull request is the corrected version of Pull request #4266.

@xinyuan-zhang

This comment has been minimized.

Contributor

xinyuan-zhang commented Aug 31, 2017

Hi @arjantijms what do you think of that?

@arjantijms

Sorry for taking so long, this review only just now bubbled up to the top of my list.

After a look at the issue and after discussing it with @BalusC, it's clear that this should indeed be fixed, but with the current fix the user is not informed about genuine mistakes, i.e. type "@Thiz", which can be confusing as well.

So I'd suggest iterating over the SearchKeywordResolver and checking with each of them if they can resolve each keyword, but since this is an qrs problem (every tag, every keyword, every resolver), I'd suggest doing this check only in dev mode.

Additionally, did you sign the OCA? This is a requirement from Oracle for PRs to be accepted.

@Toberumono

This comment has been minimized.

Toberumono commented Sep 19, 2017

Unless I am missing something, at present there is no API method for getting the loaded SearchKeywordResolvers, nor is there an API method for determining whether a SearchKeywordResolver that can resolve a given keyword exists. Furthermore, unless I misread the SearchKeywordResolver specification, it is possible for a SearchKeywordResolver instance to change what keywords it resolves based on the SearchExpressionContext (i.e. a SearchKeywordResolver that loads additional resolvers from the Request, View, or Session scopes), therefore, we cannot conclusively determine whether a keyword exists before the final evaluation of the expression. Thus, any warning would require us to assume the use of ApplicationImpl and would be prone to false positives.

I'm assuming that we cannot add an API method particularly quickly, so do you have a workaround for the above issues in mind?

My boss submitted the OCA for the company today, so I should be set on that front.

@BalusC

This comment has been minimized.

Collaborator

BalusC commented Sep 22, 2017

Perhaps you took "iterating over the SearchKeywordResolver" in Arjan's comment too literally. This can be done as below:

boolean validKeyword = facesContext
    .getApplication()
    .getSearchKeywordResolver()
    .isResolverForKeyword(searchContext, keyword);

Where keyword string doesn't have the @ prefix. E.g. this, parent, thiz, etc. It will internally iterate over all registered search keyword resolvers in documented order until one returns true.

Basically, the old AjaxBehavior class just needs to catch up with the new JSF 2.3 feature.

@Toberumono

This comment has been minimized.

Toberumono commented Sep 22, 2017

@BalusC Thank you for the pointer. I did spend a bit more time than necessary on the iteration part. Unfortunately, the latter issue that I raised (the false positives one) does still apply. A false positive can be created with a pretty simple setup. The relevant code is as follows (I'd ordinarily go with clientId rather than put the components in a map, but, using the component makes for shorter code here):

public class ViewScopedSearchKeywordResolver extends SearchKeywordResolver {

    @Override
    public boolean isResolverForKeyword(SearchExpressionContext searchExpressionContext, String keyword) {
        Map<String, Object> viewMap = FacesContext.getCurrentInstance().getViewRoot().getViewMap();
        Map<String, UIComponent> viewScopedKeywords = (Map<String, UIComponent>) viewMap.get("viewScopedKeywords");
        return viewScopedKeywords != null && viewScopedKeywords.containsKey(keyword);
    }

    @Override
    public void resolve(SearchKeywordContext searchKeywordContext, UIComponent current, String keyword) {
        Map<String, Object> viewMap = FacesContext.getCurrentInstance().getViewRoot().getViewMap();
        Map<String, UIComponent> viewScopedKeywords = (Map<String, UIComponent>) viewMap.get("viewScopedKeywords");
        searchKeywordContext.invokeContextCallback(viewScopedKeywords.get(keyword));
    }
}

Create a JSF tag (for this example, we'll go with foo:bar) and, in a render method for foo:bar method for some tag (we'll go with encodeEnd in this case, but the precise method doesn't actually matter - it just has to be a rendering-phase method), do something like this:

public void encodeEnd(FacesContext ctx) throws IOException {
    super.encodeEnd(ctx);

    Map<String, Object> viewMap = FacesContext.getCurrentInstance().getViewRoot().getViewMap();
    Map<String, UIComponent> viewScopedKeywords = (Map<String, UIComponent>) viewMap.get("viewScopedKeywords");
    if (viewScopedKeywords == null) {
        viewMap.put("viewScopedKeywords", viewScopedKeywords = new HashMap<>());
    }
    viewScopedKeywords.put("foobar", this);
}

And, in a JSF page:

<!-- Some tags -->
<h:commandButton value="Click me!">
	<f:ajax render="@foobar" /> <!-- This will say that @foobar is an invalid keyword -->
</h:commandButton>
<foo:bar ...>
	<!-- Some tags can go here - it doesn't actually matter -->
</foo:bar>
<!-- Some more tags -->

Note that the actual tags don't matter - only that the f:ajax tag is before the foo:bar tag in the tree.

When you load the page, the f:ajax tag will read in the render attribute, and immediately try to validate it. The validation will be unable to find the @foobar keyword because it does not exist at that point. However, because the component tree is built before components are targeted for execution and rendering, by the time the components identified by the render attribute are resolved, the keyword will be valid.

@BalusC

This comment has been minimized.

Collaborator

BalusC commented Sep 23, 2017

I'm not sure. Keywords are in first place intended to be applicationwide constants and not variable. This should perhaps be better documented. I'll review API doc on this later.

Joshua Lipstone
Merge branch 'MOJARRA_2_3X_ROLLING' into fix-2.3-keywords-not-availab…
…le-in-execute-render-rolling

* MOJARRA_2_3X_ROLLING:
  Let's start work for release 2.3.4-SNAPSHOT
  release cut 2.3.3
  #4270 Removed forgotten 2.3 switch based on faces-config
  #4276 Updated pom.xml files so project compiles again with Maven
@tandraschko

This comment has been minimized.

tandraschko commented Dec 3, 2017

To fix that problem, the fix is enough.
The statics are just a performance shortcut to skip new list instances for the common keywords. Of course you could create new statics for each keyword but the benefit is minimal and just a improvement.

@tandraschko

This comment has been minimized.

tandraschko commented Dec 3, 2017

PLUS: the searchexpressionresolver will be called when rendering, so there should be already a validation.

@BalusC BalusC merged commit 5de9622 into javaserverfaces:MOJARRA_2_3X_ROLLING Feb 4, 2018

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