Click fork: half the upstream imports were never repointed, so type checks in ContainerUtils and Form can never match

[vharseko]

Rewritten after a full sweep of the fork. The original report named two sites in Form.java; the root cause turns out to be systematic, so the scope below is the whole pattern. The javax-bound half is split out into a separate issue — see the end.

org.openidentityplatform.openam.click is a copy of Apache Click 2.3.0 taken in-tree so the framework could be moved from javax.servlet to jakarta.servlet. During that copy, imports were repointed at the fork class by class, and only about half were done.

Measured over the 57 files of the fork:

	count
import org.apache.click.* statements	60
— used in code	40
— appearing only in javadoc (@see, {@link})	20
distinct upstream classes imported	32
— a class of the same name already exists in the fork	16
— no fork equivalent exists	16

util/ContainerUtils.java:38-49 shows the pattern in a single import block: Control, Container, Field and Form are the fork's, while Button, Label, FieldSet, LogService, ClickUtils and HtmlStringBuffer are still upstream.

Why the mixed imports break behaviour

The fork's Button, Label and ActionLink extend the fork's Field → AbstractControl → Control. They have no relationship to the upstream classes of the same name. Container.getControls() returns List<Control> of the fork Control, and add(Control) accepts only that type, so an upstream control can never be inside a fork container.

Every instanceof against an upstream type over that collection is therefore a constant false, and every guard built on one silently does nothing.

Filters that never fire

Site	Check	Consequence
ContainerUtils:1179 addInputFields()	instanceof Label || instanceof Button	buttons and labels are not filtered out and are returned by getInputFields(), so they reach Form.validate() (Form:1141, :1357), copyContainerToObject() and getFieldMap() as if they were input fields
ContainerUtils:1209 addHiddenFields()	same	same, for getHiddenFields() (Form:2447)
ContainerUtils:1246 addFieldsAndLabels()	instanceof Button	same
ContainerUtils:1303 addErrorFields()	instanceof Button	same
ContainerUtils:1130, :1136 addButtons()	instanceof Button	getButtons() always returns an empty list
ContainerUtils:553 insert()	!(control instanceof Label)	the exemption that lets several labels share a name never applies, so insert() throws IllegalArgumentException where upstream allowed the add
ClickUtils:1454-1455 getCssSelector()	instanceof ActionLink	a fork ActionLink takes the else arm and gets a tag[name=…] selector, although the comment right above explains that links do not render a name attribute — the selector cannot match

The same logic in Form.addStatefulFields:3051 — control instanceof Label || control instanceof Button — is correct, because Form.java does not import the upstream Label/Button and they resolve inside the fork's own package. Two files carrying the same code behave differently; that is the signature of the half-finished repoint.

Branches that can never be taken

• Form:3061 — control instanceof org.apache.click.control.Container, while the cast on the next line is to the fork Container. The recursion is dead, so addStatefulFields() does not keep the promise in its javadoc to "recursively include any Fields contained in child containers", and getState()/setState() (Form:1887, :1915) skip every field nested in a child container. Had the condition ever matched, the cast would throw ClassCastException.
• Form:2526-2527 (renderControls()), Field:315, :747 (isDisabled(), isReadonly()), ContainerUtils addInputFields/addHiddenFields — all test instanceof FieldSet against the upstream class. The fork ships no FieldSet at all, so the <td> styling and the parent-walk never run.

Reachability

None of this is a live defect today. No Click Form is constructed anywhere in the product — the only HiddenField instances are the framework's own NonProcessedHiddenField inside Form.java itself — and the pages the fork's servlet serves (the configurator and upgrader) create no controls. See #1109 for the full argument. The configurator works because it exercises the one path that was repointed completely.

Suggested scope

1. Repoint the 16 upstream imports that already have a fork class of the same name: ActionLink, Button, Label, Decorator, AbstractContainer, ClickUtils, HtmlStringBuffer, ConfigService, LogService, FileUploadService, ClickResourceService, VelocityTemplateService, DefaultMessagesMapService, CssStyle, JsImport, JsScript. This is mechanical and fixes every row of the table above.
2. Decide what to do about FieldSet: add a fork class or delete the branches and the import.
3. Fix Form:3061 to name the fork Container, matching the cast below it.
4. Drop the 20 javadoc-only imports that point at classes the fork does not ship (Submit, Reset, Checkbox, Panel, …), or repoint the ones that do have an equivalent.
5. Regression cover: getInputFields() and getButtons() over a container holding a fork Button and Label give a directly assertable result. There is currently no test anywhere under openam-core/src/test/java/org/openidentityplatform/openam/click/ except the one added by Remove the Java serialization round trip from the bundled Click HiddenField #1109.
6. Closing step: with the imports repointed, drop org.apache.click:click-nodeps:2.3.0 from openam-core. It cannot be removed before then — the fork imports 32 classes from that jar and does not compile without it.

Out of scope here

The other 16 upstream classes have no fork equivalent, and three of them (Format, MessagesMap, and the calls to org.apache.click.util.ClickUtils.getLogService()) reach upstream code that is bound to javax.servlet, which is not on the classpath. That is a different failure mode — NoClassDefFoundError rather than a branch quietly not taken — and needs its own fix. Tracked separately.

[vharseko]

Likely superseded by #1062, which migrates the configurator/upgrade wizard off Apache Click and deletes all 57 files of the org.openidentityplatform.openam.click fork, along with click-nodeps and click-extras in openam-core/pom.xml, the click.version property and the OSGi export line in the root pom. I checked every one of the 16 places in master that reference the fork — 12 com.sun.identity.config.* classes, both web.xml files, Upgrade.java and the root pom — and #1062 covers all of them.

Every instanceof listed above lives in a file that PR removes, and the closing step suggested here — dropping the Click dependency — is done there too.

Not closing yet: #1062 is approved and mergeable but not merged. Worth holding off on the repoint work described above, though, since it would be thrown away if that PR lands.