fix: check for client route conflicts

[tepi]

Fixes #20144

[tepi]

Please feel free to test/review. Does need tests still.

[github-actions]

Test Results

1 142 files  ±0  1 142 suites  ±0   1h 27m 37s ⏱️ +24s
7 461 tests +2  7 411 ✅ +2  50 💤 ±0  0 ❌ ±0 
7 831 runs  +3  7 771 ✅ +3  60 💤 ±0  0 ❌ ±0 

Results for commit 055fed8. ± Comparison against base commit 4924daa.

♻️ This comment has been updated with latest results.

[tltv]

Client file-route like Frontend/views/hello-world/{{optionalId}}.tsx would end up matching with both hello-world and hello-world/:optionalId?. This might be OK with optional route param.

But it's same thing with required param with Frontend/views/hello-world/{requiredId}.tsx + hello-world/:requiredId? which seems wrong.

To avoid throwing exception in this case, there can be a check for viewInfo.children() to skip validation of client routes that have children.

[tepi]

But it's same thing with required param with Frontend/views/hello-world/{requiredId}.tsx + hello-world/:requiredId? which seems wrong.

To avoid throwing exception in this case, there can be a check for viewInfo.children() to skip validation of client routes that have children.

So to be clear, we want to allow a flow route 'hello-world' and hilla route 'hello-world/required-id' at the same time?

Edit: And I guess the same thing should work also if flow<->hilla routes are the other way around

[tltv]

So to be clear, we want to allow a flow route 'hello-world' and hilla route 'hello-world/required-id' at the same time?

Edit: And I guess the same thing should work also if flow<->hilla routes are the other way around

On second thought, I changed my mind.

It's actually hard to think of any real use cases for allowing that. It would be confusing to have /hello-world showing Flow view and hello-world/12345 showing Hilla view. Let's keep it strict unless someone has better reason to allow it anyway.

Docs is not saying anything regarding mixing or not mixing server and client routes this way

[tepi]

It's actually hard to think of any real use cases for allowing that. It would be confusing to have /hello-world showing Flow view and hello-world/12345 showing Hilla view. Let's keep it strict unless someone has better reason to allow it anyway.

I agree, and it can get even more complicated:

So in this case we could tentatively have the items/show on Flow side and items/:identifier on Hilla side. I'm not even sure what would happen with such configuration right now, my guess is items/show would just go the the parameterized Hilla view anyway.

Maybe we should instead document that at least currently such combinations with identical base route are not supported for mixing.

[tepi]

Please hold on merging for now. Still need to figure out at which phase it is best to check for the conflicts.

[tepi]

Update: Moved checking to end of VaadinService#init - everything seems to be ready for checking at that point.

[sonarcloud]

Quality Gate passed

Issues
97 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 SonarCloud

[tltv]

Update: Moved checking to end of VaadinService#init - everything seems to be ready for checking at that point.

Adding or changing client routes would not be detected even after page refresh. Could conflict check go to MenuRegistry#collectClientMenuItems(...), but only do the check if Map<String, AvailableViewInfo> is changed. Meaning that map hash should be stored somewhere and next call would check it for changes, before doing any validation. Point would be to make sure conflicts are checked on page refresh at least, if it's not possible to do it immediately after changes.

Hash or maybe even copy of the whole Map could be stored in VaadinContext in VaadinService.getCurrent().getContext(). But only in dev mode (AbstractConfiguration is already available in collectClientMenuItems).