Name the parameter when a websocket pageId is not a number

[17krishna8]

The websocket processor constructor throws NumberFormatException for a pageId that is not a number, with no hint at what was wrong with the request.

Keep failing the handshake, but throw an IllegalArgumentException that names the parameter, like the one thrown when neither pageId nor resourceName is given at all:

java.lang.IllegalArgumentException: Request parameter 'pageId' must be a number

[reiern70]

Thanks for looking at this, but the change doesn't do what the description says, and I'd rather keep the current behaviour.

NO_PAGE_ID (-1) doesn't mean "no page", it means the endpoint is a WebSocketResource (see the constant's javadoc). With pageId=NaN and no resourceName:

• getPage() returns a new WebSocketResourcePage for -1, so a page is found;
• the connection is registered under PageIdKey(-1) and stays open, it is not rejected;
• every event on it, the connect and each message, ends in WebSocketMessageBroadcastHandler creating new SharedResourceReference(null), which throws IllegalArgumentException: Argument 'name' may not be null.

The test in the PR stops at the constructor, so it doesn't get that far. Going on to onOpen() and onMessage() shows it:

TestProcessor processor = new TestProcessor(request, tester.getApplication());
processor.onOpen(new Object());
// registered rather than rejected:
assertNotNull(WebSocketSettings.Holder.get(application).getConnectionRegistry()
	.getConnection(application, sessionId, new PageIdKey(-1, null)));
processor.onMessage("hello"); // IllegalArgumentException inside the broadcast

On master the constructor throws NumberFormatException, which is an IllegalArgumentException, the same exception the constructor already throws on purpose when neither pageId nor resourceName is given. The handshake fails in onOpen and the container closes the socket, which is the right outcome for a request that names no valid endpoint. This PR replaces that with an open connection in a state the constructor otherwise rules out: no page id and no resource name.

If the goal is a clearer failure, I'd be glad to take a change that keeps the exception and only improves its message, e.g. throw new IllegalArgumentException("Request parameter 'pageId' must be a number");.

[reiern70]

A page is found: for NO_PAGE_ID getPage() returns a WebSocketResourcePage, and the connection is registered under PageIdKey(-1).

[reiern70]

This only checks that the constructor returns; what the connection does shows once onOpen() and onMessage() are called.

[reiern70]

The license header lost the leading space on each line ( * Licensed to ...); please copy it from a neighbouring file.

[17krishna8]

Thanks for walking me through what actually happens after the constructor - I had stopped too early. I did not know about WebSocketResourcePage.

I have reworked the change: the handshake still fails when pageId is not a number, but the constructor now throws IllegalArgumentException("Request parameter 'pageId' must be a number") instead of the bare NumberFormatException, and the test asserts the message.

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 61.96%. Comparing base (9395042) to head (d85d1ba).

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #1674      +/-   ##
============================================
- Coverage     61.96%   61.96%   -0.01%     
+ Complexity    11286    11285       -1     
============================================
  Files          1250     1250              
  Lines         48437    48440       +3     
  Branches       6796     6796              
============================================
+ Hits          30014    30015       +1     
- Misses        15729    15730       +1     
- Partials       2694     2695       +1     

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.