Problems with JSF views when upgrading to Payara 5

[poikilotherm]

This is a story for #4172, which seems to become an epic on the road to (promised?) Payaraland.
This is heavily inspired by #5907 and its PR #5913/#5933 plus PR #5940 and sort of a successor to it (@pdurbin already thought about closing #5907)

---

Description / Context

Digging into causes of problems (example see below) pretty quickly leads to upstream problems of the past, outrunning devs now... Once upon a time, a setting related to coercing from "" to null named javax.faces.INTERPRET_EMPTY_STRING_SUBMITTED_VALUES_AS_NULL lived happily ever after in web.xml. This is set to true since dwarfs battled elves (d6868ad) in the good old days. Background: you want to have this setting set to true to avoid saving empty strings or non-null values in your database, when null would be appropriate.

In 2014/2015 a mighty bug prince, called "String madness" brought chaos, blood and gore to beautiful land of Java JSF. Anyone using the above settings had no sharp swords in their hands, but plowshares. The prince used "Expression Language" to transfrom null back to "" right away.

Things to be noted here:

1. Glassfish 4.1 in use for Dataverse ships javax.el v3.0.1-b03
2. Things got fixed (the prince defeated) with javax.el v3.0.1-b05
3. Payara 5.192 ships javax.el v3.0.1-b11

So we are still infected with the prince madness in 2019, but undiscovered, devs worked around it and lived with it. Using fixed versions in Payara 5 breaks things violently. And there are yet more to come, when moving on.

Example problem

Let me give you an example what these breaking code pieces looks like. (Kudos to @pdurbin for finding this bug.)
On the start page of Dataverse (dataverse.xhtml), a search is done to show the root datasets and dataverses.
This view fails with an error 500 on Payara 5 (kudos to @pdurbin for the images):

This error is caused by page = 0 in SearchIncludeFragment.search(), caused by an empty GET parameter "page", being mapped to SearchIncludeFragment.page:

dataverse/src/main/webapp/dataverse.xhtml

Lines 41 to 43 in 2819ce3

	<f:viewParam name="page" value="#{SearchIncludeFragment.page}"/>
	<f:viewParam name="debug" value="#{SearchIncludeFragment.debug}"/>
	<f:viewAction action="#{SearchIncludeFragment.search()}" />

Behind the curtains, page is null first when the request is sent, gets converted to int by EL and by spec null maps to primitive 0. (See section 1.23.3 of EL 3.0 spec). So in current versions, things are moving along as specified and thus it fails.

javax.el-3.0.1-b03 shipped with GF 4.1 is known buggy for empty strings. It seems that there is a bug in mapping primitives, too: the value is not mapped at all, but int page = 1; is providing the required value of 1.

• Proof: adding ?page=1 to the GET request results in a working page on Payara 5.
• Addendum: maybe Majorra JSF 2.2.7 contains bugs, too. It is not totally clear when EL kicks in, as JSF should do conversion, too. And this no-mapping to primitive should not happen, too.

TODO

1. Better safe then sorry: test if it fails on GF 4.1, too, when replacing the EL library with a patched one.
2. If things still do not fail, replace Mojarra with a more recent one (at least there are releases up to 2.2.19)
3. Think about how to refactor to be spec-compliant, but keep things working for now on GF 4.1 (maybe by adding instructions to replace JARs as currently done for WELD and Grizzly.)

If more in-depth debugging is needed, we might need to attach things to JSF phases.

REFACTOR

Devs need to think about how to deal with empty GET view parameters. Different scenarios are possible. As I am no JSF expert, I do not know what practice would be "best practice".

Before talking about options, some general thoughts:

1. Obviously, this will have to happen for each and every view, form, etc.
2. Adding validation would help in locating bugs and enhance security. E. g. having a valid range "1-n" for the page parameter would have made it much clearer, where things break. Currently at least for SearchIncludeFragment I don't see sanitation/validation in place. Trusting user input without validation is definitly considered bad practice (at least in the PHP world...).

Option A:

Identify critical parameters, use <o:viewParam> instead of <f:viewParam> and provide a default, like done for #5933

Option B:

Handle things in code (1/2). Understand the spec rules, apply catching logic and modify. Example in #5940.

Option C:

Avoid primitives as parameters, use object types instead. Handle things in code (2/2): do proper null handling.
(I'll vote for this one...)

Of course one is free to mix'n'match the options.

[poikilotherm]

I think this is done with #6230 and recent updates.