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

Improve page session attributes resolution #95

Merged
merged 4 commits into from
Oct 11, 2023

Conversation

avpinchuk
Copy link
Contributor

@avpinchuk avpinchuk commented Oct 9, 2023

This PR based on very detailed @OndroMih's comment.

What happens:

  • If pageSession explicitly requested, return it
  • Else if pageSession not found or doesn't contains property, return null
  • Else if property present in pageSession, check standard scopes (request, view, session and application) for an updated value
  • Else return value from our custom page session.

This generally emulates behavior as for Jakarta Server Faces 3.0 VariableResolver Chain Wrapper.

I reverted from store page session to view map, because view map cleared on an every call of the FacesContext.setViewRoot() and we cannot control each call of this method.

Signed-off-by: Alexander Pinčuk <alexander.v.pinchuk@gmail.com>
Signed-off-by: Alexander Pinčuk <alexander.v.pinchuk@gmail.com>
Signed-off-by: Alexander Pinčuk <alexander.v.pinchuk@gmail.com>
@avpinchuk
Copy link
Contributor Author

@arjantijms, @hs536, @OndroMih what you thinks?

@@ -97,20 +136,7 @@ public Class<?> getType(ELContext elContext, Object base, Object property) {

@Override
public void setValue(ELContext elContext, Object base, Object property, Object value) {
if (base != null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this work? How does this set a page context value? Before, this method expected base == null and property != null and set the property into the page context. This means I think that if a page set some variable (with no base), the variable was added to the page context.

The method after your change expects that base != null, which is different to the condition before. And it doesn't set anything in the page context. What happens if a page just defines a new variable (without base) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prior to Faces 4, PageSessionResolver was VariableResolver, not ELResolver and therefore did not setup any values.

All page session values are set with our custom handlers, e.g. setPageSessionAttribute.

Now we first try to read updated values, which come to us from standard scopes, and if its not found, read from page session map.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I understand, thanks :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@OndroMih, page session attributes are "read-only" from the point of view EL engine, imo. Here description from spec https://jakarta.ee/specifications/faces/3.0/jakarta-faces-3.0#a2773.

I tried to implement this part in the getValue() method

Get the head of the VariableResolver chain and call resolveVariable(facesContext, property) and return the result

by checking standard scopes before our custom page session scope. This should prevent us from reading an obsoleted values.

Updates of the values from requests handled by Faces ScopedAttributeELResolver.

@OndroMih
Copy link
Contributor

OndroMih commented Oct 9, 2023

Looks good to me.

Signed-off-by: Alexander Pinčuk <alexander.v.pinchuk@gmail.com>
@avpinchuk avpinchuk force-pushed the ccx-fix-el-resolution branch from 942359b to a3b890f Compare October 10, 2023 22:47
@avpinchuk avpinchuk added the bug Something isn't working label Oct 10, 2023
@avpinchuk avpinchuk self-assigned this Oct 10, 2023
@avpinchuk avpinchuk marked this pull request as ready for review October 10, 2023 22:50
@avpinchuk avpinchuk requested review from OndroMih, dmatej, arjantijms and hs536 and removed request for OndroMih October 10, 2023 22:50
@dmatej dmatej merged commit cc2e8b2 into eclipse-ee4j:master Oct 11, 2023
@dmatej dmatej modified the milestones: 4.0.1, 4.0.3 Oct 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants