Improved PathSpec handling for servletName & pathInfo

[joakime]

Introduction of MatchedResource and MatchedPath to capture the results of a PathSpec match.

[gregw]

A few things from a quick look through.
I don't love that we are not optimizing if there are Regex patterns, but I think that can be a TODO for later.

[gregw]

I feel that we probably can do optimized exact matches, even for regex etc. The Trie maps to a MappedResource that includes the PathSpec, so if match produces a non ServletPathSpec match, then we know we have to run it again to ensure we match any groups etc.

But maybe this is OK for now. Perhaps add a TODO to investigate if an exact map can be done.

[joakime]

Lets punt on optimized regex for now.

[joakime]

Added some comments about the concerns on exact/prefix/suffix and non-servlet path specs

[gregw]

Again, we should be able to extract the prefix from any PREFIX_GLOB and put that in the Trie.
So again, for now add a TODO to optimize later.

[joakime]

It's not quite as easy as you think.
The comments added show what needs to be resolved.
TODO added as well.

[gregw]

Hmmm I really don't like all these optimized booleans. Might just be better to null out the map/trie and have a null mean don't look up. That is a single test rather than always testing boolean then looking at the map/trie

[joakime]

That means creating the Trie anew with every .add() or .put()

[gregw]

This should be a final field as it will always be the immutable MatchedPath.from(_declaration, null)

[joakime]

final where? if you have multiple PathSpecs, you have multiple final fields, which means a lookup/map again.

[gregw]

This also needs to avoid allocation in the common case of "/foo" matching "/foo/*"

Suggested change

	if (isWildcardMatch(path))
	{
	String pathMatch = path;
	String pathInfo = null;
	if (path.length() != (_specLength - 2))
	{
	pathMatch = path.substring(0, _specLength - 2);
	pathInfo = path.substring(_specLength - 2);
	}
	return MatchedPath.from(pathMatch, pathInfo);
	if (isWildcardMatch(path))
	{
	if (path.length() == (_specLength - 2))
	return finalFieldAllocatedInConstructor;
	String pathMatch = anotherFinalFiledAllocatedInConstructor ;
	String pathInfo = path.substring(_specLength - 2);
	return MatchedPath.from(pathMatch, pathInfo);

[joakime]

You'll have a map of values, mapping each PathSpec and potential path.

[gregw]

The pathSpec itself should know it's prefix, so that is what is returned. No map needed.

[gregw]

@joakime do you want me to have a go at fixing some of these issues?

[joakime]

I addressed most of the review comments.

The comments about final fields for various things don't make much sense to me, as we are dealing with multiple PathSpec objects in the collection, so having a final field make sense will result in another Map lookup, which kinda defeats the whole final field benefit.

If you can make it work without a new collection/trie/etc, give it a whirl.

[gregw]

I'll do a PR against this PR addressing some of my performance concerns.... good task for on the train/plane tomorrow.

[joakime]

@gregw 🚩 bump 🚩 : this need attention for the next release.

[gregw]

Ack

[gregw]

@joakime I pushed a change to this branch that should improve performance.
You'll need to get somebody else to review now.

contributing now

[joakime]

+1 from me.
Now lets get @janbartel @lachlan-roberts @sbordet or @lorban to review it (and hopefully approve it)