refactor: split the ServerRpcHandler#handleRpc method

[mletenay]

Description

The ServerRpcHandler#handleRpc method parses the json and produces RpcRequest object which is then internally passed as parameter to handleInvocations() method, but discarded afterwards.

Adjust the handleRpc() API so the RpcRequest is not discarded at the end of method invocation, but is returned as method result.

Motivation: there are situations when customers extend the ServerRpcHandler logic and since the RpcRequest is lost, it needs to be re-parsed from json once again.
(Specific scenario - "UNLOAD" beacon requests are ignored when @PreserveOnRefresh is used, but that request needs to be specially processed anyway).

Type of change

• Bugfix
• Feature

Checklist

• I have read the contribution guide: https://vaadin.com/docs/latest/guide/contributing/overview/
• I have added a description following the guideline.
• The issue is created in the corresponding repository and I have referenced it.
• I have added tests to ensure my change is effective and works as intended.
• New and existing tests are passing locally with my change.
• I have performed self-review and corrected misspellings.

Additional for Feature type of change

• Enhancement / new feature was discussed in a corresponding GitHub issue and Acceptance Criteria were created.

[cla-assistant]

All committers have signed the CLA.

[cla-assistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[tltv]

Thanks for the contribution. This reveals that API could be easier to customize, but we are also talking about internal API here.
Main concern with this change is that if internal API like ServerRpcHandler exposes RpcRequest via handleRpc and then users starts to depend on it, we have absolutely no way to verify that their code works still after we make changes to code marked as internal API. It would be ideal to get new issues/feature tickets for the missing parts, like the mentioned beacon with PreserveOnRefresh exception, and which can be then fixed and tested.

Adding return value is breaking change for extenders. Even though it's in internal API, knowing that it will break someone's project makes one think some alternatives. Since there is already a workaround of creating your own RpcRequest instance, this change feels like the last effort.

Another option that wouldn't break extenders code is to make RpcRequest available via new method:

public void handleRpc(UI ui, String message, VaadinRequest request, RpcRequest rpcRequest) 
                   throws InvalidUIDLSecurityKeyException, MessageIdSyncException {
  // most of the code in handleRpc goes here
...

and existing method would call that:

    public void handleRpc(UI ui, String message, VaadinRequest request)
            throws InvalidUIDLSecurityKeyException, MessageIdSyncException {
        ui.getSession().setLastRequestTimestamp(System.currentTimeMillis());

        if (message == null || message.isEmpty()) {
            // The client sometimes sends empty messages, this is probably a bug
            return;
        }

        RpcRequest rpcRequest = new RpcRequest(message, request.getService()
                .getDeploymentConfiguration().isSyncIdCheckEnabled());

        handleRpc(ui, message, request, rpcRequest);
    }

[mletenay]

I understand and agree it is a breaking change and I also understand it is internal method.
I reverted the change and split the former handleRpc as suggested and also extracted the handleUnloadBeaconRequest for that specific need.
Methods have intentional protected visibility to make clear it is internal API only meant for rare cases when the class is extended.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud