A bookmark outlives the thing it points at

Background

portletDefinitionId + "_" + layoutNodeId + "_" + userId

?pCa=27_ctf9_142032&pP_27_ctf9_142032_implicitModel=...

The method that does that parsing is parsePortletParameterName. It takes a raw parameter name, and the set of entity ids the request declared, and returns a pair: the real parameter name, and the window the parameter belongs to. uPortal calls that pair a Tuple, whose fields are named first and second.

uPortal-rendering/.../url/UrlSyntaxProviderImpl.java:894

protected Tuple<String, IPortletWindowId> parsePortletParameterName(
        HttpServletRequest request, String name, Set<String> additionalPortletIds) {

Resolving the second value means turning 27_ctf9_142032 into a live window. That runs through two registries: a window registry, which asks an entity registry to resolve the id first. The entity registry splits the string, looks up the definition, and if the definition is not there, it throws.

uPortal-rendering/.../portlet/registry/PortletEntityRegistryImpl.java:757

if (portletDefinition == null) {
    throw new IllegalArgumentException(
            "No parent IPortletDefinition found for "
                    + portletDefinitionIdString
                    + " from entity id string: "
                    + consistentEntityIdString);
}

IllegalArgumentException is unchecked, so Java does not require any caller to handle it, and nothing did. It traveled up through URL parsing, out of the rendering pipeline, and into the filter that sits across every request.

Intuition

Before the change, one missing portlet definition cost the whole page. The user asked for their layout, and got an error page, because a parameter belonging to one removed portlet could not be resolved.

The whole layout is gone, not just the missing portlet. Nothing on the page told the user their bookmark was the problem.

The layout renders without the removed portlet, and the operator gets a warning naming the parameter. The user sees a working page.

The shape of the fix

The method already had an answer for "this parameter does not belong to any known window": return the parameter name with a null window id. That path existed for the case where none of the declared ids appear in the parameter name at all.

So the fix does not invent a new outcome. It catches the exception, logs it, and lets control reach that existing fallback.

The diagram below shows the simple case, where one id fails and the method falls through. The catch actually sits inside a loop over every declared id, so a failure is not always the end of the attempt. Step 3 of the walkthrough follows that, and it is where the detail lives.

What the caller receives

The same story with the timing made explicit, and both outcomes side by side.

sequenceDiagram
    autonumber
    actor U as Browser
    participant G as getPortalRequestInfo
    participant S as parsePortletParameterName
    participant R as portletWindowRegistry
    participant E as PortletEntityRegistryImpl
    participant F as ExceptionLoggingFilter

    U->>G: GET with pCa=27_ctf9_142032
    G->>S: name = pP_27_ctf9_142032_implicitModel
    S->>R: getPortletWindowId(request. "27_ctf9_142032")
    R->>E: getPortletEntity for definition 27
    E--)R: IllegalArgumentException. no definition 27
    R--)S: propagates unchanged

    rect rgb(251, 231, 233)
    Note over S,F: before. nothing catches it
    S--)G: propagates
    G--)F: propagates
    F->>F: logger.error. then rethrow
    F->>U: HTTP 500 via /500.html
    end

    rect rgb(228, 243, 234)
    Note over S,G: after. the catch logs and the loop moves on
    S->>S: logger.warn at line 913
    S->>S: no more ids match. fall to line 922
    S-->>G: Tuple("27_ctf9_142032_implicitModel". null)
    G->>G: null window id. drop or reattribute the parameter
    G->>U: HTTP 200. layout without the deleted portlet
    end

Code walkthrough

The flow, end to end. A browser sends a stale entity id. getPortalRequestInfo, the method that turns a request into parsed information, hands each prefixed parameter to parsePortletParameterName. That method asks the window registry, which asks the entity registry, which throws. What happens next is the change.

Four files: one fix, one test file, and two log lines in other modules.

1. Where the exception is born

The entity id has three parts, and the throw quoted in Background is what happens when the first of them, the definition id, does not resolve. That is the failure this change is about. Two details matter for reading the log.

First, a deleted portlet is not the only cause. The same lookup returns null, and so raises the same exception with the same message, when the definition exists but the user is not permitted to render it. A warning naming definition 27 does not by itself tell an operator which of the two happened.

Second, successful parses are cached, and failures are not. So a stale id pays the full lookup on every request that carries it, and logs a warning every time.

2. The 500 path, before

uPortal-web/.../web/ExceptionLoggingFilter.java:46

try {
    chain.doFilter(request, response);
} catch (Throwable t) {
    // ... builds a message with the URL, query string, user and IP
    this.logger.error(
            "uPortal: unhandled exception '" + t.getMessage() + "' " + msg.toString(), t);
    // ...
    if (t instanceof RuntimeException) {
        throw (RuntimeException) t;
    }

This filter is mapped to /*, so it saw the exception, and its name says what it does with it: it logs. Then it rethrows unchanged, the container turns an uncaught exception into a 500, and web.xml maps 500 to /500.html. Nothing in that chain was ever going to produce a partial page, because nothing in it knows that one parameter out of many is the problem. The only place with enough context to make that call is the parser.

3. The fix, and where control actually goes

uPortal-rendering/.../url/UrlSyntaxProviderImpl.java:908

+            try {
                 final IPortletWindowId portletWindowId =
                         this.portletWindowRegistry.getPortletWindowId(request, additionalPortletId);
                 return new Tuple<String, IPortletWindowId>(paramName, portletWindowId);
+            } catch (IllegalArgumentException e) {
+                this.logger.warn(
+                        "Failed to resolve portlet window id for parameter '{}': {}."
+                                + " This may be caused by a stale URL referencing a"
+                                + " removed portlet. The parameter will be ignored.",
+                        name,
+                        e.getMessage());
             }

Read what the catch block does not contain. No return, no break, no continue, no rethrow. It logs, and then control reaches the end of the loop body.

That matters, because this code sits inside a for loop over every entity id the request declared. So a failure is not the end of the attempt. The loop advances to the next id, and if a later one both matches the parameter name and resolves, line 911 returns a real window after all. Only when no iteration returns does control leave the loop and reach the fallback.

Row three is the case the fallback handles on its own, and there the two strings agree, because a parameter with no id in it loses nothing when only the prefix is stripped. Row two reaches the same line with a name that does contain an id. That is the edge case from Intuition, seen from the code side.

4. The pattern it says it matches

The pull request describes the new catch as matching an existing one in the same class. That existing catch is real, and it is worth comparing precisely.

uPortal-rendering/.../url/UrlSyntaxProviderImpl.java:945

            try {
                return this.portletWindowRegistry.getPortletWindowId(request, portletWindowIdStr);
            } catch (IllegalArgumentException e) {
                this.logger.warn(
                        "Failed to parse portlet window id: "
                                + portletWindowIdStr
                                + " null will be returned",
                        e);
            }
        }

        return null;

Same shape: try the registry call, catch the same exception type, log at warn, fall through to a no-window result. Three differences a reader should see.

The older one is single-shot, guarded by a contains check rather than sitting in a loop, so it can log at most once. The newer one is inside the loop, so one parameter with several declared ids can log several times.

The older one returns a bare null. The newer one returns a non-null Tuple whose second is null, which is why the caller has a null check rather than a null result to handle.

The older one passes the exception object e to the logger, so the stack trace is recorded. The newer one passes e.getMessage(), so the log carries the message without the trace. That is the difference between knowing what failed and knowing where.

5. What the caller does with a null window

Grepping the repository for parsePortletParameterName finds one production caller, getPortalRequestInfo in the same class, plus the new test. When second comes back null the caller takes one of two paths, and neither one throws.

If no portlet is targeted by the request, it logs its own warning, saying the parameter will be ignored, removes the parameter, and breaks. If a portlet is targeted, the parameter is attributed to that portlet under the name in first. For the stale case that name still carries the entity id, so the targeted portlet is handed a parameter it does not read.

Either way, parsing completes, rendering proceeds, and the user gets their layout minus the removed portlet.

6. The tests, and what they cover

Two tests are added. Both stub the registry to throw the real message, and both assert that second is null rather than that an exception escapes.

uPortal-webapp/.../url/UrlSyntaxProviderImplTest.java:1059

    public void testParsePortletParameterNameWithInvalidPortletEntity() {

That one covers the new catch. The second, at line 1088, exercises parsePortletWindowIdSuffix, the sibling method whose catch already existed and which this diff does not touch. It changes no behavior; it records the reference the new code was written to match.

Note what the first test asserts and what it does not. It asserts that second is null. It does not assert the value of first, so the difference between implicitModel and 27_ctf9_142032_implicitModel from step 3 is not pinned by a test.

7. Thirty tests that were not running

The pull request describes itself in three bullets: the try/catch, the two log lines, and "Existing UrlSyntaxProviderImplTest updated with test for stale portlet entity IDs". The words @Ignore, MockitoJUnitRunner, and Silent do not appear in it. So the two lines below are in the diff without being in the description.

uPortal-webapp/.../url/UrlSyntaxProviderImplTest.java:53

-@Ignore // Breaks on move to Gradle
-@RunWith(MockitoJUnitRunner.class)
+@RunWith(MockitoJUnitRunner.Silent.class)
 public class UrlSyntaxProviderImplTest {

@Ignore on a JUnit 4 class skips the entire class. Not one method ran, and the setup methods did not run either. The class holds 32 @Test methods at this commit, two of them new, so 30 tests had been reporting as skipped rather than passing. A green build said nothing about this file.

The comment gives the reason as // Breaks on move to Gradle, which names a build migration and not a defect in the code under test. When the annotation landed is not something this page can tell you, because the checkout it was written from holds a single commit.

The runner change is what switching them back on required. Plain MockitoJUnitRunner is an alias for MockitoJUnitRunner.Strict: after the class runs, Mockito collects any when(...) stubbing that was never actually called and fails the class with UnnecessaryStubbingException. Silent runs with lenient strictness, which switches off that detection and leaves verify behavior untouched. This project pins Mockito 4.11.0 and JUnit 4.13.2.

The file shows what that detection would have caught. In one URL-generation test, three stubbings sit together on portletWindow2, which stands in for a delegate window:

uPortal-webapp/.../url/UrlSyntaxProviderImplTest.java:421-423

        when(portletWindow2.getPortletEntity()).thenReturn(portletEntity2);
        when(portletWindow2.getDelegationParentId()).thenReturn(portletWindowId1);
        when(portletEntity2.getLayoutNodeId()).thenReturn(subscribeId2);

The middle one is used: generateUrl reads getDelegationParentId() for a non-targeted window. The other two are not. generateUrl reads getPortletEntity() and getLayoutNodeId() only inside its targetedPortletWindowId != null branch, and portletWindow2 is not the targeted window here. So two of the three stubbings are never realized, and strict mode fails the whole class on them, not the one test. The same two appear again in another generation test at line 546.

8. Two log lines in other modules

The remaining two files are unrelated to stale URLs. Each lowers one log level. The frequencies below say how often each line fires, which is the fact you need to judge the edits.

uPortal-security/.../url/ClassicMaxInactiveStrategy.java:47

-            log.info(
+            log.debug(
                     "No {} permissions apply to user '{}'",

This runs in a filter mapped to the portal's main servlets, and the filter re-checks at most once every five minutes per session. The branch it sits in is the state of any deployment that never grants the permission. At INFO, such a deployment logs this line once per authenticated session per five minutes.

uPortal-utils/.../personalize/PersonalizerImpl.java:110

-                log.warn("Person attribute value is not a string!! : [{}]", key);
+                log.debug("Person attribute value is not a string : [{}]", key);

This one sits in the else branch of a loop over every attribute of a person, so it repeats per attribute, not per event. One call site passes no session, which skips the cache and re-runs the whole loop, re-logging it, for every personalized string. It was at WARN, so it appeared among genuine faults.

Two smaller things in the same hunk. The message text changed: the doubled exclamation marks are gone. And the pull request describes both log edits as going "from info to debug", which is right for ClassicMaxInactiveStrategy and not for this one, which was at WARN. Worth knowing if you are reading the description rather than the diff.

One piece of context a reader of this repository should have. uPortal keeps a file called AGENTS.md at its root, holding the conventions it expects contributors and coding agents to follow. Its surgical-changes section says "Touch only what the task requires. Do not 'improve' adjacent code, comments, or formatting" and "Every changed line must trace directly back to the task at hand". Its boundaries section lists "A change would affect more than one module" under when to always stop and ask. These two edits are in two modules that have nothing to do with stale URLs. Whether that reads as welcome cleanup or as unrelated scope is a call for this project's reviewers; the page's job is to make sure you noticed the edits are there.

Quiz

Five questions about why the change is shaped the way it is. Click an option to see the answer.