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

[Scope]Map#containsKey() doesn't check if key is present but only if value is non-null #5530

Closed
BalusC opened this issue Nov 16, 2024 · 3 comments

Comments

@BalusC
Copy link
Contributor

BalusC commented Nov 16, 2024

And this is incorrect.

Observed in ApplicaitonMap, InitParameterMap, RequestHeaderMap, RequestHeaderValuesMap, RequestParameterMap, RequestParameterValuesMap and SessionMap.

E.g. RequestMap:

    @Override
    public boolean containsKey(Object key) {
        return request.getAttribute(key.toString()) != null;
    }

should basically have been

    @Override
    public boolean containsKey(Object key) {
        return Collections.list(request.getAttributeNames()).contains(key);
    }

The only scope Map which has containsKey() correctly implemented is ELFlash.

There's however a potential performance penalty with this change in case the map has a thousand of attributes or so and thus this needs to be carefully evaluated.

@pizzi80
Copy link
Contributor

pizzi80 commented Nov 20, 2024

The performance problem is primarily caused by the Servlet specification
of request.getParameterNames()
which returns an Enumeration<String> which is a superseded Java interface
which is also conceptually and technically wrong.

It should return a Set<String> or at least a Collection<String>
or a more modern SequencedCollection<String> (Java 21)

In this way the Faces' [Scope]Map.containsKey implementation
could be as simple as request.getParameterNames().contains(param)

Another solution is simply to add the relevant methods
in the Servlet spec:

HttpServletRequest
public boolean hasParameter(String name)

HttpSession
public boolean hasAttribute(String name)

[...]

under the hood the Servlet Container
can do this check in O(1) with a 1 line of code
because it will probably be backed internally again by a Map

In the meantime the Faces remediation plan could be:

context.getExternalContext().getRequestParameterMap().containsKey("paramName");
for the request

And for the other scopes cache the Enumeration<String> in a Set

@pizzi80
Copy link
Contributor

pizzi80 commented Nov 22, 2024

After a second read of this Tomcat ticket

https://bz.apache.org/bugzilla/show_bug.cgi?id=69444

Mark Thomas said:

From the setAttribute() Javadoc:
"If the object passed in is null, the effect is the same as calling {@link #removeAttribute}."
I think we are going to need to set it explicitly to the empty String.

if this is a standard behaviour, the actual implementation could be safe enough... ?

@Override
public boolean containsKey(Object key) {
    return request.getAttribute(key.toString()) != null;
}

@BalusC
Copy link
Contributor Author

BalusC commented Nov 22, 2024

Indeed. Thanks!

@BalusC BalusC closed this as completed Nov 22, 2024
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

2 participants